emacs-helm / helm

Emacs incremental completion and selection narrowing framework
https://emacs-helm.github.io/helm/
GNU General Public License v3.0
3.36k stars 389 forks source link

global-rainbow-delimiters-mode messes up the colors of helm #627

Closed unbalancedparentheses closed 9 years ago

unbalancedparentheses commented 9 years ago

For some reason (global-rainbow-delimiters-mode 1) is messing the colors of helm: 2014-09-19-154155_1920x1080_scrot

With (global-rainbow-delimiters-mode 1) 2014-09-19-154350_1920x1080_scrot

Any idea on why this might be happening?

Fanael commented 9 years ago

See jlr/rainbow-delimiters#33.

thierryvolpiatto commented 9 years ago

This looks unrelated to helm.

Fanael commented 9 years ago

It's not. It's caused by helm using face text property rather than font-lock-face (or both).

unbalancedparentheses commented 9 years ago

@thierryvolpiatto please check https://github.com/jlr/rainbow-delimiters/issues/33 and you will se that it is related with helm.

unbalancedparentheses commented 9 years ago

@thierryvolpiatto could you please check it? It is helm fault as described by @Fanael. Sorry for disturbing you.

thierryvolpiatto commented 9 years ago

Federico Carrone notifications@github.com writes:

@thierryvolpiatto could you please check it?

Please submit a bug report to the rainbow developer, I have no idea what this package does and what is the problem with helm and why helm is involved into this.

It is helm fault as described by @Fanael.

If there is really a problem with helm provide a complete recipe on how to reproduce the problem and what is wrong exactly.

Thanks.

Thierry Get my Gnupg key: gpg --keyserver pgp.mit.edu --recv-keys 59F29997

Fanael commented 9 years ago

I have no idea […] what is the problem with helm

As I already said, the problem is helm using face text property to highlight text instead of font-lock-face (or, possibly, both face and font-lock-face). If helm is changed to use that instead everywhere where text properties (not overlays) are used, the problem will go away.

lewang commented 9 years ago

@Fanael Are there compatibility considerations with this change?

michael-heerdegen commented 9 years ago

I see the problem, and I believe @Fanael that what he suggests would fix this problem. But @Fanael why do you consider this a bug in helm? Using the face property is totally legitime, especially in situations where font-lock-mode is not used to fontify the buffer, as in our case. And I don't see why rainbow-delimiters-mode must destroy the lookout of the whole buffer, instead of just highlighting the parens?

Fanael commented 9 years ago

@Fanael Are there compatibility considerations with this change?

If both font-lock-face and face are used, then none that I'm aware of. If only font-lock-face is used, the minor mode font-lock-mode must be active in the buffer. global-font-lock-mode is the default since ancient Rome, though.

And I don't see why rainbow-delimiters-mode must destroy the lookout of the whole buffer, instead of just highlighting the parens?

It's not rainbow-delimiters-mode, it's font-lock-mode. As such, pretty much any minor mode that uses font-lock-mode will break the highlighting, because it has to tell font-lock-mode to invalidate the existing highlighting and re-highlight everything, and then font-lock-mode removes whatever face properties there were and overwrites them with their own. You can see for yourself by calling font-lock-flush (or font-lock-fontify-buffer if your Emacs doesn't have font-lock-flush) in a Helm buffer.

Pretty much the only reason why it's rainbow delimiters what exposes the problem is because global-rainbow-delimiters-mode exists and some people mistakenly believe it's a good idea to use it, instead of turning on rainbow-delimiters-mode only where it actually makes sense to do so. Most other highlighting-related minor modes don't have a globalized version, preventing users from hurting themselves by enabling them where they don't belong.

tuhdo commented 9 years ago

@unbalancedparentheses Just use rainbow-delimiters with prog-mode-hook. I had this problem and then I realized it's better that way.

michael-heerdegen commented 9 years ago

@Fanael I this issue still pertinent? Because I see that global-rainbow-delimiters-mode has been obsoleted?

Anyway, the behavior seems to depend on font-lock-fontify-buffer-function. In the helm buffer, this is font-lock-default-fontify-buffer, and calling that leads to a call to font-lock-unfontify-region (in font-lock-default-fontify-region) that unfontifies the complete buffer. That's our culprit.

michael-heerdegen commented 9 years ago

I asked how this should be handled at emacs-help. Hopefully, some Emacs maintainer is answering. It's a general problem, and I wonder which package is doing something "forbidden".

michael-heerdegen commented 9 years ago

Drew Adams reminded me of a related discussion in emacs-dev about the general issue of font-lock erasing face text properties:

http://lists.gnu.org/archive/html/emacs-devel/2014-08/msg00540.html

Fanael commented 9 years ago

I this issue still pertinent? Because I see that global-rainbow-delimiters-mode has been obsoleted?

As I said, anything requesting font-lock-mode to re-highlight will break the highlighting. You can see it by switching to a helm buffer and calling font-lock-flush or font-lock-fontify-buffer. I wouldn't be surprised if it turned out there are other minor modes that expose this issue, but are nowhere as popular as rainbow-delimiters is, so the chances of somebody actually reporting the issue are much lower.

michael-heerdegen commented 9 years ago

Fanael Linithien notifications@github.com writes:

As I said, anything requesting font-lock-mode to re-highlight will break the highlighting. You can see it by switching to a helm buffer and calling font-lock-flush or font-lock-fontify-buffer.

I meant, is it still relevant for using global-rainbow-delimiters-mode and helm together, now that global-rainbow-delimiters-mode has been obsoleted?

BTW, just font-lock-flush doesn't show the problem for me, although it is defined. font-lock-fontify-buffer does OTOH.

I wouldn't be surprised if it turned out there are other minor modes that expose this issue, but are nowhere as popular as rainbow-delimiters is, so the chances of somebody actually reporting the issue are much lower.

Yes, that could happen, of course.

But the problem is more complex than just Helm. The issue could happen for any buffer that uses the face text property. We use face in Helm, mainly because we don't use font-lock-mode in any regard in the completions buffer, I guess. Is that an error? Is using face generally forbidden to use because of this problem? If it is, the elisp manual should say that it's discouraged to be used in a buffer. I don't know, that's why I asked at help-emacs. If what we currently do is wrong, I guess Thierry is willing to do what you suggest.

Fanael commented 9 years ago

I meant, is it still relevant for using global-rainbow-delimiters-mode and helm together, now that global-rainbow-delimiters-mode has been obsoleted?

No, for global-rainbow-delimiters-mode will be removed soon.

But the problem is more complex than just Helm. The issue could happen for any buffer that uses the face text property. We use face in Helm, mainly because we don't use font-lock-mode in any regard in the completions buffer, I guess. Is that an error? Is using face generally forbidden to use because of this problem? If it is, the elisp manual should say that it's discouraged to be used in a buffer.

I dunno, https://www.gnu.org/software/emacs/manual/html_node/elisp/Precalculated-Fontification.html is pretty clear to me: if you don't want font-lock-mode to screw up your highlighting, use font-lock-face text property, and if you don't, don't be surprised when font-lock-mode screws it up.

michael-heerdegen commented 9 years ago

Fanael Linithien notifications@github.com writes:

https://www.gnu.org/software/emacs/manual/html_node/elisp/Precalculated-Fontification.html is pretty clear to me: if you don't want font-lock-mode to screw up your highlighting, use font-lock-face text property.

Thanks for the pointer. I don't find it clear in that regard.

Mmh, other Emacs packages, e.g. buffer-menu.el, seem to do what you suggest, even when they don't use font-lock-mode by themselves. So I tend to agree that it could be TRT.

michael-heerdegen commented 9 years ago

@thierryvolpiatto What do you think now about this issue?

The problem is that any code (e.g. minor-mode) that uses font locking in the completions buffer will destroy our highlighting.

What @Fanael suggested (use font-lock-face instead of face) seems quite reasonable to me. What we currently have doesn't work correctly with some valid configurations, so this is a helm issue.

thierryvolpiatto commented 9 years ago

Michael Heerdegen notifications@github.com writes:

@thierryvolpiatto What do you think now about this issue?

I wonder why people are enabling such a mode in an helm buffer. I will have to modify all occurences of 'face by font-lock-face, modifying something that work very well just for few people using an unuseful mode (I mean unuseful in a helm buffer), thus introducing possible bugs.

The problem is that any code (e.g. minor-mode) that uses font locking in the completions buffer will destroy our highlighting.

I think these modes should have a variable to not enable the mode in specific buffers (e.g like golden-ratio)

What @Fanael suggested (use font-lock-face instead of face) seems quite reasonable to me. What we currently have doesn't work correctly with some valid configurations, so this is a helm issue.

I don't agree, sorry.

Thierry Get my Gnupg key: gpg --keyserver pgp.mit.edu --recv-keys 59F29997

michael-heerdegen commented 9 years ago

Thierry Volpiatto notifications@github.com writes:

I wonder why people are enabling such a mode in an helm buffer.

It's just a globalized version of a minor mode - a useful one.

The problem is that any code (e.g. minor-mode) that uses font locking in the completions buffer will destroy our highlighting.

I think these modes should have a variable to not enable the mode in specific buffers (e.g like golden-ratio)

How should such a mode know which specific buffers do handle face text properties not correctly?

What @Fanael suggested (use font-lock-face instead of face) seems quite reasonable to me. What we currently have doesn't work correctly with some valid configurations, so this is a helm issue.

I don't agree, sorry.

Too bad, I really think what we have is wrong.