Skip to content

Commit

Permalink
disable "memo" in zsh-syntax-highlighting
Browse files Browse the repository at this point in the history 
See zsh-users/zsh-syntax-highlighting#418.
  • Loading branch information
@romkatv
romkatv committed Jul 14, 2020
1 parent f5cde88 commit 3060d8e
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions fn/-z4h-init
View file
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,8 @@ if (( _z4h_use[zsh-syntax-highlighting] )); then
fi

if (( _z4h_use[zsh-syntax-highlighting] && _z4h_use[zsh-autosuggestions] )); then
typeset -gi zsh_highlight__memo_feature=0

local event
for event in pre-redraw init finish; do
if (( $+widgets[zle-line-$event] )); then
Expand Down

8 comments on commit 3060d8e

@danielshahaf
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask what the rationale for this is? I wonder if there are changes that zsh or z-sy-h should make around the memo= feature.

@romkatv
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zsh4humans disables widget wrapping in z-sy-h and z-asug and manually invokes them at certain points. It effectively uses z-sy-h and z-asug as if they were libraries with no hooks of their own. This yields several improvements compared to simply loading z-sy-h and z-asug.

  1. Better performance through elimination of unnecessary z-sy-h and z-asug calls. Such calls normally result from widgets that invoke other zle widgets, each of which would normally get wrapped. E.g., a widget that invokes zle kill-word twice will trigger z-asug twice but not in zsh4humans.
  2. Elimination of race conditions when keyboard input is buffered. In zsh4humans the same sequence of keystrokes will always yield the same final autosuggestion and the same syntax highlighting when it gets processed regardless of how long it takes to process each individual key (this property doesn't hold neither in z-sy-h nor in z-asug on their own).
  3. Correct highlighting when invoking all widgets, including run-help and the like.
  4. Correct highlighting in the following case:
% WORDCHARS=
% bindkey '^[f' forward-word
% rm -rf -- /tmp/test-*(N)
% touch /tmp/test-a /tmp/test-
% rm /tmp/test-a
% touch /tmp/<ALT+f>

With the stock z-sy-h and z-asug /tmp/test- on the last line will end up without an underline (because /tmp/test-a doesn't exist). In zsh4humans it'll be underlined (because /tmp/test- exists).

These aren't huge improvements. Some can be backported to the individual projects but not all of them. E.g., (1) and (4) can be quite difficult to fix without introducing tighter coupling between z-sy-h and z-asug or new interop API.

I've disabled memo feature because zsh4humans doesn't need it and because disabling it yields faster code path. Granted, it's almost certainly not measurable.

To summarize, my code would benefit from z-sy-h as a library (I recall there was a long-standing feature request for this). The new memo feature would belong to z-sy-h the plugin but not to z-sy-h the library.

@danielshahaf
Copy link

@danielshahaf danielshahaf commented on 3060d8e Jul 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed answer, @romkatv. Issues (1) and (4), at least, will be fixed by merging zsh-users/zsh-syntax-highlighting#579 zsh-users/zsh-syntax-highlighting#749, which is literally the next thing on my z-sy-h todo list. (4) is zsh-users/zsh-syntax-highlighting#90. I'm not sure about (2) and (3), whether or not they'd also be fixed by that branch. (No bugs were reported to z-sy-h over either of these, and I don't have time to test them right now.)

The catch is, the z-sy-h fixes will only be enabled when the memo feature is enabled, so if you disable that feature, your users will not receive those fixes (and a dozen others) even when they upgrade to a zsh that has the memo feature.

Thanks again.

@romkatv
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed answer, @romkatv. Issues (1) and (4), at least, will be fixed by merging zsh-users/zsh-syntax-highlighting#579, which is literally the next thing on my z-sy-h todo list. (4) is zsh-users/zsh-syntax-highlighting#90. I'm not sure about (2) and (3), whether or not they'd also be fixed by that branch. (No bugs were reported to z-sy-h over either of these, and I don't have time to test them right now.)

Thanks for the info!

The catch is, the z-sy-h fixes will only be enabled when the memo feature is enabled, so if you disable that feature, your users will not receive those fixes

My users don't need these fixes. It already works correctly for them.

@romkatv
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(4) is zsh-users/zsh-syntax-highlighting#90.

I had a quick look and it appears to be a different issue. (4) on my list is caused by z-sy-h applying highlighting to $BUFFER$POSTBUFFER, which might be different from highlighting just $BUFFER. There are no failing widgets in the test case I listed. It looks like it should be fixed once z-sy-h starts highlighting on zle-pre-redraw.

Either way, for my use case it would be perfect to have z-sy-h as a library. Absence that API I'm dynamically disabling widget wrapping and all sort of hooks that z-s-h installs and then manually calling it. This allows me to achieve perfect integration of z-sy-h, z-asug, fzf-tab and everything else. It has extra maintenance cost but it's not high.

@danielshahaf
Copy link

@danielshahaf danielshahaf commented on 3060d8e Jul 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(4) on my list is caused by z-sy-h applying highlighting to $BUFFER$POSTBUFFER.

z-sy-h doesn't use $POSTBUFFER: https://github.com/zsh-users/zsh-syntax-highlighting/blob/2d60a47cc407117815a1d7b331ef226aa400a344/highlighters/main/main-highlighter.zsh#L424

There are no failing widgets in the test case I listed.

Okay, sorry for misreading it.

Either way, for my use case it would be perfect to have z-sy-h as a library. Absence that API I'm dynamically disabling widget wrapping and all sort of hooks that z-s-h installs and then manually calling it. This allows me to achieve perfect integration of z-sy-h, z-asug, fzf-tab and everything else. It has extra maintenance cost but it's not high.

Well, you're very welcome to open one or more bug reports on https://github.com/zsh-users/zsh-syntax-highlighting/issues describing your use-cases, and we can take them from there. For example, offering a way not to opt out of installation of widgets/hooks certainly sounds reasonable.

@romkatv
Copy link
Owner Author

@romkatv romkatv commented on 3060d8e Jul 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

z-sy-h doesn't use $POSTBUFFER

I know. Here's what happens.

  1. The user invokes a widget that's listed in ZSH_AUTOSUGGEST_PARTIAL_ACCEPT_WIDGETS.
  2. z-asug takes over. It sets BUFFER to $BUFFER$PREBUFFER and invokes the underlying widget.
  3. The underlying widget is a wrapper from z-sy-h. It invokes the actual widget (which moves the cursor to the right) and then applies highlighting.
  4. z-asug gets control back. It truncates BUFFER at CURSOR.

The end result is that BUFFER may be highlighted incorrectly.

Well, you're very welcome to open one or more bug reports on https://github.com/zsh-users/zsh-syntax-highlighting/issues describing your use-cases, and we can take them from there. For example, offering a way not to opt out of installation of widgets/hooks certainly sounds reasonable.

Thanks! I'll keep it mind. zsh4humans is still under active development and experimentation, so it might be premature to request features that I may not need in a week or in a month.

Just to be clear, I realize that the current usage of z-sy-h in z4h is unsupported and that my code can be broken by z-sy-h updates. You'll see no complaints from me if that happens.

@danielshahaf
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The end result is that BUFFER may be highlighted incorrectly.

I see. Again, this wasn't reported before as a bug. What did get reported is zsh-users/zsh-syntax-highlighting#579, and zsh-users/zsh-syntax-highlighting#749 will fix it under zsh 5.9+.

Correction: A few comments ago when I linked to 579 I had intended to link to 749. Sorry.

Thanks! I'll keep it mind. zsh4humans is still under active development and experimentation, so it might be premature to request features that I may not need in a week or in a month.

Thanks ☺

Just to be clear, I realize that the current usage of z-sy-h in z4h is unsupported and that my code can be broken by z-sy-h updates. You'll see no complaints from me if that happens.

Fair enough, but at the same time, if there's anything you need from us, don't hesitate to reach out. Cheers!

Please sign in to comment.