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

feature/redrawhook breaks zsh-autosuggestions #579

Closed
frebib opened this issue Nov 3, 2018 · 8 comments
Closed

feature/redrawhook breaks zsh-autosuggestions #579

frebib opened this issue Nov 3, 2018 · 8 comments

Comments

@frebib
Copy link

frebib commented Nov 3, 2018

Reproducible:

zsh -f
source path/to/zsh-autosuggestions/zsh-autosuggestions.zsh
source path/to/zsh-syntax-highlighting/zsh-syntax-highlighting.zsh
echo one two three
echo  # notice that 'one two three' are faint from autosuggestions.

Repeat as above but with the feature/redrawhook branch and the 'autosuggested' text is in fact white instead of the default (? it's gray on mine) gray.

@danielshahaf
Copy link
Member

Don't have time to investigate right now, but tentatively milestoning 0.7.0 since there's been talk of finally merging redrawhook for that release.

@danielshahaf
Copy link
Member

Discussion with @romkatv: users/24616

@danielshahaf
Copy link
Member

The patches posted in #418 (comment) (one patch to zsh itself, one patch to z-sy-h) seem to fix this issue for me without any change to z-asug. Could someone confirm, please? (I don't use z-asug myself.)

@danielshahaf
Copy link
Member

Since a fix that requires a zsh patch would take a while to reach everyone, we should also figure out a solution — or at least a more graceful failure mode — that doesn't involve changing zsh itself. What's the best we can do here, without changing zsh itself?

One option is to let z-asug users take the non-redrawhook codepath in z-sy-h, but that would just leave them behind, so it's less than ideal. It would be better to let everyone use the redrawhook codepath, but make the two plugins interoperable. @ericfreese Any thoughts about this?

In particular, in light of @romkatv's analysis, would it help if z-asug registered its own pre-redraw hook (when supported by zsh)? I tried that, but haven't been able to get it to work. My attempt was:

source /usr/share/zsh-autosuggestions/zsh-autosuggestions.zsh
if autoload -U +X add-zle-hook-widget 2>/dev/null; then
        # add-zle-hook-widget zle-line-pre-redraw _zsh_autosuggest_highlight_reset
        # add-zle-hook-widget zle-line-pre-redraw _zsh_autosuggest_highlight_apply
        # add-zle-hook-widget zle-line-pre-redraw _zsh_autosuggest_toggle
        # add-zle-hook-widget zle-line-pre-redraw _zsh_autosuggest_toggle
        # add-zle-hook-widget zle-line-pre-redraw _zsh_autosuggest_enable
fi
source /path/to/zsh-syntax-highlighting.zsh # master, with feature/redrawhook merged

(I uncommented a different line each time.)

@danielshahaf
Copy link
Member

danielshahaf commented May 4, 2020

There was a subtle bug in feature/redrawhook. I've fixed it and will push it in a moment: b08d508. With that bug fixed:

  • Without the owner=* patch for zsh, if I run

        autoload add-zle-hook-widget
        add-zle-hook-widget zle-line-pre-redraw _zsh_autosuggest_fetch
        add-zle-hook-widget zle-line-pre-redraw _zsh_autosuggest_highlight_apply

    after loading both plugins, then both plugins work. (If I don't run that, suggestions are displayed not greyed out.)

  • With the owner=* patches for zsh and z-sy-h, both plugins work regardless of whether z-asug's calls add-zle-hook-widget before or after z-sy-h is sourced.

Note that add-zle-hook-widget has an undocumented feature: add-zle-hook-widget foo 50:bar can be used to specify the index of a hook. If z-sy-h and z-asug both pass an index, and z-sy-h passes a larger index, then z-sy-h's hook will run after z-asug's regardless of the order of the add-zle-hook-widget calls. (EDIT: Sorry, that's just wrong. I misread add-zle-hook-widget's code.)

I'll open a z-asug issue to discuss further.

danielshahaf added a commit that referenced this issue May 4, 2020
…-redraw hooks from running.

Without this patch, `_zsh_highlight` was invoked by add-zle-hook-widget
with `$?` being non-zero (see add-zle-hook-widget:48-52).  Since
`_zsh_highlight` preserves `$?` from its caller's point of view,
add-zle-hook-widget saw a non-zero exit code from `_zsh_highlight` and
did not run any the remaining zle-line-pre-redraw hooks.

See #579 (comment).
@danielshahaf
Copy link
Member

I'll open a z-asug issue to discuss further.

Here: zsh-users/zsh-autosuggestions#529

@danielshahaf
Copy link
Member

danielshahaf commented Jun 26, 2020

zsh-users/zsh#57 (the memo= feature) has been merged to zsh.

With zsh from master (post-merge) and z-sy-h master, I get the autosuggestions after typing the e of the last line, but they disappear on typing the c.

With zsh from master (post-merge) and z-sy-h feature/redrawhook plus the patch in #418 (comment) to use the memo= feature, this issue is fixed.

I expect this will be fixed by merging redrawhook after making it on by default only on zsh that has the memo= feature.

@danielshahaf
Copy link
Member

Fixed as of cb33cc0 — see that commit and its recent parents. Thanks for the easy-to-read report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants