-
-
Notifications
You must be signed in to change notification settings - Fork 394
feat: add filename sorting #842
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: add filename sorting #842
Conversation
this is now ready for review |
I apologize for the inconvenience, #844 has now been pulled and a rebase targeting |
620cf6c
to
e358594
Compare
rebase is done. I have also fixed the search test by updating the search library to the latest db version. Further I have added a db migration test for the new migration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good here, the only rough spot I've run into is that it can take some time for the new filename column to populate when opening a library (45 seconds for 200k files on an SSD for me).
It may be a good idea (in a future PR if antyhing) to reformat the SQL to SQL migration logic in a way similar to how the JSON to SQL logic checks in with the frontend first before migrating, just to allow for something like an indefinite progress bar popup notifying the user that things are still happening. Though this process also took place during the splash screen for me since I used the launch flags, so maybe there's even more than needs to be considered first.
In any case, thank you so much for your work on adding this! And thank you for updating the test files as well!
Summary
Note: The failing test is due to an issue that is present on main and has gone unnoticed until now. I have added the missing assertion so it should be easy to fix in a separate PR.
Tasks Completed