Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ui): show filenames in thumbnail grid (Closes #85) #633

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

CyanVoxel
Copy link
Member

@CyanVoxel CyanVoxel commented Dec 10, 2024

This PR adds the ability display an entry's filename underneath its thumbnail in the grid view. This feature can be toggled on/off through the View -> "Show Filenames in Grid" menu option. I've also organized some of the existing menu options to make more sense, notably removing the "Window" menu.

image

@CyanVoxel CyanVoxel added Type: QoL A quality of life (QoL) enhancement or suggestion Type: UI/UX User interface and/or user experience Priority: Medium An issue that shouldn't be be saved for last labels Dec 10, 2024
@CyanVoxel CyanVoxel added this to the Alpha v9.5 (Post-SQL) milestone Dec 10, 2024
@CyanVoxel CyanVoxel linked an issue Dec 10, 2024 that may be closed by this pull request
@CyanVoxel CyanVoxel changed the title feat: show filenames in thumbnail grid (Close #85) feat(ui): show filenames in thumbnail grid (Closes #85) Dec 10, 2024
@CyanVoxel CyanVoxel marked this pull request as ready for review December 11, 2024 01:58
@CyanVoxel CyanVoxel added the Status: Review Needed A review of this is needed label Dec 11, 2024
Copy link
Collaborator

@Computerdores Computerdores left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the "Open Library on Start" / "Show Recent Libraries" Stuff even be included here?

@@ -551,6 +568,10 @@ def toggle_libs_list(self, value: bool):
self.preview_panel.libs_flow_container.hide()
self.preview_panel.update()

def toggle_grid_filenames(self, value: bool):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't "toggle" it "sets"; it should probably not be called toggle_grid_filenames

def set_filename(self, filename: Path | str | None):
self.file_label.setText(str(filename))

def toggle_filename(self, value: bool):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as with the other method; Doesn't toggle and should therefore probably not be called such

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there intentional changes to this? If not should probably not merge this for fear of overwriting other intentional changes (Note: Maybe we should add this file to the .gitignore so it doesn't get accidentally included in commits?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoot, I didn't realize something happened to this file when committing (nor what) - and since it's part of a test that we can change I don't think we can get away with adding it to the .gitignore...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could add something to the test harness that automatically reverts any changes to it when cleaning up, unless it had already been changed during init of the harness?
(not sure about the details; reverting changes should be easy, but checking whether it has been changed, no idea)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further note: There is a tool called sqldiff made by SQLite that checks two sqlite files for differences, maybe that could be used.
It only checks whether data in the tables has changed, while git would also report metadata changing. So if sqldiff finds no differences but git does it could be reset?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the tests clean it up could work as a temporary solution or as a good solution for tests that require modifying a fixture database, however this database should not be being modified here and signals that there's a deeper issue.

We also went in depth with sqldiff, sqlite cli, git text diffs, even a hex editor yesterday and narrowed down the problem a bit. It seems to be the internal file change counter value being updated, and I've opened up a dedicated issue for it here: #644

open_on_start_action.triggered.connect(
lambda checked: self.settings.setValue(SettingItems.START_LOAD_LAST, checked)
)
file_menu.addAction(open_on_start_action)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Open Library on Start" option exists twice, once in View and once in File, and (un)checking one of them doesn't update the visual state of the other

@CyanVoxel
Copy link
Member Author

Should the "Open Library on Start" / "Show Recent Libraries" Stuff even be included here?

The incorrect placements and names of these actions were getting in my way when adding the new action, so I went ahead and moved them to more accurate places, both in the code and in the menus themselves.

@Computerdores

This comment was marked as outdated.

@Computerdores
Copy link
Collaborator

Should the "Open Library on Start" / "Show Recent Libraries" Stuff even be included here?

The incorrect placements and names of these actions were getting in my way when adding the new action, so I went ahead and moved them to more accurate places, both in the code and in the menus themselves.

Maybe reference that in the PR description / title, but sounds good

@CyanVoxel
Copy link
Member Author

During the start I get a really weird issue where a small window will rapidly open and then close again: image (you can see how in the task bar there is two little bars, those are from instances of that little window in the middle) I think this rapidly opening and closing window might have something to do with the splash, however the splash is still shown.

First bad commit is: a93809d.

I got this error yesterday on Windows as well, but it should've been fixed by 901ec7c? 👀

@CyanVoxel
Copy link
Member Author

Should the "Open Library on Start" / "Show Recent Libraries" Stuff even be included here?

The incorrect placements and names of these actions were getting in my way when adding the new action, so I went ahead and moved them to more accurate places, both in the code and in the menus themselves.

Maybe reference that in the PR description / title, but sounds good

This is what I was referring to in the description with "I've also organized some of the existing menu options to make more sense, notably removing the "Window" menu", but maybe I could've been more clear about it

@CyanVoxel CyanVoxel removed the Status: Review Needed A review of this is needed label Dec 14, 2024
Copy link
Collaborator

@Computerdores Computerdores left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything was addressed and I wasn't able to find any other issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium An issue that shouldn't be be saved for last Type: QoL A quality of life (QoL) enhancement or suggestion Type: UI/UX User interface and/or user experience
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

[Suggestion] File name displayed in grid view
2 participants