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

fix font-lock-face for rg-toggle-{on,off}-face #36

Closed
wants to merge 1 commit into from
Closed

fix font-lock-face for rg-toggle-{on,off}-face #36

wants to merge 1 commit into from

Conversation

qwerbzuio
Copy link
Contributor

The faces rg-toggle-{on,off}-face were not processed properly. Apparently only the car of the list assigned to font-lock-face is being considered.

If this is true, also some other settings for font-lock-face won't have the desired effect (i.e. the cdr of the lists get discarded, which mostly means that the 'bold' property is ignored).

The faces rg-toggle-{on,off}-face were not processed properly. Apparently only the car of the list assigned to font-lock-face is being considered.
@coveralls
Copy link

coveralls commented May 2, 2018

Coverage Status

Coverage remained the same at 82.535% when pulling 51dc3b4 on qwerbzuio:master into d50bd10 on dajva:master.

Copy link
Owner

@dajva dajva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What version of emacs are you using?
This works perfectly fine in my setup and according to documentation the font-lock-face property can be a list of faces to apply.
Maybe you don't have the bold face for some reason?
Do you reproduce with emacs -Q?

@qwerbzuio
Copy link
Contributor Author

I'm using version 25.3.1.
But, indeed I didn't check before with emacs -Q. Sorry. The issue indeed disappears then. So I'll have to find out what's wrong with my configuration.
As to the documentation of font-lock-face, I could only find a reference here. No mention of a list is made there. Could you kindly point me to your reference?

@dajva
Copy link
Owner

dajva commented May 2, 2018

That's my reference as well. It says that:

This property specifies a value for the face property that Font Lock mode should apply to the underlying text.

which I interpret as it has the same format as the face property, which could be a list of faces according to the same page. Not super clear but I think it make sense that you can use the same format.

The rg faces inherits from other faces. Wonder if it's something in your config that overrides some parent face possibly. Please let me know if there is some issue with how the faces in this package is defined.

@qwerbzuio
Copy link
Contributor Author

Turns out that my color-theme setup was broken: the face bold had the color set to black. This is the leuven theme, which is probably to blame for the issue, as a face bold shouldn't affect the color (in fact, I had leuven combined with a primary dark color-scheme, which rendered the black on/off-text unreadable).

To make rg.el more robust against such theme-settings, I'd propose to write
- (propertize value 'font-lock-face `(bold ,face))
+ (propertize value 'font-lock-face `((:weight bold) ,face))

But, in principle, it's not rg.el's fault here. I'll close the requrest.

@qwerbzuio qwerbzuio closed this May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants