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

driver: Highlight even after a widget returns non-zero #470

Closed
wants to merge 1 commit into from

Conversation

phy1729
Copy link
Member

@phy1729 phy1729 commented Dec 25, 2017

This needs some thought and discussion before (if) merging. May have yet unconsidered unintended side effects.

Closes #90.

@danielshahaf
Copy link
Member

As discussed on IRC:

The && goes back to the very first commit by @nicoulaj: 89e3c63 line 87

I guess the rationale is that after a general widget returns an error, "Do no harm" kicks in so we avoid setting $region_highlight since we don't know that zle's in a defined state.

Can we distinguish "Completion had no matches" from "Completion had an error"? E.g., for completion widgets, check ${compstate[nmatches]}, or something... (maybe ask zsh-users@)

Thinking out loud — how about if we restrict this to completion widgets? E.g., those that appear in zle -lL or ${(k)widgets[(R)completion:*]}?

@phy1729 phy1729 force-pushed the highlight-after-error branch 2 times, most recently from 14546b3 to a5579e1 Compare January 8, 2018 05:18
@phy1729 phy1729 force-pushed the highlight-after-error branch from a5579e1 to ba58d68 Compare January 20, 2018 00:44
@danielshahaf
Copy link
Member

The incumbent behaviour is actually useful, in a way: typing rsync -4 -6 --<TAB> (which is an error: -4 and -6 are declared mutually exclusive in _rsync) currently gives no indication of the error other than the command word changing from green to uncoloured.

Sure, in a way that's an upstream bug (unclear error reporting), but leaving the command word green might nevertheless be a usability regression.

/cc @blueyed (context: discussion on #zsh today)

@danielshahaf
Copy link
Member

If we change this it would be good to expose the wrappee's code to highlighters, e.g., —

 _zsh_highlight_call_widget()
 {
-  builtin zle "$@" && 
+  builtin zle "$@"
+  local zsh_highlight_wrappee_status=$?
   _zsh_highlight
+  return zsh_highlight_wrappee_status
 }

— although actually reporting the error condition isn't our responsibility (it's the responsibility of the wrappee, and it doesn't communicate the error to the user through $region_highlight so we don't step on its toes).

@phy1729 phy1729 force-pushed the highlight-after-error branch from ba58d68 to 326b174 Compare February 11, 2018 00:51
@phy1729
Copy link
Member Author

phy1729 commented Feb 11, 2018

Asked upstream in users/23100.

@phy1729 phy1729 force-pushed the highlight-after-error branch from 326b174 to 4a1d77f Compare February 11, 2018 15:16
@danielshahaf
Copy link
Member

zsh master does print a warning nowadays, doesn't it? Which would address the above 'usability regression' point.

@phy1729
Copy link
Member Author

phy1729 commented Feb 11, 2018

There was a mailed patch to _arguments and a suggestion to modify _main_complete instead, but as far as I can tell no changes made their way to git.

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

Successfully merging this pull request may close these issues.

2 participants