wkirschbaum / elixir-ts-mode

Elixir mode using Treesitter for fontification, navigation and indentation
GNU General Public License v3.0
61 stars 11 forks source link

Changed (hopefully improved) font locks #31

Closed scohen closed 11 months ago

scohen commented 1 year ago

The current font locks relied very heavily on keywords and types for things that weren't keywords or types, which made themes very monochromatic.

This PR does the following:

wkirschbaum commented 1 year ago

Thanks for the PR.

The current font locks relied very heavily on keywords and types for things that weren't keywords or types, which made themes very monochromatic.

This PR does the following:

* Adds a face for elixir atoms

* Changes the @moduledoc tag to look like other module attribues

Why do you want to change this? The entire construct is a doc and there is no need to draw attention to the attribute.

* Defines keyword lists as atoms

Just because it is technically an atom, does not mean we should merge the grammar's distinction into one. I personally like it, because you can distinguish the values from the keys.

* Changes brackets, braces and parens to use the bracket font lock

* Tries to distinguish between function names and calls (i need more help here)

A ts query has to be specified, so might be possible. Why do they have to be different?

scohen commented 1 year ago

Why do you want to change this?

To be consistent with other attributes, and to call out the fact that @moduledoc isn't a doc, it's the marker for a doc, which follows it. I find that having this makes docs stand out a bit more, which is nice, though I'm not going to die on this hill.

Just because it is technically an atom, does not mean we should merge the grammar's distinction into one

I think the core problem here is naming. Elixir and erlang have things called "keyword lists" and the grammar shortens them into keyword. Simultaneously, other languages have things called keywords, which emacs defines as "for a keyword with special syntactic significance, like ‘for’ and ‘if’ in C.", and emacs has a default font-lock with the same name. The emacs font-lock is for painting language-level constructs, which does not describe the keys in a keyword list. Under the hood, keyword keys are atoms, and I think they should be painted as such. Perhaps a better approach would be to make a keyword-key defface that inherits from the atom defface. This way, they would start off looking like atoms, but it would allow users to change their keyword key face if they want. The problem with the current font lock is that it is placing a keyword key, into the same bucket as language-level constructs makes the mode less flexible, and very monochromatic. Also, having them painted the same as atoms (at least by default) will go a long way towards illustrating to new users what keyword lists really are. This is something that new users stumble over a lot.

A ts query has to be specified, so might be possible. Why do they have to be different?

This is something that I've seen in other language modes, and it allows for the definition of a function to be painted differently (often with more bold, eye catching colors) than usage. Consider the screenshot where the definition is in a dark yellow, but the calls are light grey:

Screenshot 2023-10-25 at 8 15 08 AM

I don't see a downside here, the font-lock for function calls inherits from font-lock-function-name-face. If it's as easy as making a tresitter query, why not support it?

scohen commented 1 year ago

As an expression it is a doc.

To be slightly pedantic, @moduledoc is not documentation, the documentation is the string that follows @moduledoc. @moduledoc is a module attribute that's only special because the compiler looks for it. Like I said I don't feel strongly about what it inherits from, since it's got its own face, and I can still affect how it looks in my themes.

scohen commented 1 year ago

So, a slight clarification. Keywords were not set to font-lock-keyword-face, they were set to font-lock-builtin-face. I still think this is incorrect, because the builtin face is supposed to be for builtin functions, and a keyword is neither a function, nor is it built in. We should use the builtin face for things like bifs.

Just to clarify my position, here's what I favor, in order:

  1. Create a elixir-ts-font-keyword face that inherits from elixir-ts-font-atom, paint keywords with it.
  2. Paint keywords as elixir-ts-font-atom
wkirschbaum commented 11 months ago

Closing in favour of https://github.com/wkirschbaum/elixir-ts-mode/pull/36