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

Bad whitespace-mode highlighting of in-buffer completions #1488

Closed
basil-conto opened this issue Mar 14, 2018 · 10 comments · May be fixed by #1520

Comments

@basil-conto
Copy link
Collaborator

commented Mar 14, 2018

TL;DR

Indented Elisp variable in-buffer completion overlay gets highlighted with whitespace-trailing face.

Steps to reproduce

GNU Emacs 27.0.50 (build 15, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars) of 2018-03-12
  1. make plain
  2. M-:(setq whitespace-style '(face trailing))RET
  3. M-xwhitespace-modeRET
  4. fC-M-i
    • Non-indented variable completion looks good.
  5. C-gC-a(C-eC-M-i
    • Non-indented function completion looks good.
  6. C-gC-aSPCC-eC-M-i
    • Indented function completion looks good.
  7. C-gC-M-bDELC-eC-M-i
    • Entire indented variable completion overlay gets highlighted with the whitespace-trailing face.

In pictures, all "good" completions:

good

vs indented variable completion:

bad

I'm not sure with which library (Ivy/Whitespace/Elisp-Mode) the issue lies.

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 14, 2018

I just saw the same behaviour in a c-mode buffer. This rules out the Elisp-Mode library, but possibly implicates other completion libraries, such as Minibuffer.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Mar 15, 2018

The issue is likely that of two overlay customizations clashing. This is quite common, overlays are a shared resource, but not as transparent as text; I've had a few clashes between avy and other packages.

It might be possible to fix the issue in ivy-display-function-overlay. Might be hard to do.

@fritzgrabo

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2018

It's not exactly pretty, but advising ivy-completion-in-region to temporarily disable whitespace-mode seems to "fix" this for me:

(defun fg/with-whitespace-mode-disabled (fn &rest args)
  "Call FN with `whitespace-mode' disabled."
  (if (member 'whitespace-mode minor-mode-list)
      (progn
        (whitespace-mode -1)
        (apply fn args)
        (whitespace-mode 1))
    (apply fn args)))

(advice-add 'ivy-completion-in-region :around #'fg/with-whitespace-mode-disabled)

I'd love to dig deeper and come up with a proper fix, but my lisp-fu isn't strong enough yet ;)

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Mar 31, 2018

@fritzgrabo Thanks for the input. This could be one way of doing it, but it's noticeable by user.
I tried playing around with it, and it tends to break whitespace-mode overlays - they're not visible any more, and don't come back when toggling the minor mode.

Maybe we can figure out something better in time. I'm thinking of something like using overlays-at and adjusting them so they don't auto-grow, or making them invisible temporarily.

@fritzgrabo

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2018

@abo-abo Thanks for your suggestion. It motivated me read up on a bunch of things :)

I found out that whitespace-mode only highlights trailing spaces if the buffer-local variable whitespace-point is not at the end of the line (see function whitespace-trailing-regexp).

whitespace-point should be the same as point at all times, but it turns out that it's only set/synced in a function that whitespace-mode adds to post-command-hook. Interactions with ivy's overlay don't cause post-command-hook to run, so whitespace-point and point get out of sync and trailing spaces (including ivy's overlay) get highlighted as a consequence.

A simple (run-hooks 'post-command-hook) added to the end of ivy-display-function-overlay fixes this for me, the overlay is no longer highlighted as trailing whitespace. Alternatively, and less heavy-weight, a direct call to whitespace-post-command-hook (if defined) works as well.

I'm unsure if I want to PR such a change; running command hooks from ivy overlay code seems off? Would you have any thoughts on this?

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Apr 3, 2018

I'm unsure if I want to PR such a change; running command hooks from ivy overlay code seems off? Would you have any thoughts on this?

Yeah, it's a bit off. But when you think about it, it's pretty good that we have the option to do this. If you think of Emacs as an environment, what we're doing here in Unix terms is making sure two processes cooperate with each other /without/ the first process (whitespace-mode) having to change any code. That kind of change is likely to be ugly.

So I suggest you make the PR and we see how ugly the things will get:)

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 3, 2018

what we're doing here in Unix terms is making sure two processes cooperate with each other /without/ the first process (whitespace-mode) having to change any code

Note that, just because Ivy has to work around the current workings of whitespace-mode, doesn't a priori mean that there isn't room for improvement for future versions of whitespace-mode in this regard.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Apr 3, 2018

isn't room for improvement for future versions of whitespace-mode in this regard.

True, improvements to whitespace-mode would be welcome by the core developers. However, Ivy would still need to take care of the legacy whitespace-mode variant, which would be around (almost) forever. Right now, Ivy is compatible with emacs-24.3, released in 2014.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Nov 27, 2018

Thanks, please test.

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 27, 2018

Thanks, that mostly resolves the issue; I'll submit a new issue for a remaining minor annoyance.

basil-conto added a commit to basil-conto/swiper that referenced this issue Nov 27, 2018
ivy-overlay.el: Simplify overlay face overriding
This is a refactor of the following commit:

ivy-overlay.el (ivy-overlay-show-after): Don't inherit existing face
  2018-11-27 12:31:32 +0100 7434a79

(ivy--overlay-font-lock-hack): Remove.
(ivy-overlay-show-after): Specify default face of ivy-overlay-at to
avoid clashing with other overlays.

Re: abo-abo#1016, abo-abo#1488, abo-abo#1520, abo-abo#1547, abo-abo#1808
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.