Davejkane / riv

Riv - The Rust Image Viewer
MIT License
99 stars 10 forks source link

Refactor paths(with tests) and bulk operations #87

Closed gurgalex closed 5 years ago

gurgalex commented 5 years ago

Closes #48 cc: @Davejkane

why the refactor of paths?

Making the indexes and images fields private forces the user of access methods which at least make the caller consider the possibility of no images being present.

With private fields, removing an image must be done by calling a method which does the proper manipulations for the user.

Some methods now can limit how much mutable access (if at all) is given to the images vec. For instance, sorting the vector can do everything except that which alters its length. Some areas only need read access, so no need to give the ability to replace the whole vec outright. A dedicated reload_imags method is provided for this, which always makes sure the current image index is not larger than the last element's index.

private fields seemed to aid in writing tests as well.

MultiNormal mode

Typing numbers on either the numpad or keyboard while in Normal mode will now switch to MultiNormal mode. This allows for performing many repetitions of Normal actions, but in bulk, like jumping 10 images ahead.

Concerns with MultiNormal implementation

Small changes

NickHackman commented 5 years ago

@gurgalex Great work! Code review inc...

But first, initial thoughts: Ideally we follow Vim and put the numerical input on the right side of the infobar, since knowing your current index or image would impact your action. Also this is a really big PR, in the future breaking it up would be better in my personal opinion.

gurgalex commented 5 years ago

@Davejkane I currently have the . (dot/period) presses only perform the last non-dot pressed action. 2jand then . would perform the same operation 2j 2. would perform 2j 3. would perform 3j

Did you want the dot presses to multiply as well? Example 2j last action: 2j . would perform 2j 2. would perform 4j 2. again would perform 8j . would now perform 8j

gurgalex commented 5 years ago

@Davejkane I believe all current concerns are addressed.

Davejkane commented 5 years ago

I just checked with vim and it appears that the way you're doing it, without multiplication is correct. I'll try to get around to code review today.