Feature: Add vi-mode plugin

[alxbl]

I noticed that this issue (#165) has been open for a while and decided to try implementing it after reading the gitter.

There are a few issues right now, but I'm mostly looking for feedback before putting more effort into it.

Namely:

• Customization options for colors
• Customization for state text
• State doesn't re-display after executing command (maybe precmd hook?)
• Should the plugin automatically activate bindkey -v?

[jedahan]

Not sure what the implications are, but this at least makes it behave kinda like the other plugins (where it only renders if geometry_vi is set).

[alxbl]

Thanks for the feedback! I'm currently swamped but I should have time to update the PR this weekend. I like the idea of checking/enabling if the render function is called.

[alxbl]

I've managed to get the mode to always display even after a new line. It requires to bind the following zle hooks:

zle -N zle-keymap-select geometry-vi-draw
zle -N zle-line-pre-redraw geometry-vi-draw
zle -N zle-line-init geometry-vi-draw
zle -N zle-line-finish geometry-vi-draw

It will definitely be necessary to detect existing hooks and chain them otherwise this will almost certainly break some people's setup.

[jedahan]

yeah I wish zle supported push/pop instead of having everyone manually deal with the chain

[alxbl]

I've added the logic to properly handle existing ZLE widgets and dynamic prompt updates. I simplified the update logic by using the render function to put the current state on prompt render and then use replacements to preserve widget order. As an added bonus, GEOMETRY_INFO will also support the plugin.

I also prefixed most functions with geometry::. I'm not sure if this is desirable, but I saw other functions doing it.

[alxbl]

I was trying to find a clever way to pass the prompt as an indirect variable through ${(P)varname} but couldn't find a way to assign back, so for now this is the cleanest way to do it.

[alxbl]

Thanks to the changes in #273, this will only run once if geometry_vi is included in any of the rendering prompts.

[alxbl]

Okay, I know why it's not working, and it's a bit of a bummer.

The autoload works very well for lazy loading of functions, but unfortunately because geometry makes use of command substitution ($() ) for rendering prompt functions, this spawns a subshell where the autoload is resolved on each call. The end result is that the actual shell never initializes the functionsn. This is by design: subshells can't affect their parent shell. It also has the annoying effect that lazy loading is equivalent to loading the functions on each render pass and makes functions that provide ZLE widgets pretty difficult to code.

That's why the ZLE widgets were never called when switching vi mode. On my other PC it worked because I hadn't pulled the lazy loading code yet.

I'm going to try and find a way around this, but so far it's not looking good.

PS: Are inter-function dependencies really something that we want to handle? So far there doesn't seem to be any function that depends on another function for its output and if that were ever to become the case, I feel like it would undermine the modularity and self-contained nature of geometry functions. Let me know what you think.

[jedahan]

Handling inter-function dependencies is out of scope for geometry. I'd rather have code duplicated than shared, functions should be self-contained.

As far as I understand it, you are saying that autoload is actually re-sourcing for every subshell, which is worse for performance generally, and not helping us out. Is that correct?

Is there a way for geometry to not use a sub-shell? Might be a large refactoring but useful in reducing potential bugs like this?

[alxbl]

As far as I understand it, you are saying that autoload is actually re-sourcing for every subshell, which is worse for performance generally, and not helping us out. Is that correct?

Yes, that's my understanding as well. I don't know enough about ZSH internals to guarantee that there isn't some kind of caching mechanism, but I can definitely see my initialization code being called on every render tick.

Handling inter-function dependencies is out of scope for geometry. I'd rather have code duplicated than shared, functions should be self-contained.

In that case it should be fairly safe to revert to the "lazy loading" approach I had put in my #273 PR, where we only source the functions that are part of a prompt array. If you feel like this is still not a good solution, then I would propose reverting back to sourcing all functions until we have time to figure out a better approach for lazy loading.

Is there a way for geometry to not use a sub-shell? Might be a large refactoring but useful in reducing potential bugs like this?

As far as I'm aware, only exec and eval run in the context of the current shell, but even with eval I wasn't able to get the functions resolved in the top-level shell. I think this is because the invocation is happening inside a shell function (geometry::wrap). Moving everything to be done in the top-level shell would also break the asynchronous RPROMPT, so I don't think it's possible to refactor this in a clean way.

I'll wait for your input before moving forward with anything.

[alxbl]

Alright, at this point it should be working. I've added a check to avoid hooking the ZLE widgets if geometry_vi is not in either prompt (INFO is not checked because it doesn't require any widget)

If all is good, we can probably squash merge this as it has quite a bit of noise in the commits now.

Cheers,
Alex

[jedahan]

Hmm, I tested latest but its still not picking up when I go from insert to command mode by pressing escape.

[jedahan]

Ok I think I figured it out, the gating does not work with dynamically adding and removing geometry_vi. If we move it to the zle bound functions then I think it works in all cases.

[jedahan]

The only other thing I can think of would be to choose some nice symbol(s) that represent insert and normal modes, in line with most other plugins, but thats a minor nit.

[alxbl]

If that okay with you, I'll add ${GEOMETRY[VI]} which will be set when the function has been initialized. This will make it possible to put the initialization logic in the render function and avoid running it on each render.

I'm open to symbol suggestions. I suppose we could pick a geometric shape and use a filled/outline version of it to keep in line with the theme? Or we could an edit/window move icon combo, there isn't anything really vi specific though.

[jedahan]

I think setting in GEOMETRY is fine for a function we ship, especially because it touches zle. I'm gonna be a bit out of touch this week as I'm travelling for a conference, just a heads up.

[alxbl]

After a lot of fiddling around, I found out that this lazy initialization in the render callback will not work for the same reason as the other attempts... the callback is being run in a subshell and cannot set any variable in the top level shell session. This means that binding the widgets must be done when the function is sourced. I'm more and more convinced that without a major refactor, the only way we could have conditional initialization is if we only sourced the functions that are part of the prompt arrays.

In other words, functions cannot interact with the top level shell and alter its configuration in the current architecture. This is actually good depending on your point of view, but will definitely make this tricky to develop. We can discuss this further whenever you are available.

I understand the need to have dynamic prompt changes by appending to the prompt variables, but the best scenario I can think of if we move forward with the lazy sourcing implementation would be that the function must be in scope before being dynamically added. This means that if a user wants to do dynamic functions, they have to ensure that the functions they want are sourced beforehand, manually or otherwise.

Cheers,
Alex