Feat command mode

[NickHackman]

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

[NickHackman]

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

[gurgalex]

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.

[NickHackman]

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

[NickHackman]

@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 :smiley:

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]

As far as I'm concerned, there's just a couple of things left. And that's the "colon is not necessarily shift+semi-colon" issue. And then there were some actions that appeared to be unprocessed and so I'm not sure if they would apply to the dot operator, but it could be that I've misunderstood a change there.

[gurgalex]

@Davejkane @NickHackman Layout independent : could be resolved with a mixture of keycodes and scancodes.

Use keycodes for detecting when the OS generates a LSHIFTMOD or RSHIFTMOD. Match the scancode for a ; on a QWERTY keyboard.

KeyDown event struct

From sdl2 docs

SDL_Scancode values are used to represent the physical location of a keyboard key on the keyboard.

SDL_Keycode values are mapped to the current layout of the keyboard and correlate to an SDL_Scancode.

Which one to use is left to the application: scancodes are suited in situations where controls are layout-dependent (eg. the "WASD" keys as left-handed arrow keys), whereas keycodes are better suited to situations where controls are character-dependent (eg. the "I" key for Inventory).

[NickHackman]

@Davejkane Little confused what you're referring to, what operations were being unprocessed?

@gurgalex Thanks

Thanks so much for the input it has helped monumentally, and sorry that this has taken so long :heart:

[Davejkane]

I'm not sure I'm with you there @gurgalex. The problem isn't that semi-colon is sometimes in a different place, the problem is that colon is not always shift+semi-colon. For example, on a French keyboard (AZERTY) it is an unshifted key, whereas on a German keyboard (QWERTZ), it is shift+comma. So I'm not sure what you are suggesting would work.

[NickHackman]

Yeah, going to use scancode which uses the virtual mapping, going to convert that then check to see if it matches afterwards. I think that's keyboard independent.

[Davejkane]

@NickHackman I see that I've misunderstood something there with the unprocessed actions. So just ignore that comment. Just the colon key to handle and then I'm happy.

[gurgalex]

@Davejkane

I'm not sure I'm with you there @gurgalex. The problem isn't that semi-colon is sometimes in a different place, the problem is that colon is not always shift+semi-colon. For example, on a French keyboard (AZERTY) it is an unshifted key, whereas on a German keyboard (QWERTZ), it is shift+comma. So I'm not sure what you are suggesting would work.

Right, so scancodes return the QWERTY key which corresponds to the physical location of the key pressed regardless of OS keyboard mapping. So, a keypress on an AZERTY or QWERTZ in the same physical location will return the same scancode. (Assuming the hardware mapping isn't different)

[Davejkane]

Actually, one last thing! If the bar is hidden because it is toggled off with the t key, then entering command mode should force it to be rendered.

[Davejkane]

Also, something has been broken with move and delete image. The length of the images vector isn't updated, so the program crashes when moving to the last image. I'm going to have to do a bit more QA on this to see if anything else is broken.

[gurgalex]

@NickHackman

the following results in a panic.

Enter command mode (:)

type ng (no space)

Press Enter

thread 'main' panicked at 'byte index 3 is out of bounds of `ng`', src/libcore/str/mod.rs:2010:9

[Davejkane]

@gurgalex, I'm still not sure I understand what you mean. Are you suggesting that a German user types in Ö to enter command mode? Because that would be what they would be doing to use the key that is physically in place where an American keyboard has semi-colon.

[gurgalex]

Are you suggesting that a German user types in Ö to enter command mode? Because that would be what they would be doing to use the key that is physically in place where an American keyboard has semi-colon.

@Davejkane Yes.

[Davejkane]

Also, @NickHackman no stress and no rush at all. It's totally fine and normal that things break on a big change like this.

[Davejkane]

@gurgalex I think a German should type colon like everyone else to get into command mode, otherwise we'll be making lots of user manuals.

[NickHackman]

@gurgalex That's not reproducible on my end. Maybe pull again, that was an issue created by me messing up the merging, I fixed that in the latest push.

[gurgalex]

@gurgalex I think a German should type colon like everyone else to get into command mode, otherwise we'll be making lots of user manuals.

@NickHackman Okay, in that case. Matching on TextInput {text: ":", ..} should work instead.

https://docs.rs/sdl2/0.32.2/sdl2/event/enum.Event.html#variant.TextInput

[gurgalex]

@NickHackman You're right. I wasn't using the latest changes.

@gurgalex That's not reproducible on my end. Maybe pull again, that was an issue created by me messing up the merging, I fixed that in the latest push.

[Davejkane]

So sorry @NickHackman, but the command for changing the destination folder isn't handling tilde properly. It creates a folder in the current directory called ~ which is difficult to cd into 😂

[NickHackman]

@Davejkane oh dang, my bad, fixing that asap

[gurgalex]

@NickHackman

Command mode does not trigger if the infobar is set to hidden.

We may need to have two bars (an infobar and a command bar).

The command bar renders over the infobar (or just the image if hidden).

[gurgalex]

Nevermind, it does trigger when hidden. Just was confused about what I had inputted (errors were not shown in input due to hidden bar)

Suggestion about a command bar still may be useful for showing input and errors.

[NickHackman]

@gurgalex now it for sure is visible, just in case.

[gurgalex]

@Davejkane @NickHackman Do you think equivalent command mode operations should be repeatable with . (such as :help) -> Action::Help

[gurgalex]

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

[gurgalex]

We could address this later though.

[NickHackman]

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

[Davejkane]

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

[NickHackman]

@Davejkane I don't think I wrote that bug, but fixed :+1:

edit: Yeah, sorry that is certainly my fault

[Davejkane]

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]

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

[Davejkane]

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.

[NickHackman]

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 :smile:

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

Probably going to do #53 after this

[Davejkane]

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]

@Davejkane okay, I'll remove that

[Davejkane]

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]

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

[gurgalex]

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

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

[Davejkane]

I'll do the merge.