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.

Sign up for GitHub

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

Jump to bottom

use zle-line-pre-redraw hook if available #261

Closed
wants to merge 1 commit into from
Closed

use zle-line-pre-redraw hook if available #261

wants to merge 1 commit into from

Conversation

m0vie
Copy link
Contributor

@m0vie m0vie commented Dec 31, 2015

The widget binding logic is kept, so that old versions of zsh still work as before.

_zsh_highlight_bind_widgets now accepts parameters, to specify which widgets to bind.

This is needed because with the redraw-hook we are not notifed for zle-isearch-* anymore, but we still need to react on those. Maybe this is an upstream issue?

TODO: The version numer in is-at-least actually needs to be 5.2.1, once officially released.

This fixes #245.

@danielshahaf
Copy link
Member

I have my own version of that at master...danielshahaf:mikachu-redrawhook/v2

Differences from your version:

  • No special-casing of zle-isearch-update zle-isearch-exit
  • Different ways of detecting support for the new hook (solve the is-at-least issue; btw the next release will be called 5.3)
  • Different error handling

@danielshahaf
Copy link
Member

This is needed because with the redraw-hook we are not notifed for zle-isearch-* anymore, but we still need to react on those. Maybe this is an upstream issue?

Possibly. A zle-isearch-update widget that modifies BUFFER doesn't trigger zle-line-pre-redraw. Raise this subissue on -workers@?

@danielshahaf danielshahaf mentioned this pull request Jan 3, 2016
Closed
@danielshahaf
Copy link
Member

@m0vie Ping, would you raise the zle-line-pre-redraw and isearch issue on -workers, please? I think we should have that discussion before that hook is released by zsh.

@m0vie
Copy link
Contributor Author

m0vie commented Jan 10, 2016

I talked to Mikael on IRC and the he said the code was moved to the zle input loop for performance reasons to avoid multiple triggers. Hoewever the input loop is not triggered while minibuffers are active.

I raised the issue on -workers@ again.

@danielshahaf
Copy link
Member

I raised the issue on -workers@ again.

At: http://www.zsh.org/mla/workers/2016/threads.html#00092 (workers/37557)

@danielshahaf
Copy link
Member

Once this is done, we can stop requiring that z-sy-h be the last thing sourced in zshrc. (So docs will need to be updated)

@danielshahaf
Copy link
Member

... but we should document instead that z-sy-h registers a zle-line-pre-redraw hook, so if the user has such a hook (directly or via another plugin), he should arrange for his hook to call z-sy-h's hook. (Upstream discussion: workers/37639).

@danielshahaf
Copy link
Member

Possibly. A zle-isearch-update widget that modifies BUFFER doesn't trigger zle-line-pre-redraw. Raise this subissue on -workers@?

This should have been fixed in zsh master, see #259 (comment)

@m0vie m0vie force-pushed the p_redrawhook branch 2 times, most recently from 88e0d5b to 1f9d60c Compare March 28, 2016 21:00
@m0vie
Copy link
Contributor Author

m0vie commented Mar 28, 2016

Updated the pull request.
In case zle-line-pre-redraw is already bound, it is wrapped. In this case z-sy-h still needs to be loaded last, so I left the docs unchanged.

@danielshahaf I'm still using is-at-least for now. I looked at 0cd9c4c and I don't understand how .match-bracket is related to this. For a current git version of zsh zle -la .match-bracket returns 1 for me.

@danielshahaf
Copy link
Member

In case zle-line-pre-redraw is already bound, it is wrapped. In this case z-sy-h still needs to be loaded last, so I left the docs unchanged.

Okay. I mentioned this in workers/37639.

About the use of ${(q)} (https://github.com/zsh-users/zsh-syntax-highlighting/pull/261/files#diff-4ed73021a0fa2ff4e78a9909f4fc2f9bR292):

  • Is zsh-syntax-highlighting.zsh idempotent? Seems to me the code will happily do f() { f; _zsh_highlight } on the second source zsh-syntax-highlighting.zsh call, which will probably run into a recursion limit (or into the stack size limit, whichever comes first).
  • Should there be validation that [[ $currentbinding == user:* ]] there? Just belt and suspenders.

@danielshahaf I'm still using is-at-least for now. I looked at 0cd9c4c and I don't understand how .match-bracket is related to this. For a current git version of zsh zle -la .match-bracket returns 1 for me.

match-bracket is added by zsh-users/zsh@72f7f90 on the mikachu/redrawhook branch in zsh's repository, which the log message of 0cd9c4c references. I was using the presence of that widget as a test for whether the redraw hook is supported by the zsh we run under. (Should've added a comment to that effect.)

@danielshahaf
Copy link
Member

Typo: _zsh_highligh_pre_redraw_wrapper → _zsh_highlight_pre_redraw_wrapper

@m0vie
Copy link
Contributor Author

m0vie commented Mar 29, 2016

You're right, this does not work as intended. I refactored all my branches and I think I now have a solution that fixes all the recent issues we were having.

But I'm starting to get really confused with all those open pull requests and issues that depend on each other so I didn't submit yet another, yet.

I put everything here in a branch:
master...m0vie:p_allzlefixes

Maybe we can talk on IRC in the next few days and to go through it so we can close some of those issues?

@danielshahaf
Copy link
Member

Status update: we spoke on IRC and @m0vie will submit the p_allzlefixes branch as a PR. That PR would (IIUC) not depend on any other outstanding PR, so will be able to be reviewed/merged first.

@m0vie m0vie force-pushed the p_redrawhook branch 2 times, most recently from 860d0c2 to e3b32de Compare April 16, 2016 16:32
@danielshahaf danielshahaf added this to the 0.5.0 milestone May 13, 2016
@danielshahaf
Copy link
Member

danielshahaf commented May 13, 2016 •
edited
Loading

@phy1729 points out that is-at-least 5.2 should become is-at-least 5.3.

I'd like to merge one of the two versions of pre-redraw usage sooner: either just the first and fourth patches out of this PR, or some variant of my version which I linked earlier.

@danielshahaf danielshahaf mentioned this pull request Jun 15, 2016
Closed
@danielshahaf danielshahaf mentioned this pull request Jul 22, 2016
Closed
danielshahaf pushed a commit that referenced this pull request Jul 30, 2016
@m0vie @danielshahaf
driver: Enable highlighting during isearch under zsh≥5.3.
4ad311e 
This patch causes a behaviour difference in the [i257] scenario:

- Before this change, the zle_highlight[isearch] is applied and z-sy-h's
  highlighting isn't.

- With this change, both zle_highlight[isearch] and z-sy-h's
  highlighting are applied, so «echo foo» renders the first word in green
  underline (fg=green from ZSH_HIGHLIGHT_STYLES[builtin], underline from
  zle_highlight[isearch]).

This patch causes the presuppositional FAQ entry added in
a8fe22d to be correct.

This is part of #261, of which #288 was a spin-off.

[i257] #257 (comment)
@m0vie
Copy link
Contributor Author

m0vie commented Aug 24, 2016

Updated the PR. With the new widgets_to_bind logic, this is now much easier.

However the is-at-leastcondition needs to be changed to 5.3 we need to find a better way to decide if the feature is available.

@m0vie
Copy link
Contributor Author

m0vie commented Oct 8, 2016

Closing, since #356 is a better solution.

@m0vie m0vie closed this Oct 8, 2016
@m0vie m0vie deleted the p_redrawhook branch October 8, 2016 17:38
@danielshahaf danielshahaf modified the milestones: 0.5.0, 0.6.0 Oct 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature Upstream
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.

redrawhook issues
2 participants
@m0vie @danielshahaf