Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zsh-syntax-hightling + zsh-autocorrect breaks history-incremental-pattern-search-backward #387

Closed
docwhat opened this issue Nov 4, 2016 · 13 comments
Labels
Milestone

Comments

@docwhat
Copy link
Contributor

docwhat commented Nov 4, 2016

In my zsh configuration if I load both zsh-syntax-highlighting and zsh-autocorrect then history-incremental-pattern-search-backward is broken (I have it bound to ^R.

If I disable one or the other, then history-incremental-pattern-search-backward.

If I leave ^R bound to its default, history-incremental-search-backward then that works as well.

The load order of zsh-syntax-highlighting and zsh-autocorrect doesn't seem to make any difference.

I created a minimal test case for you: https://gist.github.com/docwhat/0cd192c066a5f9da69244f1631c5d481

My files:

@danielshahaf
Copy link
Member

In my zsh configuration if I load both zsh-syntax-highlighting and zsh-autocorrect then history-incremental-pattern-search-backward is broken (I have it bound to ^R.

If I disable one or the other, then history-incremental-pattern-search-backward.

If I leave ^R bound to its default, history-incremental-search-backward then that works as well.

The load order of zsh-syntax-highlighting and zsh-autocorrect doesn't seem to make any difference.

I created a minimal test case for you: https://gist.github.com/docwhat/0cd192c066a5f9da69244f1631c5d481

Thanks for the minimal example. Haven't looked yet, but two quick comments:

  1. This will probably fixed by redrawhook issues #245 (the fix for which is already written, and is only waiting for zsh 5.3 to be released).
  2. I wonder if the patch at Segfault after sourcing zshrc again - conflict with zsh-autosuggestions even when sourced in right order. zsh-autosuggestions#126 (comment) fixes this issue as well. (Need to run make after applying it to zsh-autosuggestions.)

@docwhat
Copy link
Contributor Author

docwhat commented Nov 5, 2016

I'm looking forward to 5.3, then. :-)

The mini-patch you provided did not fix my test-case (posted above) nor my normal shell environment. :-(

@docwhat
Copy link
Contributor Author

docwhat commented Nov 5, 2016

More info (hopefully useful)...

If I tell zsh-autosuggest to ignore history-incremental-pattern-search-backward and history-incremental-pattern-search-forward then it is still broken, which suggests that something else zsh-autosuggest is doing is messing with the rebinding function.

How I ignored those widgets:

source ~/zsh-syntax-highlighting/zsh-syntax-highlighting.zsh
source ~/zsh-autosuggestions/zsh-autosuggestions.zsh
typeset -ar ZSH_AUTOSUGGEST_IGNORE_WIDGETS=(
  "${ZSH_AUTOSUGGEST_IGNORE_WIDGETS[@]}"
  history-incremental-pattern-search-backward
  history-incremental-pattern-search-forward
)

However, if I tell zsh-syntax-highlighting to not re-bind those widgets, then it works fine with zsh-autosuggest's re-binding. I had to modify the source for zsh-syntax-highlighting to prevent it from re-binding those widgets because I couldn't find a similar IGNORE_WIDGETS configuration knob:

diff --git a/zsh-syntax-highlighting.zsh b/zsh-syntax-highlighting.zsh
index ec01306..962fdbc 100644
--- a/zsh-syntax-highlighting.zsh
+++ b/zsh-syntax-highlighting.zsh
@@ -275,6 +275,9 @@ _zsh_highlight_bind_widgets()
   # This is needed because we need to disable highlighting in that case.
   widgets_to_bind+=(zle-isearch-update)

+  widgets_to_bind=(${foo#history-incremental-pattern-search-backward})
+  widgets_to_bind=(${foo#history-incremental-pattern-search-forward})
+
   local cur_widget
   for cur_widget in $widgets_to_bind; do
     case $widgets[$cur_widget] in

@danielshahaf
Copy link
Member

Thanks for testing the patch and for the very detailed followup.

Are you sure z-asug is involved here? If I do just:

% bindkey '^R' history-incremental-pattern-search-backward 
% echo foo
% echo bar
% <press Ctrl+R>echo<press Ctrl+R>

then the result is different if I source z-sy-h after the bindkey,
compared to if I don't source z-sy-h at all.

That's with zsh master and z-sy-h master (which still use the
5.2-and-earlier codepath), in zsh -f.

@danielshahaf
Copy link
Member

... and #245 does not solve this issue.

@danielshahaf danielshahaf added this to the 0.6.0 milestone Nov 5, 2016
@danielshahaf
Copy link
Member

Milestone 0.6.0 — I'd like to understand the root cause before 0.6.0.

@docwhat
Copy link
Contributor Author

docwhat commented Nov 5, 2016

After much digging through code and reading up on what zle and widgets are, etc...

This fixes the problem:

source ~/zsh-syntax-highlighting/zsh-syntax-highlighting.zsh
source ~/zsh-autosuggestions/zsh-autosuggestions.zsh

typeset -ar ZSH_AUTOSUGGEST_IGNORE_WIDGETS=(
  "${ZSH_AUTOSUGGEST_IGNORE_WIDGETS[@]}"
  zle-isearch-update
)

So it looks like whatever zsh-autosuggest is doing to zle-isearch-update is breaking backwards search, but only if I enabled zsh-syntax-highlighting too.

docwhat added a commit to docwhat/dotfiles that referenced this issue Nov 5, 2016
Figured out what was causing the problem(s).

I reported it at zsh-users/zsh-syntax-highlighting#387

I submitted a pull request at zsh-users/zsh-autosuggestions#206
docwhat added a commit to docwhat/dotfiles that referenced this issue Nov 5, 2016
Figured out what was causing the problem(s).

I reported it at zsh-users/zsh-syntax-highlighting#387

I submitted a pull request at zsh-users/zsh-autosuggestions#206
@danielshahaf
Copy link
Member

danielshahaf commented Nov 5, 2016 via email

@docwhat
Copy link
Contributor Author

docwhat commented Nov 5, 2016

Could you try whether the problem reproduces with zle-isearch-update() { zle .zle-isearch-update -- "$@" ; false } and zsh-autosuggestions, but without z-sy-h?

I took it for a spin in my test-gist and everything worked correctly. The problem didn't appear.

# Lines from .zshrc:
zle-isearch-update() { zle .zle-isearch-update  -- "$@" ; false }
source ~/zsh-autosuggestions/zsh-autosuggestions.zsh

zle -la | grep isearch returned nothing, so I'm not sure the test did what you wanted.

@danielshahaf
Copy link
Member

danielshahaf commented Nov 5, 2016 via email

@docwhat
Copy link
Contributor Author

docwhat commented Nov 5, 2016

Yup; this causes the exact same issue in zsh-autosuggest:

# Lines from .zshrc
zle-isearch-update() {}; zle -N zle-isearch-update
source ~/zsh-autosuggestions/zsh-autosuggestions.zsh

@danielshahaf
Copy link
Member

danielshahaf commented Nov 5, 2016 via email

@docwhat
Copy link
Contributor Author

docwhat commented Nov 6, 2016

Done!

@docwhat docwhat closed this as completed Nov 6, 2016
ericfreese pushed a commit to zsh-users/zsh-autosuggestions that referenced this issue Feb 18, 2017
It seems like all the zle-* methods are special and shouldn't be
monkeyed with.

Specifically `zle-isearch-update` and friends. Binding that widget
caused `history-incremental-pattern-search` to stop working.

Fixes zsh-users/zsh-syntax-highlighting#387
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants