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

Do not highlight in isearch widgets #257

Closed
wants to merge 1 commit into from
Closed

Conversation

m0vie
Copy link
Contributor

@m0vie m0vie commented Dec 31, 2015

As long as zle_highlight for the isearch region can't be applied
properly after zsh-syntax-highlighting is done it does not make
sense to apply any highlighting while isearch is active.

Otherwise its almost impossible to see the matched area.

As long as zle_highlight for the isearch region can't be applied
properly *after* zsh-syntax-highlighting is done it does not make
sense to apply any highlighting while isearch is active.

Otherwise its almost impossible to see the matched area.
@danielshahaf
Copy link
Member

Can you describe how to reproduce the problem, starting from zsh -f?

Does this depend on some of your other PRs? I guess it might depend on the second part of #255. (and on having defined a zle-isearch-update widget before loading z-sy-h.)

@m0vie
Copy link
Contributor Author

m0vie commented Jan 2, 2016

One example is this:

zsh -f
bindkey -e
source zsh-syntax-highlighting.zsh
echo foo
print bar
vim<^R>

then type echo.

This results in 3 characters in the beginning behing highlighted in green, because vim was 3 characters long and the highlighting was not cleared. When nothing is typed before hitting ^R there is no (visible) issue.

@danielshahaf
Copy link
Member

Thanks, I can reproduce this.

As in #259 (comment): if it's possible to write a test case for this it'd be great, but I realize that might be non-trivial.

As long as zle_highlight for the isearch region can't be applied properly after zsh-syntax-highlighting is done it does not make sense to apply any highlighting while isearch is active.

For future reference, the way to do this would be to mimic the handling of zle_highlight[region] and zle_highlight[paste] in the driver.

I think you can use [[ $KEYMAP == 'isearch' ]] in place of $YANK_ACTIVE and $REGION_ACTIVE. I'm not sure off the top of my head how to get the start and end of the underlined segment.

… and I guess you might want to set zle_highlight+=(isearch:standout) if it's to be applied on top of normal syntax highlighting.

@m0vie
Copy link
Contributor Author

m0vie commented Jan 3, 2016

I think you can use [[ $KEYMAP == 'isearch' ]] in place of $YANK_ACTIVE and $REGION_ACTIVE. I'm not sure off the top of my head how to get the start and end of the underlined segment.

I looked into the zsh source, but sadly the beginning and end of the underlined segment are not exposed in the environment at all.

@m0vie
Copy link
Contributor Author

m0vie commented Jan 3, 2016

As in #259, this patch is also useless if zle-isearch-update is not bound before loading z-s-h.

In a clean zsh -fenvironment z-s-h does not get called at all when entering isearch and while typing in there. So this patch is the wrong approach.

@danielshahaf
Copy link
Member

I looked into the zsh source, but sadly the beginning and end of the underlined segment are not exposed in the environment at all.

Expose them to widgets, then? YANK_ACTIVE was only added in 5.1.1, specifically because z-sy-h needed it...

@danielshahaf
Copy link
Member

As in #259, this patch is also useless if zle-isearch-update is not bound before loading z-s-h.

In a clean zsh -fenvironment z-s-h does not get called at all when entering isearch and while typing in there. So this patch is the wrong approach.

Is there some other thing we can do? For example, could we do if [[ $KEYMAP == isearch ]] ; then /* return without highlighting anything */; fi? (As it's better to highlight nothing than to highlight wrongly)

@danielshahaf
Copy link
Member

I can no longer reproduce this. Could you please provide reproduction instructions again? The instructions above don't reproduce the problem for me with current HEAD, even if I define a zle-isearch-update widget beforehand.

In the reproduction recipe, what widget does ^R invoke? The default keymap of zsh -f depends on $EDITOR, so it's not clear whether the keypress would be interpreted by the emacs or viins keymap. (I tried both but neither reproduced the problem.)

@m0vie
Copy link
Contributor Author

m0vie commented Mar 13, 2016

I can still reproduce the issue using the same instructions.
^R invokes history-incremental-search-backward.

By the way: The fact that zle-isearch-updateis defined does not cause the issue. Instead the fact that this "fix" works, relies on its definition.

@danielshahaf
Copy link
Member

So can I; I missed that I had to type echo after C-r. Sorry for the noise. I'll take the liberty of adding bindkey -e to the instructions.

@danielshahaf
Copy link
Member

Status:

  1. Workaround: define zle-isearch-update before loading z-sy-h.
  2. zsh-5.3 should invoke redrawhook (redrawhook issues #245) from the isearch logic (see treat zle-isearch-exit like an accept-* widget #259 (comment)), which should fix this issue, if we will be able to tell when isearch mode is/isn't active.

@m0vie
Copy link
Contributor Author

m0vie commented Mar 13, 2016

About the workaround for zsh <5.3: If zle-isearch-update is not bound when z-sy-h is loaded, we could simply bind it to _zsh_highlight.

m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Mar 29, 2016
Old version of zsh don't expose ISEARCH_ACTIVE. Therefore we are
unable to re-apply zle_highlight on top and it is impossible to
see the underlined area.

Completely disable highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
@danielshahaf
Copy link
Member

For zsh that includes zsh-users/zsh@cbc44bd (workers/38145, due to be released in zsh-5.3), this was fixed by #283 / 79e4d3d.

A different fix is needed for older zsh.

@m0vie m0vie closed this Apr 2, 2016
@m0vie m0vie mentioned this pull request Apr 2, 2016
m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Apr 2, 2016
Old version of zsh don't expose ISEARCH_ACTIVE. Therefore we are
unable to re-apply zle_highlight on top and it is impossible to
see the underlined area.

Completely disable highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Apr 2, 2016
Old version of zsh don't expose ISEARCH_ACTIVE. Therefore we are
unable to re-apply zle_highlight on top and it is impossible to
see the underlined area.

Completely disable highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Apr 2, 2016
Old version of zsh don't expose ISEARCH_ACTIVE. Therefore we are
unable to re-apply zle_highlight on top and it is impossible to
see the underlined area.

Completely disable highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
@m0vie m0vie deleted the p_isearch branch April 3, 2016 00:14
m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Apr 10, 2016
Old versions of zsh don't expose ISEARCH_ACTIVE. Therefore we
are unable to re-apply zle_highlight on top and it is impossible
to see the underlined area.

Completely disable highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Apr 16, 2016
Old versions of zsh don't expose ISEARCH_ACTIVE. Therefore we
are unable to re-apply zle_highlight on top and it is impossible
to see the underlined area.

Completely disable highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Apr 24, 2016
Therefore we are unable to re-apply zle_highlight on top in
current zsh versions. Therefore it is impossible to see the
underlined matched area.

Completely disable highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Apr 24, 2016
We are unable to re-apply zle_highlight on top in current
zsh versions. Therefore it is impossible to see the
underlined matched area.

Since that information is more important, completely disable
highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Apr 24, 2016
We are unable to re-apply zle_highlight on top in current
zsh versions. Therefore it is impossible to see the
underlined matched area.

Since that information is more important, completely disable
highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Apr 29, 2016
We are unable to re-apply zle_highlight on top in current
zsh versions. Therefore it is impossible to see the
underlined matched area.

Since that information is more important, completely disable
highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Jun 11, 2016
zsh version 5.2 and lower don't support ISEARCHMATCH_ACTIE and
we are unable to re-apply zle_highlight on top. Therefore it is
impossible to see the underlined matched area.

Since that information is more important, completely disable
highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Jun 11, 2016
zsh version 5.2 and lower don't support ISEARCHMATCH_ACTIE and
we are unable to re-apply zle_highlight on top. Therefore it is
impossible to see the underlined matched area.

Since that information is more important, completely disable
highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Jun 20, 2016
zsh version 5.2 and lower don't support ISEARCHMATCH_ACTIE and
we are unable to re-apply zle_highlight on top. Therefore it is
impossible to see the underlined matched area.

Since that information is more important, completely disable
highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
m0vie added a commit to m0vie/zsh-syntax-highlighting that referenced this pull request Jun 20, 2016
zsh version 5.2 and lower don't support ISEARCHMATCH_ACTIE and
we are unable to re-apply zle_highlight on top. Therefore it is
impossible to see the underlined matched area.

Since that information is more important, completely disable
highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
danielshahaf pushed a commit to danielshahaf/zsh-syntax-highlighting that referenced this pull request Jul 27, 2016
zsh version 5.2 and lower don't support ISEARCHMATCH_ACTIE and
we are unable to re-apply zle_highlight on top. Therefore it is
impossible to see the underlined matched area.

Since that information is more important, completely disable
highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
danielshahaf pushed a commit to danielshahaf/zsh-syntax-highlighting that referenced this pull request Jul 27, 2016
zsh version 5.2 and lower don't support ISEARCHMATCH_ACTIE and
we are unable to re-apply zle_highlight on top. Therefore it is
impossible to see the underlined matched area.

Since that information is more important, completely disable
highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.
@danielshahaf danielshahaf mentioned this pull request Jul 29, 2016
danielshahaf pushed a commit to danielshahaf/zsh-syntax-highlighting that referenced this pull request Jul 29, 2016
zsh version 5.2 and lower don't support ISEARCHMATCH_ACTIVE and
we are unable to re-apply zle_highlight on top. Therefore it is
impossible to see the underlined matched area.

Since that information is more important, completely disable
highlighting in isearch in that case.

To do that, we need to make sure we are actually called when
something changes in isearch.

Trumps zsh-users#257.

The FAQ entry presupposes zsh-users#245 will be fixed (in time for the release) too.
danielshahaf pushed a commit that referenced this pull request Jul 30, 2016
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)
danielshahaf added a commit that referenced this pull request Aug 9, 2020
The parent commit, which merged the feature/redrawhook bug and thereby
closed PR #749, also fixed the following issue:

Fixes #40.

Fixes #90, closes #470. (The latter is a PR for the former.)

Fixes #150, closes #151, closes #160. (The latter two are PR's for the first one.) The related issue #183 appears to have been fixed in master. For #150, a different fix for older versions of zsh was considered but has not been implemented.

Issue #154 was fixed in xsel(1) in 2017. The parent commit probably fixed that issue for pre-2017 xsel(1).

Is #245, #356, and #749. Fixes #245 (redrawhook umbrella issue).

Does not reintroduce #257 (comment).

Does not reintroduce #259 (comment)

Closes #281 as obsolete.

Fixes #295.

Fixes #324.

Fixes #375.

Fixes #377.

Closes #421 as obsolete.

Fixes #520.

Unblocks #536 (already milestoned).

Fixes #632.

Unblocks #635. Milestoned.

Unblocks #688. Milestoned.

Unblocks administrative issue #655 (already milestoned).

(The above is copied from
#749 (comment),
but repeated here for the sake of github's commit-to-issue linking magic.)
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