"Restore standard widgets to fix segfault when re-sourcing autosuggestions.zsh" causes errors at startup with history-substring-search #88

Closed
ronjouch opened this Issue Jan 20, 2016 · 15 comments

3 participants

@ronjouch

Following #85 / 8777836 , I get these errors at startup:

autosuggest-restore-widgets:zle:3: no such widget `.history-substring-search-up'
autosuggest-restore-widgets:zle:3: no such widget `.history-substring-search-down'

I use:

zmodload zsh/terminfo
bindkey "$terminfo[kcuu1]" history-substring-search-up
bindkey "$terminfo[kcud1]" history-substring-search-down

Reverting to 7a00bfa fixes the problem.

Is this:

  • A regression?
  • A bug in history-substring-search?
  • Some misconfiguration of mine?

Feel free to ask for more info.

@z0rc z0rc referenced this issue in zsh-users/zsh-history-substring-search Jan 21, 2016
Closed

up down functions stopped working after update #50

@hcgraf

I can confirm this problem with a very similar configuration, but without oh-my-zsh.

@hcgraf

Ok, I did some digging around, and came up with a solution, but it was more trial-and-error than me-really-understanding-what's-going-on ;)
So therefore I don't want to go the fork-and-PR way, yet…

diff --git a/autosuggestions.zsh b/autosuggestions.zsh
index 7a9112f..4106876 100644
--- a/autosuggestions.zsh
+++ b/autosuggestions.zsh
@@ -291,4 +291,10 @@ zle -N autosuggest-tab
 zle -N autosuggest-suspend
 zle -N autosuggest-accept-suggestion

+# save widgets
+for widget in $ZLE_AUTOSUGGEST_ALL_WIDGETS; do
+       [[ -z $widgets[$widget] || ! -z $widgets[.$widget] ]] && continue
+       eval "zle -A $widget .$widget"
+done
+
 autosuggest-restore-widgets

So, it seems that the .$widget (dot-prefixed) variant exists for every widget except for the history-substring-search-xxx widgets... However, I didn't find any documentation about the dot-prefix.
Can/should we just create those? Should we stick to the old autosuggest-*-orig naming style (maybe only for the widgets where no dot-variant exists)?

Any thoughts? Maybe @ericfreese would be able to answer some of my questions…

@hcgraf

Now I found something about the dot-prefix:

Notice the widgets starting with a dot? These always refer to the zsh built-in version of a widget. Widgets can we overridden with custom functionality and it is sometimes necessary to unambiguously refer to the built-in one.

Source: http://sgeb.io/articles/zsh-zle-closer-look-custom-widgets/

IMHO, the dot-variant is not what this autosuggestion plugin should use, because it completely prevents other plugins from overwriting/extending certain widgets…

@ericfreese
zsh-users member

It sounds like a regression.

I don't use oh-my-zsh or the history-substring-search-* widgets so I didn't see these errors when testing my fix.

The dot prefixed widget aliases are described here:

Each built-in widget has two names: its normal canonical name, and the same name preceded by a ‘.’. The ‘.’ name is special: it can’t be rebound to a different widget. This makes the widget available even when its usual name has been redefined.

The history-substring-search-* widgets are not built-in so they don't have a dotted alias.

IMHO, the dot-variant is not what this autosuggestion plugin should use, because it completely prevents other plugins from overwriting/extending certain widgets…

I was concerned about this, but the problem with using the non-dotted alias is that you can't rely on it to do what you expect. If the code calls zle end-of-line, it expects the cursor to go to the end of the line, but it may not behave that way or it may trigger some extra unwanted behavior if some other plugin has redefined it.

I think the right thing to do is use the dotted alias if available, otherwise use a prefix or suffix like -orig. I'd vote for a prefix with "autosuggest" in it to minimize chance of a collision.

In the mean time, I'm working on a thorough cleanup in this pull request. I'll add support for user defined widgets like history-substring-search-xxx to the todo list. Please check it out if you have time and let me know of any issues with your configuration.

@ericfreese
zsh-users member

I think the right thing to do is use the dotted alias if available, otherwise use a prefix or suffix like -orig. I'd vote for a prefix with "autosuggest" in it to minimize chance of a collision.

Thinking more about this, I'm thinking you may be right, that we shouldn't use dotted aliases. Does anyone know of an example plugin that would be broken by using the dotted aliases that we can use to test with?

@hcgraf
@ericfreese
zsh-users member

So what about using my diff from above?

Can you create a pull request? I'll take a look at it next time I'm at a computer (currently traveling). Hopefully early next week.

Would it be possible to find out about redefined widgets, and issue a warning at least?

That's a great idea. Not sure if you can detect redefined widgets though. I'll look at this next week too.

@hcgraf hcgraf pushed a commit to hcgraf/zsh-autosuggestions that referenced this issue Jan 25, 2016
Hagen Graf workaround for issue #88, probably not the final solution, see discus…
…sion
a870925
@hcgraf

Sorry for the late answer, I was also out for the weekend…
I opened a PR for revision.
However if we agree on not-using the dot-widgets at all, the PR can just be ignored.

@ericfreese
zsh-users member

@hcgraf There's a rewrite underway in PR #91. It's available on branch v0.1.x. Please try using that branch, and let me know if it solves these problems.

@ronjouch

@ericfreese thanks for your work :) . I just tried branch v0.1.x, and it has two problems for me:

  • It causes my cursor to appear one line below what I expect. Suppose I press ENTER. Instead of
~ <- cursor here

I get

~
<- cursor here
  • Also, typing anything causes error No such widget 'autosuggest-start' to show up.

Both problems screencasted here (WebM, 50kB).

@ericfreese
zsh-users member

Thanks for the feedback!

Please check out the readme in the v0.1.x branch.

zle autosuggest-start has become autosuggest_start. So the following should be somewhere in your zshrc:

# Enable autosuggestions
zle-line-init() {
    autosuggest_start
}

zle -N zle-line-init

The config variables have also changed:

AUTOSUGGESTION_HIGHLIGHT_COLOR has become ZSH_AUTOSUGGEST_HIGHLIGHT_COLOR, and the other options have been removed.

@ronjouch

@ericfreese thanks, didn't see these changes. Hope you can find a way to communicate these when the PR is merged and users update to it...

But now that I updated my .zshrc with what you mention above, now I get this at each character I type:

  _zsh_autosuggest_get_suggestion:2: bad pattern: history_items=(ls

What am I doing wrong? here's how I start the plugin in .zshrc:

source $HOME/.zsh-autosuggestions/autosuggestions.zsh
unset COMPLETION_WAITING_DOTS # https://github.com/tarruda/zsh-autosuggestions#known-issues
ZSH_AUTOSUGGEST_HIGHLIGHT_COLOR='fg=3'
zle-line-init() {
  autosuggest_start
}
zle -N zle-line-init
@ericfreese
zsh-users member

Huh... haven't seen that before. I'll look into what could be causing it.

The .zshrc snippet looks good to me. Are you sourcing any other files? If so, you could try removing them all and seeing if the problem is solved, then adding them back one-by-one to isolate which one is causing the issue.

This line is the culprit: https://github.com/tarruda/zsh-autosuggestions/blob/v0.1.x/lib/get_suggestion.zsh#L3

What happens if you run echo ${history[(R)*]} in the command line?

@ericfreese
zsh-users member

Let's carry this conversation over to issue #92.

@ericfreese
zsh-users member

Going to go ahead and close this.

@ericfreese ericfreese closed this Jan 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment