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.

Sign up for GitHub

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

Jump to bottom

Setting to set the desired file ordering #387

Closed
wants to merge 11 commits into from
Closed

Setting to set the desired file ordering #387

wants to merge 11 commits into from

Conversation

jcjgraf
Copy link
Contributor

@jcjgraf jcjgraf commented May 5, 2021 •
edited
Loading

Adds settings to specify the desired ordering of the images and the desired order of folders.

Todo:

  • Seperate options for folder and images
  • Size for directory not working
  • Alias to conveniently change the ordering; @karlch do you like them or shall I get rid of them again?
  • Implement other types of ordering. @ALL any ideas?
  • Test
    • Unit test natural sort
    • files.order using mock
    • OrderSetting
    • end2end
  • Changelog

Partially based on #76. Some code was taken directly from there

resolves #75

@jcjgraf jcjgraf force-pushed the feature/ImageOrder branch 9 times, most recently from c262a33 to 54a95e7 Compare May 9, 2021 18:35
@jcjgraf jcjgraf mentioned this pull request May 9, 2021
Closed
@jcjgraf jcjgraf force-pushed the feature/ImageOrder branch 3 times, most recently from 9a352d8 to e18aec5 Compare May 11, 2021 19:21
@karlch
Copy link
Owner

karlch commented May 13, 2021

Thanks a lot, I like where this is going! 😊

I am not quite sure about the aliases. On the plus-side, they are discoverable when typing :order. The disadvantage is that they don't have the nice completion that :set ... offers which makes them nearly useless for people that just "discovered" them without looking at the documentation.

Let me know when you are ready for a more detailed review.

@jcjgraf
Copy link
Contributor Author

jcjgraf commented May 13, 2021 •
edited
Loading

The discoverability was indeed the main reason why I have added the aliases. But as you say, without the completion they are anyway useless. I will remove them again...

Function-vise I am done. So I guess you can start reviewing that.

Concerning the test, I think I need some advice from you. So far I have added the some basic settings tests in test_settings and fixed a testcase in test_files (which expected files.listdir to list files ordered). Now, should every single ordering method be testes? If so, in what file? Also, would it be a good Idea to add an end2end test to test if the order in the library really changes when changing this option? What else should I add testcases for?

I have also thought about renaming renamed the ordering as follows. Do you have any opinion or suggestions?

Current New
name alphabetical
name-desc alphabetical-desc
name-natural natural
name-natural-desc natural-desc
modify recently-modified-first
modify-desc recently-modified-last
size smallest-first
size-desc largest-first

@jcjgraf jcjgraf force-pushed the feature/ImageOrder branch 2 times, most recently from 0f7aca5 to b2ac50d Compare May 20, 2021 14:50
karlch
karlch reviewed May 23, 2021
View reviewed changes
Copy link
Owner

@karlch karlch left a comment

Choose a reason for hiding this comment

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

Thanks again, I really like where this is going and am very fond of the new names for the ordering types. I left a few minor comments concerning the implementation.

Concerning the tests, I am fine with not testing every single ordering function that is trivial, however the natural sorting would be nice to test in the test_settings module as it is a class method.

One end2end test checking if the order changes in the library when the setting is changed would be great 😊

vimiv/api/settings.py Outdated Show resolved Hide resolved
vimiv/utils/files.py Outdated Show resolved Hide resolved
vimiv/api/working_directory.py Outdated Show resolved Hide resolved
vimiv/api/settings.py Outdated Show resolved Hide resolved
@jcjgraf jcjgraf force-pushed the feature/ImageOrder branch 11 times, most recently from 726e969 to 6efe2d5 Compare August 1, 2021 15:15
@karlch
Copy link
Owner

karlch commented Aug 8, 2021

This is now almost complete, right? Just some minor work on the testing, add to changelog, and then we are ready to merge? 😊

@jcjgraf
Copy link
Contributor Author

jcjgraf commented Aug 8, 2021

Thanks for your comments, it will look into them. Yes, this PR is pretty much complete. I just have to create an E2E test to test the general behaviour and mock the test_order, as indicated by you first comment. Since I am still not that familiar with PyTest (or testing in general) it may take some time till I am done with that 🙃

@karlch
Copy link
Owner

karlch commented Aug 8, 2021

Sounds good, as always take all the time you need 😊

@jcjgraf jcjgraf mentioned this pull request May 15, 2022
Closed
@jcjgraf
NEW: Setting to set the desired file ordering
2b6c9eb 
The option `global.image_order` specifies the order of files. Files can
be ordere according to their name, size or modified time in ascending
and descending order. The order can be set using the ordinary
`:set image_order <ordering>` command.

Some code was take directly from #76
@jcjgraf
NEW: specify order of images and directoreis seperately
beb4840 
Add new option `directory_order` which specifies the order of the
directores. `image_order` only influences the order of the images.
@jcjgraf
FIX: test expected that files.listdir returns files sorted
edd4514
@jcjgraf
NEW: Option for natural sorting
c0e13a8
@jcjgraf
FIX: Import order
76bde48
@jcjgraf
NEW: Order_Setting test
ccc6af6
@jcjgraf
IMPROVE: Rename orderings
c6665b0 
| Current           | New                     |
| :---              | :---                    |
| name              | alphabetical            |
| name-desc         | alphabetical-desc       |
| name-natural      | natural                 |
| name-natural-desc | natural-desc            |
| modify            | recently-modified-first |
| modify-desc       | recently-modified-last  |
| size              | smallest-first          |
| size-desc         | largest-first           |
@jcjgraf
IMPROVE: Merge ImageOrderSetting and DirectoryOrderSetting
b7ccd82 
Three classes for these order seemed a bit overkill, especially since
most orderings are shared.

I did not figure out the correct syntax to reference the `_natural_sort`
method from the class variable `ORDER_TYPES`.
@jcjgraf
IMPROVE: Unpack function arguments before calling files.order
7cb38a9
@jcjgraf
NEW: unit test natural sort
d66abbb
@jcjgraf
IMPROVE: order test
21e472a
@jcjgraf jcjgraf mentioned this pull request Aug 4, 2022
Closed
@jcjgraf jcjgraf mentioned this pull request Jan 3, 2023
Open
@karlch
Copy link
Owner

karlch commented Jan 6, 2023

I had a brief look at this, seems almost ready to go, would you mind if I merge to a branch and try to finalise this while you work on #467 over the week-end?

@jcjgraf
Copy link
Contributor Author

jcjgraf commented Jan 6, 2023 •
edited
Loading

Indeed, functionality-vise everything should be done. However, I have not looked at the code myself for over 1 year. 🙈 You can merge and finalize it, or let me rebase and go over it again. However you like 😊

EDIT
Maybe to sort options alphabetical-desc and natural-desc should be renamed have suffix -inv (inverse), as descending is a bit ambiguous.

@karlch
Copy link
Owner

karlch commented Jan 6, 2023

I'm happy to take a shot, I already rebased and looked into the code, should be rather straightforward as it is looking good 😊

inv sound good, I would actually even write it out, i.e. inverse as we have completion anyway, so shortening shouldn't be needed.

@jcjgraf
Copy link
Contributor Author

jcjgraf commented Jan 6, 2023

Sounds good, happy coding session 😊

And feel free to squash all my commits as the history is rather dirty 🙈

@karlch karlch mentioned this pull request Jan 7, 2023
Merged
@jcjgraf
Copy link
Contributor Author

jcjgraf commented Jan 8, 2023

Closing, as #563 has been merged.

@jcjgraf jcjgraf closed this Jan 8, 2023
@jcjgraf jcjgraf deleted the feature/ImageOrder branch January 8, 2023 15:13
@jcjgraf jcjgraf restored the feature/ImageOrder branch January 8, 2023 22:03
@jcjgraf jcjgraf deleted the feature/ImageOrder branch June 24, 2023 10:47
@jcjgraf jcjgraf restored the feature/ImageOrder branch June 24, 2023 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karlch karlch karlch left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Different image sorting than by name
2 participants
@jcjgraf @karlch