zsh-syntax-highlighting interoperability: Please register zle-line-pre-redraw hooks

[danielshahaf]

Is your feature request related to a problem? Please describe.

As described in zsh-users/zsh-syntax-highlighting#579, zsh-syntax-highlighting's feature/redrawhook branch and z-asug don't interoperate.

See zsh-users/zsh-syntax-highlighting#579 (comment) for my latest analysis.

Describe the solution you'd like

Solution in the short term: z-asug should run

if autoload -U +X add-zle-hook-widget 2>/dev/null; then
    add-zle-hook-widget zle-line-pre-redraw _zsh_autosuggest_fetch
    add-zle-hook-widget zle-line-pre-redraw _zsh_autosuggest_highlight_apply
fi

at some point after z-sy-h ran its add-zle-hook-widget zle-line-pre-redraw … commands. This way, z-asug and z-sy-h will both work.

Describe alternatives you've considered

In the longer term, I have patches for zsh and z-sy-h that, between them, allow z-sy-h and z-asug to coexist with no changes to z-asug and without the relative ordering requirement described above: zsh-users/zsh-syntax-highlighting#418 (comment)

However, a patch that requires rebuilding zsh is not a practical option for users in the short term.

Furthermore, I have considered making a minor API change to add-zle-hook-widget, along these lines —

diff --git a/Doc/Zsh/contrib.yo b/Doc/Zsh/contrib.yo
index 0909cd4f5..81c4c9e53 100644
--- a/Doc/Zsh/contrib.yo
+++ b/Doc/Zsh/contrib.yo
@@ -328,7 +328,7 @@ tt(autoload) for var(function).  For functions contributed with zsh, the
 options tt(-Uz) are appropriate.
 )
 findex(add-zle-hook-widget)
-item(tt(add-zle-hook-widget) [ tt(-L) | tt(-dD) ] [ tt(-Uzk) ] var(hook) var(widgetname))(
+item(tt(add-zle-hook-widget) [ tt(-L) | tt(-dD) ] [ tt(-Uzk) ] var(hook) [var(index)tt(:)]var(widgetname))(
 Several widget names are special to the line editor, as described in the section
 ifnzman(Special Widgets, noderef(Zle Widgets))\
 ifzman(Special Widgets, see zmanref(zshzle)),
@@ -345,9 +345,15 @@ tt(zle-isearch-exit), etc.  The special widget names are also accepted
 as the var(hook) argument.
 
 var(widgetname) is the name of a ZLE widget.  If no options are given this
-is added to the array of widgets to be invoked in the given hook context.
-Widgets are invoked in the order they were added, with
-example(tt(zle )var(widgetname)tt( -Nw -- "$@"))
+is added to the array of widgets to be invoked in the given hook context,
+with
+
+example(tt(zle )var(widgetname)tt( -Nw -- "$@")).
+
+var(index) should be a non-negative integer.  Widgets are invoked in order of
+their var(index)es, from smallest to largest.  If var(index) is unspecified,
+var(widgetname) will be run after all widgets registered at the time of the
+call.
 
 vindex(WIDGET, in hooks)
 Note that this means that the `tt(WIDGET)' special parameter tracks the

— but I think a solution that doesn't involve changes to zsh would be preferable, even if the changes don't require recompilation to be deployed.

Additional context

Under zsh's that support add-zle-hook-widget, there's no need to wrap all widgets. However, that's merely an optimization, and doesn't block fixing the interoperability bug.

z-sy-h wraps zle-line-finish as well, so z-asug may want to wrap that as well (I haven't tested).

I think this may address #483. I haven't tested #158 and #294.

tl;dr

Please have z-asug do those two add-zle-hook-widget calls so I can merge feature/redrawhook on the z-sy-h side ☺

Thanks!

[ericfreese]

Thanks for the analysis @danielshahaf! I’ll take a look at this soon

[ericfreese]

@danielshahaf Would it be possible/helpful for z-sy-h to kind of simulate the effect of the owner patch by keeping track of all of its previously applied region highlights and only clearing out those ones that it knows it had set previously? That's basically what I do in z-asug- I keep the last highlight around in $_ZSH_AUTOSUGGEST_LAST_HIGHLIGHT and only remove it specifically from region_highlight to avoid clashes with other plugins. Granted I only have one highlight to keep track of where z-sy-h has many, but I'm curious your thoughts on this since I didn't see it mentioned in any of the threads.

[danielshahaf]

@ericfreese It would be nice to be able to simulate the owner functionality on zsh's that don't have it, but I don't see how that would work.

Suppose z-sy-h is called by a self-insert of a space when [[ -z $PREBUFFER && -z $LBUFFER && $RBUFFER == 'ls' ]]. In that case, the last _zsh_highlight call before the self-insert will have set typeset -g -a region_highlight=( '0 2 fg=green' ), but when _zsh_highlight is next entered it will see typeset -g -a region_highlight=( '1 3 fg=green' ) — so it seems z-sy-h will have to munge the remembered last-set value into the as-seen-on-entry value.

For self-insert specifically I suppose z-sy-h could do such munging itself, but I don't see how to perform this transformation in the general case. For example, quote-region would be non-trivial to support, because it might insert not only opening and closing quotes but also backslashes to escape quote characters within the region.

However, I suspect I'm missing something here.

Secondly, there are two less likely cases: if someone calls _zsh_highlight and then post-processes the outputs it adds, or conversely if someone only calls _zsh_highlight at specific times (rather than via the pre-redraw hook or by wrapping all widgets), the assumption underlying your proposal wouldn't hold.

Of course, that's just me; other z-sy-h developers may have a different take. We should discuss this on the z-sy-h issue tracker, if needed, so everyone can participate.

[ericfreese]

Ok you're right about how region_highlight works. The approach only works in z-asug's case because we remove the highlight before the widget runs. I agree you probably don't want to try to do any munging of the last-set value.

I've tried naively adding the two add-zle-hook-widget calls to _zsh_autosuggest_start on develop branch and it unfortunately breaks a few tests. Mostly it seems related to async mode and widgets in ZSH_AUTOSUGGEST_IGNORE_WIDGETS causing a suggestion fetch. For example, it breaks yank-pop functionality when async mode is enabled (which it is by default on develop branch). Unfortunately, I think an upstream patch will be necessary to switch fully to a redraw-hook implementation here so that autosuggest-suggest widget called asynchronously doesn't set lbindk or lastcmd. Or if there is some way to detect which widget was called directly before the redraw hook is executed maybe that could work but I don't see a way to do that without wrapping those widgets? I also don't like that the suggested fix ends up calling fetch (and highlight) twice for each keystroke.

I'm a bit short on time for at least the next few weeks, but if you (or anyone) has time to provide a PR that fixes the interop while keeping the test suite green and ideally not calling fetch twice per keystroke, it would help greatly.

Generally though, I'm concerned that zsh-users/zsh-syntax-highlighting#418 is a show-stopper for merging the redraw hook branch. It seems it will break and force modifications on any plugin that happens to use region_highlight. I like the idea of the "owner" patch, but I wonder if a more general term like "tag" or "id" might be a better fit since owner semantics may not always be applicable.

As another idea for a possible solution, could z-sy-h implement a POSTDISPLAY highlighter that would supersede the highlighting provided by z-asug? One might make the argument that if z-sy-h is going to clear region_highlight, that it should take ownership of all highlights.

[danielshahaf]

I've tried naively adding the two add-zle-hook-widget calls to _zsh_autosuggest_start on develop branch and it unfortunately breaks a few tests. Mostly it seems related to async mode and widgets in ZSH_AUTOSUGGEST_IGNORE_WIDGETS causing a suggestion fetch. For example, it breaks yank-pop functionality when async mode is enabled (which it is by default on develop branch). Unfortunately, I think an upstream patch will be necessary to switch fully to a redraw-hook implementation here so that autosuggest-suggest widget called asynchronously doesn't set lbindk or lastcmd. Or if there is some way to detect which widget was called directly before the redraw hook is executed maybe that could work but I don't see a way to do that without wrapping those widgets?

Thanks for having a look.

lbindk is wrapped as $LASTWIDGET, and that variable seems to have the needed value:

$ zsh -f
% f() { zle -M "$(typeset -p LASTWIDGET)" }
% autoload add-zle-hook-widget 
% add-zle-hook-widget line-pre-redraw f 
% x<CURSOR>
typeset -r LASTWIDGET=self-insert

Furthermore, zle autosuggest-suggest is called without the -w flag, so it seems to me that using $LASTWIDGET rather than $WIDGET for ZSH_AUTOSUGGEST_IGNORE_WIDGETS filtering should just work. Actually, the hook function could shadow $WIDGET (shadowing the value WIDGET=f with WIDGET=self-insert, for example), so no changes to the rest of the code would be needed.

What am I missing?

I also don't like that the suggested fix ends up calling fetch (and highlight) twice for each keystroke.

Neither do I. The proposal was intended to be a proof-of-concept, the smallest change that would achieve correctness; I didn't know z-asug's code well enough to devise a complete patch.

To avoid the duplicate work, couldn't the widget-binding code simply be skipped altogether when pre-redraw mode is in use? That's what z-sy-h does: _zsh_highlight_bind_widgets is a no-op when the pre-redraw codepath is taken:

https://github.com/zsh-users/zsh-syntax-highlighting/blob/b08d508cd8792df2b6c8e044e42dffeb7f9118fe/zsh-syntax-highlighting.zsh#L339
https://github.com/zsh-users/zsh-syntax-highlighting/blob/b08d508cd8792df2b6c8e044e42dffeb7f9118fe/zsh-syntax-highlighting.zsh#L359-L363
https://github.com/zsh-users/zsh-syntax-highlighting/blob/b08d508cd8792df2b6c8e044e42dffeb7f9118fe/zsh-syntax-highlighting.zsh#L364-L367

I'm a bit short on time for at least the next few weeks, but if you (or anyone) has time to provide a PR that fixes the interop while keeping the test suite green and ideally not calling fetch twice per keystroke, it would help greatly.

Thanks for the invitation. Simply providing a patch that disables the widget binding if pre-redraw is available and uses $LASTWIDGET shouldn't be too hard; however, setting up the test suite to be run locally would seem to require a non-trivial amount of tuits.

Anyway, I assume the approach should be as follows:

1. If pre-redraw is not supported, same as current develop.

2. Otherwise:

   2.1. Install pre-redraw hooks.

   2.2. Don't bind widgets.

   2.3. Make sure the test suite passes. (Probably requires using $LASTWIDGET or shadowing, as above.)

Generally though, I'm concerned that zsh-users/zsh-syntax-highlighting#418 is a show-stopper for merging the redraw hook branch. It seems it will break and force modifications on any plugin that happens to use region_highlight.

Merging feature/redrawhook would cause region_highlight to be cleared at a later point, yes.

On the other hand, I don't think it's practical to keep feature/redrawhook waiting until everyone has upgraded to a zsh with the owner= patch.

Perhaps z-sy-h could merge feature/redrawhook but have the code continue to use the "wrap all widgets" strategy unless zsh has the owner= patch. That should avoid regressions. It would leave users of z-sy-h HEAD and zsh new enough to support pre-redraw hook on the legacy codepath, though, but I'm not sure if there's a better option.

I like the idea of the "owner" patch, but I wonder if a more general term like "tag" or "id" might be a better fit since owner semantics may not always be applicable.

We can change the name, of course. Plenty of time to decide on a name later on, though; getting the semantics right is a higher priority. Does the patch look okay, aside from the choice of words? I am particularly interested in your take on the API docs part of the patch, as you maintain a region_highlight-related plugin.

As another idea for a possible solution, could z-sy-h implement a POSTDISPLAY highlighter that would supersede the highlighting provided by z-asug? One might make the argument that if z-sy-h is going to clear region_highlight, that it should take ownership of all highlights.

Sorry, could you clarify? I assume you're thinking of the case of someone using z-asug HEAD + z-sy-h HEAD post the feature/redrawhook merge + zsh without zsh-users/zsh#57 [which, as of this writing, hasn't been merged, let alone released], and proposing that in that case z-sy-h should arrange for z-asug's POSTDISPLAY region_highlight entries to be re-added.

That would seem to amount to reimplementing zle-line-pre-redraw in z-sy-h, and it would still require code changes in every plugin affected by zsh-users/zsh-syntax-highlighting#418, wouldn't it?

Cheers! 👍

[ericfreese]

What am I missing?

I was noticing the problem when adding a second line-pre-redraw hook (as z-asug would be doing on top of the one already added by z-sy-h):

$ zsh -df
% autoload add-zle-hook-widget
% f() {}
% g() { zle -M "$(typeset -p LASTWIDGET)" }
% add-zle-hook-widget line-pre-redraw f
% add-zle-hook-widget line-pre-redraw g
% x<CURSOR>
typeset -r LASTWIDGET=f

The triggering widget name is lost once the first redraw hook runs.

Neither do I. The proposal was intended to be a proof-of-concept, the smallest change that would achieve correctness; I didn't know z-asug's code well enough to devise a complete patch.

To avoid the duplicate work, couldn't the widget-binding code simply be skipped altogether when pre-redraw mode is in use?

Ok gotcha, yes I agree this should be possible 😄

setting up the test suite to be run locally would seem to require a non-trivial amount of tuits

I hope it's not too bad. If you have docker installed, you can run the tests with from the project directory with:

docker run --rm -it -e TEST_ZSH_BIN=zsh-5.7.1 -v $PWD:/zsh-autosuggestions ericfreese/zsh-autosuggestions-test bundle exec rspec

You should be able to change the value of TEST_ZSH_BIN to any of the available versions in the ZSH_VERSIONS file to run the tests under that specific version.

Does the patch look okay, aside from the choice of words? I am particularly interested in your take on the API docs part of the patch, as you maintain a region_highlight-related plugin.

I'll take a closer look at this in the coming days/weeks and get back to you.

I assume you're thinking of the case of someone using z-asug HEAD + z-sy-h HEAD post the feature/redrawhook merge + zsh without zsh-users/zsh#57 [which, as of this writing, hasn't been merged, let alone released]

Yeah, sorry for the confusion. This suggestion is for exactly the scenario you've outlined. It's meant as a short term solution before zsh-users/zsh#57 is released and widely available- a possible alternative to implementing fixes for zsh-users/zsh-syntax-highlighting#418 throughout all region_highlight-using plugins.

and proposing that in that case z-sy-h should arrange for z-asug's POSTDISPLAY region_highlight entries to be re-added.

That would seem to amount to reimplementing zle-line-pre-redraw in z-sy-h

I haven't looked into exactly how the highlighters work, but I'm thinking it would work like any of the other highlighters. Only it would highlight the POSTDISPLAY. Depending on how much you wanted to couple to z-asug, it could even pull from the ZSH_AUTOSUGGEST_HIGHLIGHT_STYLE variable. Or it could just be a generic "postdisplay" highlighter with its own config vars.

and it would still require code changes in every plugin affected by zsh-users/zsh-syntax-highlighting#418, wouldn't it?

Code changes would be required for each plugin affected by zsh-users/zsh-syntax-highlighting#418 in the form of highlighters being added. The difference is that those code changes would be contained within the z-sy-h repo (the one deciding to empty region_highlight) rather than scattered throughout other repos. You would have ownership over them and once zsh-users/zsh-syntax-highlighting#418 is fixed, those highlighters could be deprecated and removed.

[danielshahaf]

The triggering widget name is lost once the first redraw hook runs.

Ouch. Yes, I see what you mean now. I think the best you can do here under zsh 5.8 and earlier is to continue to wrap widgets, but not with the existing wrapper that does autosuggesting, but with another wrapper that just does () { typeset -g _Z_ASUG_LASTWIDGET_CACHE=$LASTWIDGET } (so there won't be duplicate work). For zsh 5.9 we could propose changes to add-zle-hook-widget to arrange for the value of $LASTWIDGET to be available, one way or the other. Let's discuss this last bit on zsh-workers@, shall we?

I hope it's not too bad. If you have docker installed, you can run the tests with from the project directory with:

Thanks. I don't have docker already installed, so I'll have to set that up first, and I'll probably do that in a VM, which I'd have to install first… fortunately, it's not turtles all the way down — the turtles are only two or three layers deep — so the problem is solvable; just need time. (And in the meantime we're making good progress with designing the solution.)

I'll take a closer look at this in the coming days/weeks and get back to you.

Thanks! I'll take the liberty of pressing the "Request review" button in github, then; let me know if you prefer I cancel that.

Yeah, sorry for the confusion. This suggestion is for exactly the scenario you've outlined. It's meant as a short term solution before zsh-users/zsh#57 is released and widely available- a possible alternative to implementing fixes for zsh-users/zsh-syntax-highlighting#418 throughout all region_highlight-using plugins.

Yes, it sounds like merging redrawhook will be a flag day for all such plugins. It won't particularly help that many z-sy-h users use the master branch directly (and z-sy-h doesn't have a more-bleeding-edge branch). And I'm not aware of a reliable way to get a message out to all users (or, more accurately, to all the maintainers of plugins whose users intersect with z-sy-h's users). So I'm inclined to have z-sy-h merge redrawhook but disable it by default on zsh that doesn't have the owner= patch. This way, there'll be no breakage to third parties at all (and even if I'm overlooking something, that breakage would only happen upon rebuilding zsh, not upon updating to z-sy-h HEAD from github); in particular, there'll be no coupling or dependencies between z-asug changes and z-sy-h changes, so z-asug will be able to upgrade to pre-redrawhook at its leisure.

I've opened zsh-users/zsh-syntax-highlighting#735 for this.

I haven't looked into exactly how the highlighters work, but I'm thinking it would work like any of the other highlighters.

In a nutshell, a highlighter is simply a callback function with a well-known name (_zsh_highlight_highlighter_acme_paint) that appends stuff to region_highlight. The details are at the bottom of the highlighters/ page.

Code changes would be required for each plugin affected by zsh-users/zsh-syntax-highlighting#418 in the form of highlighters being added. The difference is that those code changes would be contained within the z-sy-h repo (the one deciding to empty region_highlight) rather than scattered throughout other repos. You would have ownership over them and once zsh-users/zsh-syntax-highlighting#418 is fixed, those highlighters could be deprecated and removed.

Hmm. I'm not sure I like this idea. This would require the z-sy-h maintainers to maintain code for every single other plugin that uses region_highlight… and there's no reason to assume all such plugins are public, either. I think I'd prefer for workarounds to be deployed in the other plugins: this distributes maintenance effort to the people more familiar with those plugins, and doesn't require different technical solutions depending on whether a third-party plugin is public or private.

However, as above, my preferred approach is to make the redrawhook codepath conditional on the owner= codepath, which solves the interoperability concerns without any changes to other plugins.

[danielshahaf]

Ouch. Yes, I see what you mean now. I think the best you can do here under zsh 5.8 and earlier is to continue to wrap widgets, but not with the existing wrapper that does autosuggesting, but with another wrapper that just does () { typeset -g _Z_ASUG_LASTWIDGET_CACHE=$LASTWIDGET } (so there won't be duplicate work). For zsh 5.9 we could propose changes to add-zle-hook-widget to arrange for the value of $LASTWIDGET to be available, one way or the other. Let's discuss this last bit on zsh-workers@, shall we?

Thread: workers/46004

[danielshahaf]

Thread: workers/46004

Committed in zsh-users/zsh@0cffb0a.

That hopefully unrolls the discussion back to #529 (comment):

I've tried naively adding the two add-zle-hook-widget calls to _zsh_autosuggest_start on develop branch and it unfortunately breaks a few tests. Mostly it seems related to async mode and widgets in ZSH_AUTOSUGGEST_IGNORE_WIDGETS causing a suggestion fetch. For example, it breaks yank-pop functionality when async mode is enabled (which it is by default on develop branch). Unfortunately, I think an upstream patch will be necessary to switch fully to a redraw-hook implementation here so that autosuggest-suggest widget called asynchronously doesn't set lbindk or lastcmd. Or if there is some way to detect which widget was called directly before the redraw hook is executed maybe that could work but I don't see a way to do that without wrapping those widgets? I also don't like that the suggested fix ends up calling fetch (and highlight) twice for each keystroke.

I'm a bit short on time for at least the next few weeks, but if you (or anyone) has time to provide a PR that fixes the interop while keeping the test suite green and ideally not calling fetch twice per keystroke, it would help greatly.

Generally though, I'm concerned that zsh-users/zsh-syntax-highlighting#418 is a show-stopper for merging the redraw hook branch. It seems it will break and force modifications on any plugin that happens to use region_highlight. I like the idea of the "owner" patch, but I wonder if a more general term like "tag" or "id" might be a better fit since owner semantics may not always be applicable.

Since that comment, the "owner" patch referenced therein has been committed, and zsh-users/zsh-syntax-highlighting#418 is expected to be fixed in z-sy-h soon, which will fix the interop issue under zsh≥5.9 without z-asug changes (see zsh-users/zsh-syntax-highlighting#579 (comment); the testing described in that comment was done with z-asug's develop branch as of today). As described above, I'm planning to leave redrawhook off by default on zsh<5.9 for interop purposes, which would address — more precisely, sidestep — the interop issues under those versions of zsh (for all plugins, not just for z-sy-h with z-asug). Combined, these steps would leave this issue as an ordinary feature request without interop significance.

Makes sense?

[danielshahaf]

zsh-users/zsh-syntax-highlighting#418 was just merged to z-sy-h.

[danielshahaf]

Since that comment, the "owner" patch referenced therein has been committed, and zsh-users/zsh-syntax-highlighting#418 is expected to be fixed in z-sy-h soon, which will fix the interop issue under zsh≥5.9 without z-asug changes (see zsh-users/zsh-syntax-highlighting#579 (comment); the testing described in that comment was done with z-asug's develop branch as of today). As described above, I'm planning to leave redrawhook off by default on zsh<5.9 for interop purposes, which would address — more precisely, sidestep — the interop issues under those versions of zsh (for all plugins, not just for z-sy-h with z-asug). Combined, these steps would leave this issue as an ordinary feature request without interop significance.

So, zsh-users/zsh-syntax-highlighting#418 has been fixed in z-sy-h master and zsh-users/zsh-syntax-highlighting#579 was fixed on the redrawhook branch. That resolves the interoperability concerns with current z-asug master and develop branches, using zsh either (a) 5.7.1, (b) current master (5.9-to-be), or (c) 5.7.1 with z-sy-h modified to unconditionally take the non-redrawhook codepath, simulating a pre-5.2 zsh.

This voids the original justification for the proposed change. The change may still be a good idea, but it's no longer an interop concern, so I don't expect I'll be working on the z-asug change after all. Thanks for the support, though!

[danielshahaf]

By the way, I think that means using the feature/redrawhook branch may be a viable workaround for some of the issues mentioned in the OP.