Refactor paths(with tests) and bulk operations #87

gurgalex · 2019-05-30T07:37:37Z

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

Lots of code duplication between handling of actions of MultiNormal and Normal mode. Any ideas for somewhat merging the two modes?
Should actions that can only be performed once (like toggling fullscreen) be allowed as actions in MultiNormal mode. Example 22f would only toggle the state of fullscreen once.
Infobar display of times to repeat. Should the numbers be on the right like vim?

Small changes

Made zooming in and out undo each other (*1.1 and /1.1)
Removed duplication of process_action. It is present only once for each mode, instead of needing duplication on every keypress/text match.

Add tests to increase confidence in correctness

Add MultiNormal mode and ProccessAction for repeating actions

Command -> Normal MultiNormal -> Normal

Reduce code duplication

Functioning multiple move and delete

NickHackman · 2019-05-30T16:11:29Z

@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.

NickHackman

I think that's all I have for right now, mostly just my two cents. I'm probably going to look at it again at a later point, maybe tomorrow I don't know.

As per usual @gurgalex really good code I just think there are some things that maybe would make your life easier. ❤️

NickHackman · 2019-05-30T17:01:54Z

src/program/command_mode.rs

-            Some(self.paths.images[self.paths.index].to_owned())
-        } else {
-            None
+        let target_path = match self.paths.current_image_path() {


In this instance, sorry if this was unclear, we want the image path at the current index because we want to find the current image if it occurs in the new glob.

In this match it returns, but it really should just be None, because we have no image to find.
This causes newglob to fail on a start that is empty, your comment is right we don't have any images, but that's a 👌

This is why later on in Development there's this

if let Some(target_path) = target { // <---- here, because we don't have any images // find location of current image, if it exists in self.paths.images match self .paths .images .iter() .position(|path| path == &target_path) { Some(new_index) => self.paths.index = new_index, None => { self.paths.index = 0; } } }

Believe this is fixed with the latest changes.

NickHackman · 2019-05-30T17:44:21Z

src/program/mod.rs

-    pub fn skip_forward(&mut self) -> Result<(), String> {
-        let skip_size = compute_skip_size(&self.paths.images);
-        self.increment(skip_size)
+    pub fn skip_forward(&mut self, times: usize) -> Result<(), String> {


So Okay, I could be wrong here, but hear me out.

So in every function you have it taking a usize called times. This is a suitable solution, but they all have the same function definition... pub fn name(&mut self, times: usize) -> Result<(), String> There's gotta be a way to simplify this down, right? Using generics and using functions?

Repeating of function signatures seems fine, as each does something different.

I'm mostly concerned with the near duplication that run_multi_normal_mode does of run_normal_mode
https://github.com/Davejkane/riv/pull/87/files#diff-52fc4303c2b25fd9f3edcce1fbd21455R497

NickHackman · 2019-05-30T18:25:23Z

src/program/mod.rs

+                            } => match (a, n) {
+                                (Action::Backspace, _) => {
+                                    // Divide repeat by int 10 to remove last digit
+                                    self.ui_state.repeat /= 10;


@Davejkane I'm not sure on how you want this to perform, but in Vim if you backspace at any point it just clears the entire buffer rather than removing one of the numbers.

NickHackman · 2019-05-30T18:27:35Z

src/program/render.rs

@@ -366,6 +371,7 @@ fn normal_help_text() -> Vec<&'static str> {
        "+------------+----------------------------+-----------------------------------------------------+",
        "| Key 1      | Key 2                      | Action                                              |",
        "+------------+----------------------------+-----------------------------------------------------+",
+        "| 0-9        |                            | Perform the below actions in bulk                   |",


This is a huge nitpick, but maybe include that it takes one of the below operations?

True
In the Key 2 slot.
Text: Key of the action to perform

NickHackman · 2019-05-30T18:41:17Z

src/ui.rs

+
+/// Perform an Action `times` times
+#[derive(Clone, Debug)]
+pub struct ProcessAction<'a> {


If you're just associating two variables, why not use a tuple? Is there a benefit of a struct versus a tuple?

I went with a struct for the labeled fields (action, and times).
Also being able to implement conversions (such as from ProcessAction -> MultiAction)

NickHackman · 2019-05-30T18:52:27Z

src/ui.rs

+            "0" | "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9" => {
+                let previous_count = state.repeat;
+                // Safe to unwrap as only digits were matched
+                let next_digit = text.parse::<usize>().unwrap();


Why parse and add it on here? Instead of just storing it in a String or maybe Cow borrowed string and then when the user hits an action then parse it? This would also make it so you don't have to divide by 10 in mod.rs to truncate a digit

Parsing immediately was one way to reduce code duplication when the text is not 0-9.
The amount of times would not have to be parsed on every action.
I think I could move the parsing out of the ui event -> MultiNormalAction translation if Normal mode read from the same field repeat.

NickHackman · 2019-05-30T18:54:08Z

src/ui.rs

+                let first_digit = text.parse::<usize>().unwrap();
+                // Save the first digit before switching
+                state.repeat = first_digit;
+                Action::SwitchMultiNormalMode


As stated above, maybe have the Mode store the number of times to repeat? (As a String or Cow maybe?) I don't know enough about Cow yet...

The artificial length is the images vec length unless capped by the user

Davejkane

@gurgalex, first off thanks for the great work. I knew this would probably be our biggest feature to date, but it's also the feature that's really going to make riv unique, so thanks for taking it on.

I haven't had time just yet to do some proper code review, but I have had the chance to do some QA.

When I use 23j for instance, after rendering the 23, it then renders the old index very briefly before rendering the new index. It's jarring. But this could potentially be taken care of by rendering the number at the right side of the bar along with the rest of the bar.

I want 56G to go the 56th image exactly. but right now it goes to the end. That's a feature request.

I want the dot operator to work with bulk operations. If I press 10k then I want every . press to also be 10k. Currently . will just be k.

Only update display in MultiNormal when deleting digits

gurgalex · 2019-05-31T23:05:52Z

@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

…efactor_paths_and_test_bulk_operations

gurgalex · 2019-05-31T23:33:41Z

@Davejkane
I believe all current concerns are addressed.

Davejkane · 2019-06-01T05:23:52Z

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.

Davejkane

This all looks great to me. Really good job. Please squash and merge and tidy up the commit message.

gurgalex added 23 commits May 29, 2019 13:12

Draft refactor of paths

e035da3

Refactor paths data struct. More private

a62d819

Add tests to increase confidence in correctness

Doc fix (None is unlimited)

1278d8e

Impl default for ui::State

30361bb

MultiAction draft

2f03b56

Add MultiNormal mode enum and struct

c23242c

Refactor to reduce process_action duplication

58670a8

Add MultiNormal mode and ProccessAction for repeating actions

Add MultiMode skeleton quit functionality and log

2ce0759

Furth privatize paths access

46cd592

Add test for capping index when reloading images

2e70cc3

Copy file does not need mutable access

0f38aee

Implement multiple copies at once

fe37e9a

Zoom out and In multple. Diagnose zoom reset transition normal

e7c1f31

Fix rebase from development

bbe9917

Fix zoom resetting when exiting modes

085e263

Command -> Normal MultiNormal -> Normal

Use one copy method for every copy action

4cc0123

Zoom in and then out should have same inc/dec

8d2133a

Reduce code duplication

fmt

b18a460

Internalize zoom in and out. Tests

6f33d80

Move and delete images can delete multiple

0ede4e7

Functioning multiple move and delete

Clarify private get fn docs

4aa4070

pan up, down, left, right, skip(forward and back) now times to repeat

978e2d5

MultiNormal mode input visible and backspace

be3cad4

gurgalex requested a review from Davejkane May 30, 2019 07:37

gurgalex added 6 commits May 30, 2019 02:39

Clippy and fmt

880a665

Remove redundant import

3a52516

Update docs for paths (panic heading)

c80d6de

Paths docs clarification

dada66e

Update Normal help and READEM with numeric keys

aceaf20

Add currently failing test to fix

46c4416

gurgalex force-pushed the refactor_paths_and_test_bulk_operations branch from 2a02a5c to 46c4416 Compare May 30, 2019 10:37

NickHackman suggested changes May 30, 2019

View reviewed changes

gurgalex added 13 commits May 30, 2019 19:51

Test fixes. Rename soft_cap -> art_len

488680b

The artificial length is the images vec length unless capped by the user

Extra space

fff0de7

Draft reduction in duplication multi an normal

bb4253c

Fix success message not disappearing

87c9bd8

Fix 100% cpu message. Move sleep out one bracket

eeaaa80

remove contracting doc comment

76132d3

Update help and README with better bulk op doc

68fb8f4

Take reference of event for processing

f4a7681

Remove ProcessAction duplication. Fix success msg MultiNormal Mode

960698e

fix doc stating where repeat count is stored

23aa827

document normal mode action handler

a0b79cc

fmt

9e7185d

Allow single use actions in MultiMode. repeat once

404626f

Davejkane requested changes May 31, 2019

View reviewed changes

gurgalex added 3 commits May 31, 2019 03:19

MultiNormal G goes to specified image

4ede986

Render on exit MultiNormal in one go

eeff0ab

Only update display in MultiNormal when deleting digits

comment reword

0c7d97d

gurgalex added 3 commits May 31, 2019 18:18

Period now works with bulk actions

0b18215

Merge branch 'development' of https://github.com/Davejkane/riv into r…

048a863

…efactor_paths_and_test_bulk_operations

Fix MultiNormal G not rendering Normal Mode

4523b7d

Davejkane approved these changes Jun 1, 2019

View reviewed changes

Add help message for G for bulk ops

6cc076b

gurgalex merged commit 5053228 into Davejkane:development Jun 1, 2019

gurgalex deleted the refactor_paths_and_test_bulk_operations branch June 8, 2019 03:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor paths(with tests) and bulk operations #87

Refactor paths(with tests) and bulk operations #87

gurgalex commented May 30, 2019 •

edited

Loading

NickHackman commented May 30, 2019 •

edited

Loading

NickHackman left a comment

NickHackman May 30, 2019

gurgalex May 31, 2019

NickHackman May 30, 2019

gurgalex May 31, 2019

NickHackman May 30, 2019

NickHackman May 30, 2019

gurgalex May 30, 2019

NickHackman May 30, 2019

gurgalex May 30, 2019

NickHackman May 30, 2019

gurgalex May 30, 2019

NickHackman May 30, 2019

Davejkane left a comment

gurgalex commented May 31, 2019

gurgalex commented May 31, 2019 •

edited

Loading

Davejkane commented Jun 1, 2019

Davejkane left a comment

Refactor paths(with tests) and bulk operations #87

Refactor paths(with tests) and bulk operations #87

Conversation

gurgalex commented May 30, 2019 • edited Loading

why the refactor of paths?

MultiNormal mode

Concerns with MultiNormal implementation

Small changes

NickHackman commented May 30, 2019 • edited Loading

NickHackman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Davejkane left a comment

Choose a reason for hiding this comment

gurgalex commented May 31, 2019

gurgalex commented May 31, 2019 • edited Loading

Davejkane commented Jun 1, 2019

Davejkane left a comment

Choose a reason for hiding this comment

gurgalex commented May 30, 2019 •

edited

Loading

NickHackman commented May 30, 2019 •

edited

Loading

gurgalex commented May 31, 2019 •

edited

Loading