fixes for zle handling

[m0vie]

New pull request replacing #257 and providing a proper solution for what #259 tried to do.

[danielshahaf]

Should _zsh_highlight_set_or_wrap_special_zle_widget do argument validation? For example, check [[ -n $1 ]], or possibly even [[ $1 == zle-* ]]?

Stylistically, I prefer (( ${+widgets[$hook]} )) to [[ -z … ]].

troubleshooting.md: please link to the relevant section of zshzle(1). Since zle-* aren't often added, I would tend to enumerate the current set of such widgets (zle-isearch-exit zle-isearch-update zle-line-init zle-line-finish zle-history-line-set zle-keymap-select), for greppability.

docs:

• troubleshooting.md: Please strike "This leads to some things that have to be considered and some issues": that sentence is not informative.
• The zle-* entry is unclear. (It is clear with the context of our IRC conversation in mind; however, readers won't have that context.) I suggest the following to clarify it: First, state the behaviour (widgets named zle-* do not trigger highlighting, except zle-line-finish and zle-isearch-update); then, give the rationale for it; and finally, address the edge case consequence (user-defined widgets matching that naming pattern won't be wrapped).
• isearch entry: The title is inaccurate: the "minibuffer" is the line below the prompt — the one that execute-named-cmd uses — and it never gets highlighted. The issue is with highlighting the command shown in lieu of $BUFFER whilst an isearch is active.
• isearch entry: The body of the entry doesn't describe the failure mode of Do not highlight in isearch widgets #257 (comment).
• isearch entry: "Therefore" should be followed by a comma.
• isearch entry: s/it is not possible to apply highlighting by z-sy-h/it is not possible for z-sy-h to highlight/
• log message typo: "Old version" → "Old versions"

I haven't tested the fifth commit yet.

[m0vie]

Should _zsh_highlight_set_or_wrap_special_zle_widget do argument validation? For example, check [[ -n $1 ]], or possibly even [[ $1 == zle-* ]]?

I don't think that this is necessary. The function is only used internally, and even with a check for zle-*, invalid input is still possible.

Stylistically, I prefer (( ${+widgets[$hook]} )) to [[ -z … ]].

I do, too. But most of z-sy-h uses [[ -n and [[ -z so I stuck with that.

zsh-* exclusion

I dropped the whole commit excluding zle-* widgets by default. After thinking about it more, there is no reason to:

• If the user has bound some of those hooks already, it is likely that he does something to BUFFER in there. We should respect that and simply wrap the widget just as we did before.
• I initially added the exclusion to avoid binding a widget twice. But that can't happen due to the _zsh_highlight_widget_* check, so there is no need for special handling.

So I also rolled back the troubleshooting.md extraction.

isearch entry: The title is inaccurate: the "minibuffer" is the line below the prompt — the one that execute-named-cmd uses — and it never gets highlighted. The issue is with highlighting the command shown in lieu of $BUFFER whilst an isearch is active.
isearch entry: "Therefore" should be followed by a comma.
isearch entry: s/it is not possible to apply highlighting by z-sy-h/it is not possible for z-sy-h to highlight/

I improved the wording of that paragraph (which can now be found in the FAQ in README.md).

log message typo: "Old version" → "Old versions"

Fixed.

isearch entry: The body of the entry doesn't describe the failure mode of #257 (comment).

The failure from that comment does not apply anymore now that we always bind zle-isearch-update. It can't happen anymore that highlighting from a previous line gets "stuck". The only remaining issue is that in old versions of zsh we can't do both -- apply syntax highlighting and underlining.

[m0vie]

Rebased.

[m0vie]

Changed the last commit about isearch. Removed the zsh-5.3-specific fallback. We can track that separately.

[danielshahaf]

Review of current branch (6fbeac3):

1. Log messages: Please capitalize the first letter of the first line.

2. I don't think that this is necessary. The function is only used internally, and even with a check for zle-*, invalid input is still possible.

   "Being used internally" is a weak argument: bugs can be internal too. However, I think we do need to check this, since otherwise passing no argument will result in the first if branch being entered, and zle -N would be invoked with invalid arguments.

   I'm voting for a ${+widgets[$1]} check.

3. I dropped the whole commit excluding zle-* widgets by default. After thinking about it more, there is no reason to:

   I agree that _zsh_highlight_set_or_wrap_special_zle_widget needn't restrict itself to zle-* widgets only. However, in that case, please update its docstring comment.

   Could you please push the old branch (the one I reviewed in my previous comment) somewhere? I don't have it any more locally, and I'd like to interdiff the previous / current branches and to make sense of my previous review comments.

4. First commit: What if [[ $currentbinding == completion:* ]]? Don't you need to do what _zsh_highlight_bind_widgets does? In fact, shouldn't +_zsh_highlight_set_or_wrap_special_zle_widget and the body of the for loop in _zsh_highlight_bind_widgets share code?

   I think we also need a catchall else branch if the widget exists and the prefix of $widgets[$1] isn't recognised, but sharing code with _zsh_highlight_bind_widgets will fix this.

5. Second commit: I'm convinced that it's correct and would be happy to merge it as soon as the issues in its parent are addressed.

6. Third (tip) commit: Don't say "current zsh versions"; name specific lower or upper bound version numbers. (The word "current" will bitrot.) Also, mention $ISEARCHMATCH_ACTIVE.

7. Third (tip) commit: I'm having trouble reconciling the two comments, the one that says "We don't highlighting in isearch" and the one that says "We hook to isearch to receive updates". The first comment should point to the FAQ entry and the FAQ entry to a zsh-workers@ thread about the zsh API lacuna in question. The second comment should state why we hook isearch (not only that we do so; that is evident from the code), since apparently the purpose isn't for highlighting to run.

Sorry for the long delay — it is entirely my fault. I look forward to merging the first two commits as soon as they're ready (even if the third commit might then need another iteration).

[danielshahaf]

Checklist (for me to review pre-merge):

• Review points in my last comment
• Check Do not highlight in isearch widgets #257 (comment) works
  • zsh 5.0.7 and 5.2-dev, z-sy-h this master: ech in fg=green (no underline), o in fg=default,underline
  • zsh 5.0.7, and 5.2-dev, z-sy-h this branch: fg=default, underline
  • zsh 5.2-dev with the redrawhook branch (danielsh/mikachu-redrawhook/v3) shows the echo in fg=green,underline.
• Check treat zle-isearch-exit like an accept-* widget #259 (comment) works
• Check : /usr/loca<accept-line> removes highlighting
• Question in fixes for zle handling #288 (comment)
• Test everything with a $ISEARCHMATCH_ACTIVE-less zsh
• Test everything with a $ISEARCHMATCH_ACTIVE-ful zsh
• Review new FAQ entry

After merge:

• Arrange for zle-isearch-update and zle-line-finish to be wrapped using add-zle-hook-widget if possible (cf redrawhook issues #245).

[m0vie]

Could you please push the old branch (the one I reviewed in my previous comment) somewhere? I don't have it any more locally, and I'd like to interdiff the previous / current branches and to make sense of my previous review comments.

Old version can be found here: https://github.com/m0vie/zsh-syntax-highlighting/tree/p_allzlefixes_old

1. Log messages: Please capitalize the first letter of the first line.

Done.

6. Third (tip) commit: Don't say "current zsh versions"; name specific lower or upper bound version numbers. (The word "current" will bitrot.) Also, mention $ISEARCHMATCH_ACTIVE.

7. Third (tip) commit: I'm having trouble reconciling the two comments, the one that says "We don't highlighting in isearch" and the one that says "We hook to isearch to receive updates". The first comment should point to the FAQ entry and the FAQ entry to a zsh-workers@ thread about the zsh API lacuna in question. The second comment should state why we hook isearch (not only that we do so; that is evident from the code), since apparently the purpose isn't for highlighting to run.

Done

I agree that the code should be unified. I changed it in the latest version. Now, in _zsh_highlight_bind_widgets it is possible to specify additional hooks that should be bound, even if they are currently unbound. This way, all the code is re-used and the whole thing is much simpler now.

[danielshahaf]

Sorry for the very long delay ☹

Review (of 56b81a9):

1. Log messages: s/'driver':/driver:/

2. First commit: the '' case does not imply that the widget is a zle-* widget:

   $ zsh -f
   % bindkey x foo
   % [[ $widgets[foo] == '' ]] && echo empty
   empty
   

   So, the comment doesn't match the code. One or the other should be fixed.

   Also, the eval there is not needed. It's needed in the other branches for the closure effect (hardcoding the current value of the calling scope's $cur_widget in the body of the defined function), but the function body in this case uses no parameters of the calling scope.

3. Second commit (8dfe7b0) is the same as 2ba33bb (parent of 6fbeac3), which I deemed ready to merge in the previous review iteration.

4. Third commit: please mention the X-Seq of the message you link to. (Or alternatively use a http://www.zsh.org/cgi-bin/mla/redirect?WORKERNUMBER=12345 URL.)

5. Third commit: aren't there some conditions under which syntax highlighting does work during an incremental search? If so, I think the FAQ entry should state them up front (first sentence), to avoid creating a bad impression for FAQ readers who haven't yet installed z-sy-h.

   That first sentence could be something like this:

   While searching history with the history-incremental-search-backward widget (bound by default to Ctrl+R in zsh's bindkey -e keymap), syntax highlighting only works if […].

6. The FAQ entry says "5.3 is", however, when I just merged this branch and then use zle-line-pre-redraw hook if available #261 onto a temporary head (redrawhook issues #245 (comment), second bullet, last paragraph) I got a merge conflict because the other head had "v5.3 it is". Leaving the v bikeshed aside, the it is correct, so it seems something got lost in one of the rebases somewhere. Anyway, please re-add that it.

[danielshahaf]

Second commit (8dfe7b0) is the same as 2ba33bb (parent of 6fbeac3), which I deemed ready to merge in the previous review iteration.

Thinking ahead, that commit adds zle-line-finish wrapping. And zle-line-finish is one of the widgets supported by add-zle-hook-widget. So I think the #245 solutions (#245 (comment)) will want to treat zle-line-finish and zle-line-pre-redraw the same way; namely: use add-zle-hook-widget if available and the current custom approach otherwise.

I don't think this consideration affects this branch, though; it's just something the other branches will need to do after 8dfe7b0 is merged.

edit: This also applies to zle-isearch-update.

[danielshahaf]

I've implemented the suggestions from my penultimate comment on a branch: https://github.com/zsh-users/zsh-syntax-highlighting/compare/master...danielshahaf:m0vie-i288-v2?expand=1

I dropped the link to the "API for zsh5.3 concerning ISEARCHMATCH_ACTIVE" thread since it didn't seem relevant to the comment next to it (the thread is about 5.3 behaviour but the code is about 5.2 support); did you mean to link to a different thread?

I dropped the explanation from the first log message since upstream now documents that (commit users/21752: zsh-users/zsh@18d676f).

[m0vie]

Thank you for taking care of it. I'm very busy lately and didn't have time to look at it.

I agree with all the changes you made.

Just two small typos left:

"Under those version of zsh it is not possible for..."
-> "versions"

"Bind to z-sy-h driectly."
-> "directly"

Should I update this PR again, or are you going to merge your changes?

[danielshahaf]

Fixed both and force-pushed my branch. Thanks for the review.

My plan is first of all to get add-zle-hook-widget sorted upstream (workers/38933), then this issue, then the redrawhook issues. I posted my review checklist for this issue hereinabove.

Re updating the PR, feel free: git push m0vie +danielsh/m0vie-i288-v2:p_allzlefixes It'd be a good thing — special cases are the root of all evil.

Quick question, what do you think of limiting the new codepath master...danielshahaf:m0vie-i288-v2#diff-4ed73021a0fa2ff4e78a9909f4fc2f9bR299 to zle-* widgets? That is, of making that codepath behave as it does now for [[ $cur_widget == zle-* ]] and as the * codepath otherwise.

[danielshahaf]

@phy1729 If you have a moment I'd love a third pair of eyes over the upcoming FAQ entry: master...danielshahaf:m0vie-i288-v2#diff-04c6e90faac2675aa89e2176d2eec7d8R35 Thanks!

[m0vie]

Quick question, what do you think of limiting the new codepath master...danielshahaf:m0vie-i288-v2#diff-4ed73021a0fa2ff4e78a9909f4fc2f9bR299 to zle-* widgets? That is, of making that codepath behave as it does now for [[ $cur_widget == zle-* ]] and as the * codepath otherwise.

Sounds like a good idea to be on the safe side!

Re updating the PR, feel free:

OK, I'll update it with your changes later.

[danielshahaf]

Sounds like a good idea to be on the safe side!

I wrote a quick draft of this and appended it to the branch.

OK, I'll update it with your changes later.

Okay. I've stopped making changes to the branch here; the mutex is yours.

[m0vie]

Merged in 171a4ee

[danielshahaf]

And after the merge 4ad311e was committed, q.v..