zsh-history-substring-search icon indicating copy to clipboard operation
zsh-history-substring-search copied to clipboard

Stop depending on zsh-syntax-highlighting implementation details

Open danielshahaf opened this issue 9 years ago • 14 comments

Currently, this project defines the functions _zsh_highlight and _zsh_highlight_bind_widgets if they are not already available.

Those functions are private functions of zsh-syntax-highlighting, in its namespace, and z-sy-h reserves the right to change the semantics of those functions incompatibly without notice.

As a matter of good engineering, please stop depending on implementation details of z-sy-h.

https://github.com/zsh-users/zsh-history-substring-search/blob/557d25e940d3fc07b9b24ec7f549561d770f96cb/zsh-history-substring-search.zsh#L89-L176

danielshahaf avatar Jul 04 '16 19:07 danielshahaf

I should add: I'm the maintainer of z-sy-h, and I'd be happy to make changes on z-sy-h's side to make parts of it more reusable by other projects. Just open an issue and say what you want made reusable ☺.

danielshahaf avatar Jul 04 '16 19:07 danielshahaf

It's been a while but AFAIK that code was embedded here to make both plugins play well together. :thought_balloon:

Could you expose a public API function that rebinds all ZLE widgets to invoke a user-specified function? :gift: That way we can drop both _zsh_highlights() and _zsh_highlight_bind_widgets() from this codebase.

sunaku avatar Jul 16 '16 16:07 sunaku

_zsh_highlight_bind_widgets is going away: zsh 5.3 will have two new features that z-sy-h will use in lieu of _zsh_highlight_bind_widgets. (With latest zsh master: autoload -U add-zle-hook-widget && add-zle-hook-widget zle-line-pre-redraw foo where foo is a zle widget name.)

Encapsulating the "Use _bind_widgets under zsh<5.3 and zle-line-pre-redraw under zsh≥5.3" is an interesting question. z-sy-h will have such code written to it (tracked in https://github.com/zsh-users/zsh-syntax-highlighting/issues/245). Once it's written (the dust hasn't settled on the upstream API details), we can think of how to make it reusable by any plugin. (/cc @psprint as this question is comparable to his https://github.com/psprint/allcompletions.)

However, I don't think such a library function being made available is a prerequisite for addressing my request, which was simply for z-h-s-s to stop defining functions called _zsh_highlight_*. I'm simply asking for the cached copy of the function embedded in z-h-s-s.zsh to be renamed something other than _zsh_highlight_*.

I realize that wouldn't solve the technical debt that duplicating the function definition introduced. However, it would be an improvement and a step in the right direction (it would prevent a class of bugs).

To be fair, _zsh_highlight_bind_widgets is itself believed to have a namespacing bug, https://github.com/zsh-users/zsh-syntax-highlighting/issues/305, which boils down to "If you duplicate the function definition, change the orig-* prefix to something else".

Cheers!

danielshahaf avatar Jul 17 '16 11:07 danielshahaf

Hi @danielshahaf, Thanks a lot for all your work on z-sy-h! Unfortunately I do not understand the details of the issues around zle-line-pre-redraw. However, would a simple perl -p -i -e "s/orig-/zssh-/" ./zsh-history-substring-search.zsh be enough to satisfy the above request?

guidovansteen avatar Aug 06 '16 08:08 guidovansteen

Good morning, @guidovansteen.

However, would a simple perl -p -i -e "s/orig-/zssh-/" ./zsh-history-substring-search.zsh be enough to satisfy the above request?

It would solve one problem, but leave one other problem and one other edge-case problem unresolved.

The problem it would solve is coexistence of z-h-s-s and z-sy-h under zsh≤5.2.

The problem it leaves unresolved is the fact that z-h-s-s defines and invokes functions in the _zsh_highlight_* namespace, which are considered implementation details of z-sy-h.

The edge-case problem is a known bug in _bind_widgets: so long as it uses a static naming pattern, be it orig-$foo or zssh-$foo or $HOST-$foo, it is vulnerable to infinite loops after source z-h-s-s; source z-sy-h; source z-h-s-s [which is common when doing source .zshrc]: see https://github.com/zsh-users/zsh-syntax-highlighting/issues/305#issuecomment-217159398. This bug only applied to _bind_widgets, it does not apply to pre-redraw.

So, in short: that perl pie would effect an improvement, but it wouldn't solve the issue I opened this ticket about.

Unfortunately I do not understand the details of the issues around zle-line-pre-redraw.

Short summary: in zsh 5.3, one will be able to do f() {}; zle -N f; autoload add-zle-hook-widget; add-zle-hook-widget zle-line-pre-redraw f to get f to be called "after every keypress". In 5.2 and earlier, _zsh_highlight_bind_widgets simulates the same effect by binding f to every widget.

Cheers,

danielshahaf avatar Aug 10 '16 16:08 danielshahaf

Thanks @danielshahaf. Would it help if we use something like "s/orig-/zssh-$RANDOM/"?

guidovansteen avatar Aug 11 '16 05:08 guidovansteen

I assume that'd solve the widgets issue but not the functions issue.

danielshahaf avatar Aug 11 '16 06:08 danielshahaf

People who want to use this plugin but don't want to enable zsh-syntax-highlighting can install this fork https://github.com/changyuheng/zsh-syntax-highlighting instead. This fork has all the functions of zsh-syntax-highlighting but not enabling it.

The reason I don't want to enable zsh-syntax-highlighting is that it's performance is too bad. If you paste multiple lines of commands at once, your terminal would get stuck for a while which can be longer than 10 seconds.

mrjohannchang avatar Apr 12 '17 17:04 mrjohannchang

This zsh-history-substring-search plugin does not require zsh-syntax-highlighting! :policeman: It stands alone. :1st_place_medal: If zsh-syntax-highlighting is present, then this plugin tries to play nicely with it. That's all, nothing more.

sunaku avatar Apr 12 '17 18:04 sunaku

@sunaku Maybe I was wrong but I got this error when I disabled zsh-syntax-highlighting. _zsh_highlight- function definition file not found

mrjohannchang avatar Apr 12 '17 18:04 mrjohannchang

@changyuheng That might a side-effect of something else in your Zsh configuration or environment. At minimum, this is what should happen when you use this plugin (all by itself, inside a bare -f Zsh session): asciicast

sunaku avatar Apr 12 '17 18:04 sunaku

@sunaku It seems like since I had installed zsh-syntax-highlighting before, I have to remove .zcompdump so that zsh-history-substring-search can get loaded properly.

mrjohannchang avatar Apr 12 '17 21:04 mrjohannchang

The reason I don't want to enable zsh-syntax-highlighting is that it's performance is too bad. If you paste multiple lines of commands at once, your terminal would get stuck for a while which can be longer than 10 seconds.

You may want to set ZSH_HIGHLIGHT_MAXLENGTH=100 in your .zshrc. If you see further issues with zsh-syntax-highlighting, please report them on the z-sy-h issue tracker (or IRC). Thanks.

(I'm a z-sy-h maintainer)

danielshahaf avatar Apr 13 '17 18:04 danielshahaf

@danielshahaf It works perfectly, thank you!

mrjohannchang avatar Apr 13 '17 19:04 mrjohannchang