Skip to content

Conversation

imustafin
Copy link
Contributor

We already support highlight-numbers (by highlighting highlight-numbers-number). This PR uses the same font for font-lock (highlighting font-lock-number-face).

This fixes the inconsistency if you use both highlight-numbers and something which uses font-lock. For example, ruby-mode and ruby-ts-mode. For example, notice how the numbers are not highlighted only in the top-left cell in the following table:

BeforeAfter

image

image

image

image

-----------------

Before submitting a PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • You've added a before/after screenshot illustrating visually your changes.
  • You've updated the readme (if adding/changing configuration options)

Thanks!

@thomasf
Copy link
Collaborator

thomasf commented Jan 24, 2025

This theme aims to be minimalist, I don't think anything that could enable different colors for numbers unless the user opts in to it by enabling a specific mode or customization is a good fit.

Having said that, I just took a quick look at a couple of programming modes and the font-lock-number-face wasn't applied to numbers in either one of them.

In any case, if you really think it's important, add a defcustom that defaults to being off for it.

@thomasf
Copy link
Collaborator

thomasf commented Jan 24, 2025

This is a very similar kind of customization

(defcustom solarized-distinct-doc-face nil
  "Make `font-lock-doc-face' stand out more.
Related discussion: https://github.com/bbatsov/solarized-emacs/issues/158"
  :type 'boolean
  :group 'solarized)

@bbatsov
Copy link
Owner

bbatsov commented Feb 9, 2025

@thomasf Btw, this face is used by all modes using TreeSitter to my knowledge, as is the newer font-lock-variable-usage-face, etc. But whether numbers really need a separate face is a subject for debate and also something everyone can customize easily for themselves.

@imustafin
Copy link
Contributor Author

@thomasf thanks for the suggestion. I moved highlight behind an option.

Treesit-based modes often use font lock and set font-lock-number-face. This includes ruby-ts-mode and js-ts-mode to name a few.

This PR unifies color used for highlight-numbers-number (from highlight-numbers mode) and font-lock-number-face. With this PR both of them will be controlled using one option which is off by default.

I see one problem in my implementation. Those who had highlight-numbers installed will lose the highlight until they enable solarized-highlight-numbers. Is this a big problem? How can we improve this?

@@ -52,6 +52,13 @@ Related discussion: https://github.com/bbatsov/solarized-emacs/issues/158"
:type 'boolean
:group 'solarized)

(defcustom solarized-highlight-numbers nil
Copy link
Owner

Choose a reason for hiding this comment

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

I'd probably name this solarized-font-lock-numbers, so it's more in line with Emacs conventions.

@bbatsov
Copy link
Owner

bbatsov commented Feb 10, 2025

I see one problem in my implementation. Those who had highlight-numbers installed will lose the highlight until they enable solarized-highlight-numbers. Is this a big problem? How can we improve this?

I think that's not a big deal, as with time the highlight-numbers package will become kind of redundant. Just mention this in the README and the docstring as something people need to be aware of.

@thomasf thomasf merged commit 5d13673 into bbatsov:master Feb 22, 2025
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.

3 participants