Add command line flag for sorting options

[Davejkane]

Goal

To allow a command line flag (probably -s, --sort) to take an additional sort method, and sort the filepaths accordingly.

Implementation

Add a new command line flag option to cli.rs. -s for short, --sort for long. It should take a value.

possible values should be

• Breadth first (current directory, then sub directories, the sub-sub-directories etc.)
• Depth first (which appears to be the default)
• By date (will require looking at file metadata)
• By size (will require inspecting file size)
• Alphabetical (by filename only).

We'll need to check that the supplied option is valid, and if it is, after globbing, sort the Vec<PathBuf> accordingly.

Try to write this code in such a way that this function could eventually be used to sort the paths at runtime.

[NickHackman]

I'd be happy to take this one

[gurgalex]

@NickHackman All yours then.

[Davejkane]

@NickHackman awesome. You might want to use this method in the clap app.

I'm probably going to move the cli function call from program/mod.rs to main, because I'll need it for conditionally building the window in fullscreen (#39). But don't worry about that just now, it'll be a trivial refactor for you if I beat you to merge.

[NickHackman]

@Davejkane In advance, my apologies, I should've asked these sooner on rather than diving head first. my bad :(

I have a few specification questions, opening up with the larger ones what did you want breadth first/depth first to do exactly?

As of right now, it feels super awkward, after input/globing I get the parent directory of the inputted files and perform depth/breadth on that. That seems a little crazy, since doing that on "~/test.jpg" would pull in all pictures from $HOME, by default.

I think it'd make more sense instead of performing those sorts post input/globing would be to check to see if any of the inputs are directories and to perform them on them. While the other sorts only make sense in post, making me think splitting these up seems logical, would love to hear what your stance is on this and more specifics on what you'd like the output to be.

An introduction of a depth limit would be a good feature as well

Name/Date/Size all perform the same as GNU ls at the moment.

[Davejkane]

Don't worry about man, there's never a bad time for clarifications.

To be clear, we only want to sort the paths that we already have after collecting them. So once we have the vector we want to sort it. So your code won't have to do any globbing or directory traversal.

But you raise some important points about depth and breadth sorting. I was thinking that maybe there would be some way to determine the directory depth of a path, but I guess we could end up with a mixture of absolute and relative paths which would make this difficult. But basically if there is some way to determine the directory depth of paths, then breadth first should put the ones with lowest depth first, with alphabetical as tie breaker. Depth first would put the ones with the biggest directory depth first, also with alphabetical as tiebreaker.

If all this is too awkward, it might be enough for depth first to just be what is currently done, and breath first as reversing the vector.

Does that all make sense?

[Davejkane]

I guess a naive implementation would be to count the number of slashes (may need a separate Unix and windows function) in the stringified path.

[NickHackman]

To be clear, we only want to sort the paths that we already have after collecting them. So once we have the vector we want to sort it. So your code won't have to do any globbing or directory traversal.

Completely understand everything besides, directory traversal is necessary for breadth/depth

I just think that having the program do a depth first by default pulls in a lot of probably unintended images into riv, for instance all of firefox's cache.

I was thinking that maybe there would be some way to determine the directory depth of a path, but I guess we could end up with a mixture of absolute and relative paths which would make this difficult.

I'm mostly confused about the entry point, since we're doing this post the provided input isn't a directory, but a vector of paths to images. My algorithm follows this pseudo code

Breadth First: Find parent directory of image put parent in queue go through queue one by one if it's a directory push all its children on queue, if it's an image push onto vector (note I'm returning a new vector)

I'm mostly just wondering if this is the intended result? Personally if I wanted to add an "album" of images providing an directory as input and calling breadth/depth makes a lot of sense. While as a default it feels unintended.

I guess a naive implementation would be to count the number of slashes (may need a separate Unix and windows function) in the stringified path.

I believe this is builtin to std::path::PathBuf

[gurgalex]

since we're doing this post the provided input isn't a directory, but a vector of paths to images.

It may make sense to rename and repurpose current_dir for tracking the base directory of the glob search. https://github.com/Davejkane/riv/blame/a1c8a79e7569aaf40dce816e398e416d6cc6b598/src/paths.rs#L13

Examples:

# User launched riv in pictures directory
# folder for all images is `/home/user/pictures/`
riv **/*.png   

There's a potential limitation with globs (I think) that only directories where *.png is being performed will return pictures. A Possible feature request as it may be intended behavior of **/*.png glob.

# folder for all images is `/home/user/Downloads/`
riv /home/user/Downloads/**/*.png   # Does not return Downloads/file.png

So, any pictures that are in the base folder of Downloads, will not be returned, but any subfolders with pictures will.

[Davejkane]

I think we're confusing a breadth/depth first sort with a breadth/depth first search. That's probably my poor explanation. We don't need to do any directory traversal here. We're only sorting the Vector that we have. We're not finding extra image files or recursively traversing into directory paths that weren't provided to the program.

Also to be clear, we're no longer doing glob search in the program. It is now being handled by the shell. The shell I use is zsh, and maybe I have taken for granted that in zsh **/*.png will find all the png files in the current directory and all sub (and sub-sub and so on) directories. That could be an oversight on my part if that's not how bash works. But it is also how the rust glob library works (which we're currently not using).

Also, I'm not asking the program to do a depth first sort by default. That's just how it is. Whether using rust's glob library or letting the shell do it, that's what we get back by default. If I type ls **/*.jpg in the Downloads folder in my shell. The first entries are those two folders deep, and the last entries are the current directory files. I'm not a particular fan of that default behaviour, which is part of the motivation for this feature.

[Davejkane]

I think a better way of putting it would have been depth first vs depth last sort. Rather than depth first, vs breadth first sort.

[gurgalex]

Yeah, globstar (**) is disabled by default in bash, so that's why **/*.png was giving a different number of results. I didn't think shells behaved differently.

[NickHackman]

Man, I honestly had no idea you meant that, I implemented breadth/depth first search, thinking "Okay, this could be useful in specific cases, but it still feels weird". Now I think I got what you mean! :+1:

Just to clarify though, so you want depth first to put the farthest (from current_dir) possible images first, and breadth first to put the current directory first followed by each depth? And we're provided all the files we just have to arrange them in that order?

In this case adding a depth limit seems a lot easier, I'm going to add a --reverse/-r flag as well, unless you're opposed to it.

[Davejkane]

Exactly. Sorry for the shitty explanation. A reverse flag sounds like a nice bonus I hadn't thought of. Don't worry about depth limits just now. We'll maybe do that separately when we re-implement internal globbing, although I think a vector length limit would probably make more sense. I don't care how nested your file is if there's only two of them.

[gurgalex]

Closed by #51