From: Dmitry Gutov <dmitry@gutov.dev>
To: Wilhelm Kirschbaum <wkirschbaum@gmail.com>
Cc: Andrey Listopadov <andreyorst@gmail.com>, 67246@debbugs.gnu.org
Subject: bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently
Date: Mon, 20 Nov 2023 03:50:23 +0200 [thread overview]
Message-ID: <c0ca3975-2930-dbcd-ac44-03d511309b8e@gutov.dev> (raw)
In-Reply-To: <CAOS0-34j_6=QYEVokunwM89F_v7xieWjpnkvGf_-yhBa+yj9Yw@mail.gmail.com>
On 18/11/2023 09:50, Wilhelm Kirschbaum wrote:
>
> On Sat, Nov 18, 2023 at 3:36 AM Dmitry Gutov <dmitry@gutov.dev
> <mailto:dmitry@gutov.dev>> wrote:
>
> On 17/11/2023 21:50, Andrey Listopadov wrote:
> >
> > I've upgraded from elixir-mode to elixir-ts-mode and noticed that the
> > latter uses faces rather inconsistently.
> >
> > For example, the =font-lock-keyword-face= is used for both
> keywords and
> > method calls, as well as for parentheses. The
> > =font-lock-function-name-face= is used both for function definitions,
> > parameter names, and calls. Some calls use the
> > =font-lock-keyword-face=, for example the call to `raise'. The
> > =font-lock-type-face= is used both for types and =:symbols=.
> >
> > All of that basically makes Elixir code mostly use 2 colors.
> > Additionally, it makes impossible selectively disabling
> highlighting, as
> > disabling the function call highlighting will disable the function
> > definition highlighitng an so on.
> >
> > I believe, Emacs 29 introduced a lot of faces for all kinds of
> > situations possible in Tree-sitter generated AST, so perhaps the
> fix is
> > to use them more semantically, rather than for good looks.
>
> Thanks for the report. Wilhelm, could you look into this? If you
> have time.
>
> Speaking of new faces, we added font-lock-function-call-face that
> can be
> used for function calls, while font-lock-function-name-face stays used
> on definitions.
>
> For parens, if one wanted to highlight them at all,
> font-lock-bracket-face could be used. Though it's probably better to
> leave them to the 4th feature level (see js-ts-mode as an example).
> elixir-ts-mode currently defines 3 levels.
>
> For levels, we've currently settled on the scheme that function calls
> and property lookups go to the 4th level of highlighting, whereas the
> default is 3. This is all tweakable by the individual user, but I think
> it's better to stay consistent between the modes.
>
> Finally, I see that font-lock-function-name-face ends up being used for
> parameters (as Andrey mentioned) and local variables as well. That's
> not
> great; probably a query that ended up matching too widely. We prefer to
> use font-lock-variable-name-face (for parameter declarations) or
> font-lock-variable-use-face for such identifiers. Though it can be hard
> to reliably distinguish them in a dynamic language, so as far as I'm
> concerned, they could stay non-highlighted (the uses, that is; the
> declarations are usually easy to find using queries).
>
>
> Thanks for the detailed explanation. It's unfortunate timing, because I
> published a rework of the faces on MELPA so long and a few people are
> trying it out. It is a total rework using the faces a bit better. I can
> submit the patch later today and start the conversation from there?
I guess I expected that if the mode has been added to the core then the
development is led here too. And modes maintained externally live more
easily in ELPA. Anyway, we are where we are.
I see that the latest work
(https://github.com/wkirschbaum/elixir-ts-mode/pull/36) anticipated at
least some of my comments.
Remainder:
1) font-lock-variable-name-face is intended for declarations and perhaps
initial assignments (where it's also an implicit declaration), but for
other variable references it's better to use
font-lock-variable-use-face. With my test file, I see examples to the
contrary, even though some attempt to use the latter face had been made
(inside the 'elixir-function-call' feature's query).
2) Moving highlighting of function calls and variable (and/or property)
references to the 4th feature level. Looking at your font-lock rules, it
seems like the elixir-function-call matches is more targeted than the
elixir-variable one, but still the other built-in modes put both at 4,
so it would be good for uniformity. Function definitions (and variable
definitions/declarations, if they're highlighted separately) can remain
in the 'declaration' or 'definition' feature which is put in a lower
feature level (e.g. ruby/js/typescript modes have it on level 1).
Something else I would recommend:
3) Removing the '-face' suffix from the face names. This is the best
practice across most modes, and it's something the manual entry for
'defface' recommends:
You should not quote the symbol face, and it should not end in
‘-face’ (that would be redundant).
Face names inside font-lock.el are a historical exception (and we
followed it when adding new faces recently), but if you search for
'defface' inside the Emacs codebase, such names are in the minority.
I haven't done too much testing myself. Perhaps Andrey will take the
upstream version for a spin. Or we'll wait for the changes to be merged
here and continue.
next prev parent reply other threads:[~2023-11-20 1:50 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-17 19:50 bug#67246: 30.0.50; elixir-ts-mode uses faces inconsistently Andrey Listopadov
2023-11-18 1:36 ` Dmitry Gutov
2023-11-18 7:50 ` Wilhelm Kirschbaum
2023-11-20 1:50 ` Dmitry Gutov [this message]
2023-11-20 10:00 ` Andrey Listopadov
2023-11-24 18:56 ` Wilhelm Kirschbaum
2023-11-24 19:05 ` Dmitry Gutov
2023-11-24 19:23 ` Wilhelm Kirschbaum
2023-11-24 19:30 ` Dmitry Gutov
2023-11-24 19:47 ` Wilhelm Kirschbaum
2023-11-25 0:21 ` Dmitry Gutov
2023-11-25 8:33 ` Andrey Listopadov
2023-11-25 23:26 ` Dmitry Gutov
2023-11-27 17:59 ` Wilhelm Kirschbaum
2023-11-29 3:24 ` Dmitry Gutov
2023-12-03 10:41 ` Andrey Listopadov
2023-12-04 17:50 ` Wilhelm Kirschbaum
2023-12-04 17:46 ` Wilhelm Kirschbaum
2024-01-10 17:47 ` Stefan Kangas
2024-01-13 8:50 ` Wilhelm Kirschbaum
2024-01-29 4:08 ` Dmitry Gutov
2024-01-30 1:59 ` Dmitry Gutov
2024-02-05 17:05 ` Wilhelm Kirschbaum
2024-02-05 17:34 ` Wilhelm Kirschbaum
2024-02-05 17:42 ` Dmitry Gutov
2024-02-05 17:47 ` Wilhelm Kirschbaum
2024-02-05 20:51 ` Dmitry Gutov
2024-02-07 2:21 ` Dmitry Gutov
2024-02-23 15:05 ` Wilhelm Kirschbaum
2024-02-07 2:21 ` Dmitry Gutov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c0ca3975-2930-dbcd-ac44-03d511309b8e@gutov.dev \
--to=dmitry@gutov.dev \
--cc=67246@debbugs.gnu.org \
--cc=andreyorst@gmail.com \
--cc=wkirschbaum@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).