Skip to content

Conversation

gotgenes
Copy link
Contributor

This fixes an issue where zsh-vi-mode ignores the values for commands hooks arrays set before sourcing the plugin. For example, the documentation shows the following:

# Append a command directly
zvm_after_init_commands+=('[ -f ~/.fzf.zsh ] && source ~/.fzf.zsh')

but zvm_after_init_commands was always getting reset to the empty array upon sourcing this plugin.

@gotgenes
Copy link
Contributor Author

@jeffreytse Is there anything I can do to help move this PR forward?

Copy link
Owner

@jeffreytse jeffreytse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gotgenes

Thanks for your contribution. I am trying to understand what scenario this change can be used to, one of the scenario I can find is we need to set these commands when we change initial mode to ZVM_INIT_MODE=sourcing, as maybe some internal commands will be added to these aspects in the future and keep the order fixed. : )

Thanks & Regards

@jeffreytse jeffreytse self-assigned this Jun 22, 2022
@jeffreytse jeffreytse added the enhancement New feature or request label Jun 22, 2022
@gotgenes
Copy link
Contributor Author

Yes, I can give you my scenario. Here Is how I'm setting up zsh-vi-mode..

I have two sets of keybindings I'd like to register. I define the functions to set the keybindings before sourcing zsh-vi-mode. Because sourcing zsh-vi-mode resets the hooks arrays to empty arrays, I can't directly configure these arrays as described in the docs.

Let's focus just on one of the two sets of keybindings so it is more clear what I mean.

Here is a set of insert-mode keybindings I want to register as a command to run after zsh-vi-mode init:

function set_insert_keybindings() {
    zvm_bindkey viins "^[[H" beginning-of-line
    zvm_bindkey viins "^[[F" end-of-line
    zvm_bindkey viins "^[[Z" reverse-menu-complete
    zvm_bindkey viins "^[[A" history-substring-search-up
    zvm_bindkey viins "^[[B" history-substring-search-down
}

Here is how the documentation describes registering the command:

zvm_after_init_commands+=(set_insert_keybindings)

Then I source zsh-vi-mode with

zinit light jeffreytse/zsh-vi-mode

Putting it all together, it would look like this:

function set_insert_keybindings() {
    zvm_bindkey viins "^[[H" beginning-of-line
    zvm_bindkey viins "^[[F" end-of-line
    zvm_bindkey viins "^[[Z" reverse-menu-complete
    zvm_bindkey viins "^[[A" history-substring-search-up
    zvm_bindkey viins "^[[B" history-substring-search-down
}
zvm_after_init_commands+=(set_insert_keybindings)
zinit light jeffreytse/zsh-vi-mode

When we source zsh-vi-mode, zvm_after_init_commands gets redefined as an empty array, and so my keybindings fail to get set.

You can see that right now I have to hack around this shortcoming by registering the hooks within another hook:

function zvm_before_init() {
    # zsh-vi-mode has set zvm_after_init_commands to an empty array at this point
    # but it has not yet invoked it, so we can modify it
    zvm_after_init_commands+=(set_insert_keybindings)
}

I'd rather set the hooks directly, not within another hook.

This PR fixes that issue by only defining the hooks arrays if they're not yet defined. They will no longer get overwritten.

@gotgenes
Copy link
Contributor Author

I pushed up some changes to improve clarity. I hope they help.

Copy link
Owner

@jeffreytse jeffreytse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gotgenes

I can understand what you mean, I still have a question, why you don't want to put them after loading zsh-vi-mode.

zinit light jeffreytse/zsh-vi-mode

function set_insert_keybindings() {
    zvm_bindkey viins "^[[H" beginning-of-line
    zvm_bindkey viins "^[[F" end-of-line
    zvm_bindkey viins "^[[Z" reverse-menu-complete
    zvm_bindkey viins "^[[A" history-substring-search-up
    zvm_bindkey viins "^[[B" history-substring-search-down
}
zvm_after_init_commands+=(set_insert_keybindings)

Thanks & Regards

@gotgenes
Copy link
Contributor Author

If the plugin is already loaded, I would expect changing zvm_after_init_commands has no effect. I believe we should have already finished calling the commands in that array. What have I misunderstood?

Copy link
Owner

@jeffreytse jeffreytse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gotgenes

Currently, I am going to merge your PR, as the indent is 2 spaces, could you format it by the standard?

Thanks & Regards

@gotgenes gotgenes force-pushed the check-for-existing-commands branch from fbca865 to 176dbfa Compare July 12, 2022 03:19
@gotgenes
Copy link
Contributor Author

@jeffreytse My apologies. I have updated the code to fit the 2-space indentation.

Copy link
Owner

@jeffreytse jeffreytse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great, thanks for your contribution, and keep going on! : )

@jeffreytse jeffreytse merged commit b0fd403 into jeffreytse:master Jul 12, 2022
gotgenes added a commit to gotgenes/dotfiles that referenced this pull request Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants