Feat command mode #58

NickHackman · 2019-05-18T17:30:10Z

Features

infobar displays command mode input, and errors

Command Mode Features

 * h/help
 * q/quit
 * sort
 * ng/newglob, handles `~` and `$HOME` properly and directories are assumed to be `/dir/*`
 * df/destfolder
 * r/reverse
 * m/max

Known bugs:

In command mode: cannot type : due to the event buffer not being cleared this character is ignored, because it will commonly be added on first loop of get_command in /src/program/command_mode.rs
When program starts in a directory that has no images, Command mode isn't properly displayed on the infobar

Future Fixes

Decouple the rendering so that I and @gurgalex can update the infobar without having to pass the info to display to render_screen, allow updating of smaller components like the infobar before calling render or some other solution.
Force rendering, workaround solution being used for cases like newglob which should re-render when the current viewed image isn't in the new glob.

@gurgalex Here's how I'm handling it, few bugs I wanted to iron out before setting up the pull request

* Colon now enters command mode * Added a modal approach kept track of in Program struct Current modes: normal, command, exit * Changed previous fn run -> fn run_normal_mode * Add basis for future command mode commands

* Modified infobar so that it takes a &str called msg in the case of command mode displays what the user is currently typing * Add :help to print the help message and return to normal mode * Add :quit to terminate the program from command mode

Command mode is any mode that requires user input * Moved command_mode related functions into its own file * Command Mode Features: * newglob (takes only one paramter). Properly handles ~/$HOME when provided by user. When the current path exists in the current vec of images and in the future one, remain at its index * sort (optionally one paramter). if called alone just performs the selected sort on the images, if provided with a second argument (a SortOrder) switch to that then sort. User is moved to new index of current image prior to sort * reverse, reverses the current set of images, keeping the index of the current image * destfolder, sets the destination folder * help, exits command mode and displays help info in normal mode * quit, terminates program * info bar displays user input, and that the user is in command mode * window event handling while in command mode

* Moved the event match and Mode to ui.rs * Fixed bug, where input for :max could be 0 and it would be interpreted as 0 instead of infinitely many

Increased performance, inside command_mode function get_command by not rendering user input ever event, but only when it changes

src/program/command_mode.rs

NickHackman · 2019-05-19T04:50:03Z

@Davejkane, @gurgalex potential future issues based off of work done in command mode and discussed in #57

Future Feature Issues

Infobar is easier to update/decoupled from the rest of rendering or refractored in a way where smaller components of rendering can be updated or rendered without rerendering everything
Good First Issue Info bar accepts the input of error messages, changes the current image index field to a red color and displays "Error" and then the error message in the current image path field. Update errors to now, if possible, be sent to the infobar removing "Error:" when necessary from the message
Good First Issue Add the add command to command mode, which should follow the format :add glob file directory or :a glob file directory and append images to self.paths.images
Add autocomplete for command mode for files, commands, etc
Force rendering of application as mentioned above
Good First Issue Keep a history of command mode commands entered by user

Any other ideas? I feel like these are good first issues that require some work, but not like a crazy amount where they need to understand how the app functions as a whole.

edit: moved from a sub comment into main discussion.

* Enum that lists out the possible commands and implements FromStr for each

gurgalex · 2019-05-19T06:32:51Z

Good First Issue Add the delete command to command mode, which should follow the format :delete /path/to/image regex or :d /path/to/image otherwise delete current image from self.paths.images. Don't actually delete the file

Maybe untrack?. I'd like to reserve the word delete or remove for actually deleting an mage from storage.
Yes, the fn name currently is called remove_image, so possibly need to rename that sometime.

* Newglob wouldn't properly accept files like /home/etc/test\ file\ name * Add dependency regex to be able to fix Newglob bug * Add dependency shellexpand to properly handle ~/$HOME/env vars * Minor refractors

* Render self.render_blank now passes command mode input to it to properly render, so the user can populate the images * Fixed bug, in newglob where it would crash on empty self.paths.images

NickHackman · 2019-05-19T16:56:10Z

Update on progress:

Added dependency shellexpand to better expand ~ and $HOME and other env variables. Note shellexpand uses deprecated std::env::home_dir pull request sent in fixing that.
Fixed bug, where command bar wouldn't be rendered on empty images
Fixed bug, mentioned by @gurgalex where newglob wouldn't accept paths like /home/etc/path\ to\ dir/* by adding dependency regex (will be used later on for search as well)
Overall, restructure of code base in src/program/command_mode.rs to be more intuitive

Opening code for review

… into feat-redefine-glob

Davejkane

I've made a couple of comments, nothing too major. In general I'm impressed with the code and certainly with the enthusiasm to just jump in and make such a huge feature. The project is going to be all the better for it.

src/program/command_mode.rs

src/infobar.rs

src/program/command_mode.rs

Davejkane · 2019-05-19T19:18:55Z

src/ui.rs

-            Home => state.process_action(Action::First),
-            End => state.process_action(Action::Last),
+            Semicolon => {
+                if state.left_shift || state.right_shift {


Really not sure this holds true on non-american keyboards. Shift semicolon does not always equal colon. Please see if there's another way to do this. Maybe a virtual keycode if you can get them working.

src/ui.rs

This solution path was taken in response to the sketchy passing command to render_screen. This is a more proactive approach that is easier to adapt in the future. * Add Mode, Error, to display error messages * ui.rs Mode variants now take Strings if they need to display on infobar * Fixed documentation, removed TODOs * Infobar::update now matches on Modes

gurgalex

Avoid recompiling the regex every time it is called. See https://docs.rs/regex/1.1.6/regex/#example-avoid-compiling-the-same-regex-in-a-loop

src/program/command_mode.rs

NickHackman · 2019-05-20T20:19:00Z

@Davejkane thanks for the advice/review! in the future I'm going to have a more cross platform mindset compared to currently as that seems like a big issue. In addition to that a more proactive mindset as well some of the code I wrote in this was quick and dirty/sketchy. I'm going to iron out these issues ASAP 😃

Yeah... this seemed like it wouldn't be too bad going in, but quickly became this behemoth.

I think that we should remove all the normal mode features from src/program/mod.rs -> src/program/normal_mode.rs or something along those lines to better layout the project, just a thought though

Davejkane

Still a few little things. I would like us to try to support windows if at all possible. At some point it would be nice to get a windows user on board. I have no idea if this even works on windows.

src/ui.rs

src/program/command_mode.rs

gurgalex · 2019-05-21T22:31:38Z

Vim seems to use @: for repeating the last operation in command mode.
https://vim.fandom.com/wiki/Repeat_last_change

gurgalex · 2019-05-21T22:32:21Z

We could address this later though.

NickHackman · 2019-05-21T22:32:55Z

@gurgalex that sounds like maybe a different PR, but I like the idea

Davejkane · 2019-05-22T05:32:21Z

@NickHackman wow, I'm impressed you got through so many tweaks. One of the big ones left is still that when I move or delete an image, the len of the images vector isn't being updated, meaning that then visiting the last file causes an out of bounds panic.

* delete and move would panic when deleting/moving from the end or on empty * command mode destfolder/df wouldn't remove escaped spaces

NickHackman · 2019-05-22T06:28:56Z

@Davejkane I don't think I wrote that bug, but fixed 👍

edit: Yeah, sorry that is certainly my fault

Davejkane · 2019-05-22T06:36:24Z

It was working fine before the new max_viewable logic. And as far as I can tell, it isn't fixed. I just tried to delete the last image when I had 4 images, and it crashed the program.

NickHackman · 2019-05-22T06:43:49Z

@Davejkane I'm confused, that's not reproducible on my end.
You updated your local repo after the push right?

Davejkane · 2019-05-22T07:03:54Z

OK, just tried again and that seems to be fine, think pulling the branch didn't work. The program now seems to be exiting with this error message now Error: "SDL2 error: Text has zero width" when I hit q or esc. I promise that's my last gripe.

src/program/render.rs

src/infobar.rs

* Infobar was rendering empty Strings for the Exit mode

* If destfolder is still 'keep' change it whenever newglob changes directories

NickHackman · 2019-05-22T16:21:35Z

Honestly guys thank you so much for all the help, I didn't handle so many edge cases that would've come back to haunt me in the near future. I think we ironed at least a large majority of them, can never say they're all gone, but all the ones that I've tested for are gone. I'm honestly really happy with the result and the level of code quality 😄

It was a joy working with you both on this, and I hope to contribute to the project more in the future ❤️

Probably going to do #53 after this

gurgalex

@NickHackman
Thanks for all the help.

Should be the last change request from me.

Rename paths.current_dir to base_dir
Keep selected destination folder the same even if a new glob no longer contains the destination folder.

Example

Selected df
/home/alex/pictures

Current glob
/home/alex/*.png

New glob
/home/alex/Downloads/*.png

Destination folder is still
/home/alex/pictures

gurgalex · 2019-05-22T16:57:15Z

src/program/command_mode.rs

@@ -310,6 +335,9 @@ impl<'a> Program<'a> {
                    return Ok(());
                }
                self.newglob(&arguments);
+                if self.paths.dest_folder.ends_with("keep") {
+                    self.paths.dest_folder = self.paths.current_dir.join("keep");


@NickHackman

Sorry to request more changes.

The destination folder should be kept the same unless the user changes it with df

I believe the field current_dir should be called base_dir to say that this is the base directory from which the glob finds images.

In your above example that's how it should function currently.

The only instance it would change in newglob is if the user hasn't modified the default destination folder. If that's the case we should maintain the invariant, stated in help/README, that the default folder should be ./keep

I think that would make sense, for instance running riv in /riv/src/program initially and going to ~/Pictures and hitting move it'd be really weird if we moved the image to /riv/src/program/keep rather than ~/Pictures/keep I personally would think that's unexpected.

I think that we should probably keep track if the user changes the destination folder in case of the instance where the user actually wants the folder name "keep" in a directory. That seems like a bug/unintended behavior.

* Changes to dest_folder is now kept track for the case of newglob to maintain the invariant that if the user hasn't set dest folder, images should be moved to "./keep" by default unless specified otherwise

Davejkane · 2019-05-22T18:32:19Z

I like the behaviour of the keep folder as it is right now. As a user I'll probably make a keep folder on my desktop and jump around in different folders using new glob, and it's nice that the keep folder doesn't move with me.

I've just got a last little bit of QA to do before my blessing.

NickHackman · 2019-05-22T18:38:22Z

@Davejkane okay, I'll remove that

Davejkane · 2019-05-22T18:41:31Z

In the readme, it says that the command ? or help will toggle fullscreen. Please correct that.

I'm also a little surprised that toggling the help bar with h, and toggling it with the command mode aren't linked, so one can't undo the other. Also, the plain h command no longer works when there are no images loaded on initial load, even if I then newglob to get a bunch of pictures, h doesn't work.

Scrap that, hadn't seen you fixed it to question mark. 👍

Davejkane

You did good. Well done. That was a bit epic, but we've pulled through. If there are any bugs, we'll catch them in follow up tickets, but it got past my QA for now. Now that we have this functionality, I'm going to write a couple of tickets.

README.md

Davejkane · 2019-05-22T19:03:20Z

@gurgalex are you happy with this PR now? (please approve if so)

gurgalex · 2019-05-22T19:06:45Z

@Davejkane

Doing some testing to see if this is functioning.

I like the behaviour of the keep folder as it is right now. As a user I'll probably make a keep folder on my desktop and jump around in different folders using new glob, and it's nice that the keep folder doesn't move with me.

gurgalex · 2019-05-22T19:12:28Z

Seems fine to me. Even though I'm not seeing a commit that undid the changes for a moving ./keep directory.

Davejkane · 2019-05-22T19:13:12Z

I'll do the merge.

NickHackman added 7 commits May 16, 2019 22:18

Add vim like command mode

61a815b

* Colon now enters command mode * Added a modal approach kept track of in Program struct Current modes: normal, command, exit * Changed previous fn run -> fn run_normal_mode * Add basis for future command mode commands

Add command mode m/max, takes new max image length

3a9f4a4

Refractored code, fixed bug in :max

5ae1ab1

* Moved the event match and Mode to ui.rs * Fixed bug, where input for :max could be 0 and it would be interpreted as 0 instead of infinitely many

Add force render, added docs to command_mode_run

553e0a5

Refractor name change in ui.rs

9fa525a

NickHackman changed the title ~~Feat command mode, redefine glob~~ Feat command mode May 18, 2019

Add newglob Err msg if file doesn't exist, bug fix

1cbce37

Increased performance, inside command_mode function get_command by not rendering user input ever event, but only when it changes

gurgalex reviewed May 19, 2019

View reviewed changes

src/program/command_mode.rs Outdated Show resolved Hide resolved

Add Commands enum for command mode

f84d117

* Enum that lists out the possible commands and implements FromStr for each

This was referenced May 19, 2019

Debug draft for parsing multiple chracter command #57

Closed

Zoom and Pan #22

Closed

NickHackman added 2 commits May 19, 2019 12:01

Fix bug in newglob, add dep shellexpand and regex

5bbf788

* Newglob wouldn't properly accept files like /home/etc/test\ file\ name * Add dependency regex to be able to fix Newglob bug * Add dependency shellexpand to properly handle ~/$HOME/env vars * Minor refractors

Fixed Bugs, empty images didn't render bar command

0c266b6

* Render self.render_blank now passes command mode input to it to properly render, so the user can populate the images * Fixed bug, in newglob where it would crash on empty self.paths.images

NickHackman marked this pull request as ready for review May 19, 2019 16:56

NickHackman and others added 3 commits May 19, 2019 13:16

Merge branch 'development' into feat-redefine-glob

66af23a

Refractored code in response to clippy suggestions

7be5457

Merge remote-tracking branch 'refs/remotes/origin/feat-redefine-glob'…

0a12516

… into feat-redefine-glob

Davejkane requested changes May 19, 2019

View reviewed changes

gurgalex requested changes May 20, 2019

View reviewed changes

src/program/command_mode.rs Outdated Show resolved Hide resolved

Refractored code in command_mode.rs

d0908dc

Davejkane requested changes May 21, 2019

View reviewed changes

src/ui.rs Outdated Show resolved Hide resolved

src/program/command_mode.rs Outdated Show resolved Hide resolved

gurgalex added 2 commits May 21, 2019 11:45

Add lazy_static!. Make path_regex compile once

4799c82

Remove unneeded lifetimes due to cloning

ad6bbc3

Fixed bugs delete, move, and command mode df

a0db6e8

* delete and move would panic when deleting/moving from the end or on empty * command mode destfolder/df wouldn't remove escaped spaces

gurgalex reviewed May 22, 2019

View reviewed changes

src/program/render.rs Show resolved Hide resolved

gurgalex reviewed May 22, 2019

View reviewed changes

src/infobar.rs Outdated Show resolved Hide resolved

NickHackman added 2 commits May 22, 2019 11:17

Fixed bug, program returns an error

5e548a4

* Infobar was rendering empty Strings for the Exit mode

Command mode newglob, reset keep folder

2ea2226

* If destfolder is still 'keep' change it whenever newglob changes directories

gurgalex requested changes May 22, 2019

View reviewed changes

Paths::curent_dir -> base_dir, better track of df

42941ac

* Changes to dest_folder is now kept track for the case of newglob to maintain the invariant that if the user hasn't set dest folder, images should be moved to "./keep" by default unless specified otherwise

Davejkane approved these changes May 22, 2019

View reviewed changes

gurgalex reviewed May 22, 2019

View reviewed changes

README.md Show resolved Hide resolved

gurgalex approved these changes May 22, 2019

View reviewed changes

Davejkane merged commit f2d49ba into Davejkane:development May 22, 2019

NickHackman deleted the feat-redefine-glob branch May 22, 2019 20:56

This was referenced May 23, 2019

Normalize relative paths #61

Closed

Reverted changes made to destfolder in #58 #63

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat command mode #58

Feat command mode #58

NickHackman commented May 18, 2019 •

edited

Loading

NickHackman commented May 19, 2019 •

edited

Loading

gurgalex commented May 19, 2019 •

edited

Loading

NickHackman commented May 19, 2019

Davejkane left a comment

Davejkane May 19, 2019

gurgalex left a comment

NickHackman commented May 20, 2019

Davejkane left a comment

gurgalex commented May 21, 2019

gurgalex commented May 21, 2019

NickHackman commented May 21, 2019

Davejkane commented May 22, 2019 •

edited

Loading

NickHackman commented May 22, 2019 •

edited

Loading

Davejkane commented May 22, 2019

NickHackman commented May 22, 2019 •

edited

Loading

Davejkane commented May 22, 2019 •

edited

Loading

NickHackman commented May 22, 2019

gurgalex left a comment •

edited

Loading

gurgalex May 22, 2019

NickHackman May 22, 2019 •

edited

Loading

Davejkane commented May 22, 2019

NickHackman commented May 22, 2019

Davejkane commented May 22, 2019 •

edited

Loading

Davejkane left a comment

Davejkane commented May 22, 2019 •

edited

Loading

gurgalex commented May 22, 2019

gurgalex commented May 22, 2019

Davejkane commented May 22, 2019

Feat command mode #58

Feat command mode #58

Conversation

NickHackman commented May 18, 2019 • edited Loading

Features

Command Mode Features

Known bugs:

Future Fixes

NickHackman commented May 19, 2019 • edited Loading

Future Feature Issues

gurgalex commented May 19, 2019 • edited Loading

NickHackman commented May 19, 2019

Davejkane left a comment

Choose a reason for hiding this comment

Davejkane May 19, 2019

Choose a reason for hiding this comment

gurgalex left a comment

Choose a reason for hiding this comment

NickHackman commented May 20, 2019

Davejkane left a comment

Choose a reason for hiding this comment

gurgalex commented May 21, 2019

gurgalex commented May 21, 2019

NickHackman commented May 21, 2019

Davejkane commented May 22, 2019 • edited Loading

NickHackman commented May 22, 2019 • edited Loading

Davejkane commented May 22, 2019

NickHackman commented May 22, 2019 • edited Loading

Davejkane commented May 22, 2019 • edited Loading

NickHackman commented May 22, 2019

gurgalex left a comment • edited Loading

Choose a reason for hiding this comment

gurgalex May 22, 2019

Choose a reason for hiding this comment

NickHackman May 22, 2019 • edited Loading

Choose a reason for hiding this comment

Davejkane commented May 22, 2019

NickHackman commented May 22, 2019

Davejkane commented May 22, 2019 • edited Loading

Davejkane left a comment

Choose a reason for hiding this comment

Davejkane commented May 22, 2019 • edited Loading

gurgalex commented May 22, 2019

gurgalex commented May 22, 2019

Davejkane commented May 22, 2019

NickHackman commented May 18, 2019 •

edited

Loading

NickHackman commented May 19, 2019 •

edited

Loading

gurgalex commented May 19, 2019 •

edited

Loading

Davejkane commented May 22, 2019 •

edited

Loading

NickHackman commented May 22, 2019 •

edited

Loading

NickHackman commented May 22, 2019 •

edited

Loading

Davejkane commented May 22, 2019 •

edited

Loading

gurgalex left a comment •

edited

Loading

NickHackman May 22, 2019 •

edited

Loading

Davejkane commented May 22, 2019 •

edited

Loading

Davejkane commented May 22, 2019 •

edited

Loading