[m-rev.] for review: color schemes

[Zoltan Somogyi]

On 2024-06-03 16:27 +10:00 AEST, "Peter Wang" <novalazy at gmail.com> wrote:
> On Mon, 03 Jun 2024 01:31:15 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
>> This diff implements the rough consensus we reached about
>> a month ago on m-rev on how colors should be controlled,
>> or at least my understanding of it. For review, preferably,
>> by everyone, not just by one person.
>> 
>> Peter, would you like to pick the colors to use for the
>> four builtin color schemes, {dark,light}{16,256}? And do you
>> have a proposed set of default colors, for use when the user
>> does not specify a color scheme, and therefore we don't know
>> what color background his/her terminal has?
> 
> How about:
> 
> light16
>     subject=6           (normal cyan)
>     correct=2           (normal green)
>     incorrect=9         (bright red)
>     possible_cause=8    (normal yellow)
> 
> dark16
>     subject=14          (bright cyan)
>     correct=10          (bright green)
>     incorrect=9         (bright red)
>     possible_cause=11   (bright yellow - only if not default)

What does "only if not default" mean here?

> light16 looks acceptable to me on both light and dark backgrounds,
> so I'll suggest light16 as the default color scheme, if there is to be
> one.

Agreed.

> I'm attaching a screenshots of those two color schemes using two
> xfce-terminal presets (just because it has a drop-down for the color
> palette). I tried other presets as well, but I don't want to send
> too many images to the mailing list.

I like three of those, but agree that dark16 does not look good
on a white background. Since it is not intended for that background, that is ok.

> And maybe you were right that "lightmode" and "darkmode" would be less
> ambiguous names. Change it if you like.

I will add them as synonyms.

> I don't have suggestions for 256 color modes yet.

Understandable, since that is a much bigger space to search.
I will just use the 16 versions for now, until you have some.

>> compiler/handle_options.m:
>>     Add code to allow the user to specify a color scheme using either
>> 
>>     - a new maybe_string option named --color-scheme, or
>>     - an environment variable named MERCURY_COLOR_SCHEME.
>> 
>>     Also add code to allow an environment variable named MERCURY_NO_COLOR
>>     to turn off the use of color regardless of any other color settings.
>> 
> 
> MERCURY_NO_COLOR doesn't appear in the diff. There should be no need
> as we have NO_COLOR and MERCURY_COLOR_SCHEME=none.

I was using MERCURY_NO_COLOR in the code as well, until I remembered
that you said that NO_COLOR was used by other programs as well.
I changed the code, but forgot to change the log message. Done it now.

>> +    else if
>> +        string.remove_prefix("specified@", SchemeName, SettingsStr)
>> +    then
> 
> Is the "specified@" prefix necessary? We can assume that builtin scheme
> names will never contain a '=', for example.

It is indeed not necessary NOW. The reason I added it is to allow us
to add other syntaxes for specifying color schemes more easily.
With a prefix, we can add another syntax by adding another prefix,
and have the prefix select which syntax to parse for. Without a prefix
on this scheme, that wouldn't be possible, and we would need
more ad-hoc mechanism to effectively guess which syntax the user
intended, especially if the string they wrote had a syntax error.

>> +                ColorColors = choose_number(MissingRoles, "color", "colors"),
>> +                RoleRoles = choose_number(MissingRoles, "role", "roles"),
>> +                Pieces = [words("Error: the value of")] ++ Source ++
>> +                    [words("does not specify the"), words(ColorColors),
>> +                    words("to use for the"), words(RoleRoles), words("of")] ++
>> +                    piece_list_to_pieces("and", MissingRoles) ++
>> +                    [suffix("."), nl],
>> +                Specs = [no_ctxt_spec($pred, severity_error, phase_options,
>> +                    Pieces)]
>> +            )
> 
> If a role is not specified, it can just be left uncolored instead
> instead of reporting an error.

Are you saying that because you think that is a useful capability,
or just out of general concerns about the length of the string
required to specify all four colors?

At the moment, if the use of color is enabled, then all four colors must
be specified. That can be changed, but that would itself raise some questions.
The one I am thinking of is color nesting. At the moment, none of our
diagnostics generates piece lists that do this:

abc
start subject color
def
start incorrect color
ghi
end incorrect color
jkl
end subject color
mno

but the code of write_error_spec.m does have code to handle that,
and that machinery works. I know, because I have created diagnostics
with such nesting in the past, admittedly by accident, and it worked.

If the user specified a subject color but not an incorrect color,  what
color should ghi be in the above example? Should it be the neutral
color, making ghi match abc and mno, or should it be the subject color,
making ghi match def and jkl? Neither alternative seems right,
though that would matter only if we *did* have real diagnostics
with nesting. The reason I ask is that I can see two different ways
to implement the "allow no color to be specified for a role" change,
and those give different answers to that question.  It definitely isn't
necessary, but it would be nice if the approach I pick now would
work in a future where we start using color nesting. Either that,
or decide now to disallow nesting now and forever.

The latter choice may seem appealing, but there is an argument
against it, which is the argument that made me add that support
in the first place. That argument is this.

- We add color by invoking a function that adds start color/end color
  piece pairs around a list of format pieces.

- It is obviously possible to pass a format piece list to those functions
  that already contains start color/end color pieces. This guarantees
  that some nesting *will* occur in buggy code that does not pay
  attention to nesting. Indeed, this *has* happened several times,
  usually when I was changing what was colored and what wasn't.

- There are some diagnostics that our test suite has no test for.

- If some user encounters a diagnostic that (a) does color nesting, and
  (b) the test suite does not test for, I would prefer that they get
  their diagnostic, even if it is colored incorrectly, rather than get
  a compiler abort from the code that detects the nesting and
  says "nope".

>> +:- type maybe_color_strings
>> +    --->    maybe_color_strings(
>> +                mcs_subject             ::  maybe(string),
>> +                mcs_correct             ::  maybe(string),
>> +                mcs_incorrect           ::  maybe(string),
>> +                mcs_possible_cause      ::  maybe(string)
>> +            ).
>> +
>> +:- pred parse_color_specifications(list(format_piece)::in, list(string)::in,
>> +    maybe_color_strings::in, maybe_color_strings::out,
>> +    list(error_spec)::in, list(error_spec)::out) is det.
>> +
>> +parse_color_specifications(_, [], !MaybeColorStrs, !Specs).
>> +parse_color_specifications(Source, [Setting | Settings],
>> +        !MaybeColorStrs, !Specs) :-
>> +    ( if
>> +        [Name, Value] = string.split_at_char('=', Setting),
>> +        ( Name = "subject"
>> +        ; Name = "correct"
>> +        ; Name = "incorrect"
>> +        ; Name = "possible_cause"
>> +        )
> 
> possible_cause is a bit long. Ideally, it would be another single word,
> say "possible", "maybe", "suggestion", "hint"? None of those is likely
> to be a perfect match, but one of them may be close enough.

At the moment, we use that color for two purposes, which may be
distinct enough that maybe they should be different color names,
which may, or may not, be assigned different shades.

One purpose is add additional info about an error, info that tells the user
about the usual cause of the error. For this use, the word "hint" is a perfect
description.

The other purpose occurs when we have an error that reports not that
some part of the user's code is definitely wrong, but that two parts of
the user's code are inconsistent with one another. In that case, one
of those pieces of code is wrong, but we don't which one, so we
can't color one correct and one incorrect, so we color both with possible_cause.
For this use, the word "possible" would work, as a shorthand for "possibly incorrect".
"inconsistent" would also work.

Should these roles be split? I am fine with either answer.
I do note that we would need a fifth color for each color scheme,
and in the 16 versions, there are not that many left to choose from.

>> +:- type is_color_result
>> +    --->    is_color(color_spec)
>> +    ;       not_color_int_outside_range(int, int)
>> +            % The range the int is outside of, both inclusive.
>> +    ;       not_color_unknown_format.
>> +
>> +:- func is_string_a_color_spec(string) = is_color_result.
>> +
>> +is_string_a_color_spec(Str) = Result :-
>> +    % XXX COLOR
>> +    % After we test for 8-bit colors, we can test for 24-bit colors as well,
>> +    % looking for a format such as as "rgb-R-G-B", where each of R, G and B
>> +    % would be integers between 0 and 255.
> 
> The most common syntax for 24-bit colors is #RRGGBB (originating from
> HTML or CSS, I believe), where R,G,B are hexadecimal digits.

That works for me.

> I think this is how things should work:
> 
>  0. If NO_COLOR is set, color is disabled; ignore the rest.
> 
>  1. --color-scheme takes precedence over MERCURY_COLOR_SCHEME.
> 
>  2. If neither --color-scheme or MERCURY_COLOR_SCHEME is set,
>     use a default color scheme (that could be 'none').

The default scheme should not be none: there should be a meaningful default
color scheme. However, the default could be that the use of color is *not enabled*
by default. (Julien argued for the ability of  e.g. Linux distributions to decide
whether color is enabled by default, and the code I wrote them implements that.)

The difference lies in what happens when a user adds --enable-color-diagnostics
to an mmc command line. With your proposal, they get the default color scheme
in mmc, which they had no hand in choosing. With Julien's proposal, they get
the color scheme in MERCURY_COLOR_SCHEME, which they (probably) *did* choose.

I don't have a strong opinion on which of MERCURY_COLOR_SCHEME and --color-scheme
should win if both are specified. I have no objection to switching priority to the
option, provided Julien does not object. I gave priority to the envvar in my draft code
because I thought it quite unlikely that anyone would type in the long argument
need after --color-scheme on the command line, which means that any such option
will almost certainly come from a Mmakefile or Mercury.options file. Since such
files are shared among several people, the envvar is more personal than that.
I though that the personal should override the shared, not the other way around.

>  3. The --set-color-* options override choices for individual roles
>     made by the color scheme.

I am not arguing for making this possible, and it seems neither are you, probably
for the same reason: specifying four (or five) options is harder than using --color-scheme.
But see below.

>  4. Enable color diagnostics if at least one role has a color assigned.

And if NO_COLOR was not present.

Actually, the code in handle_options.m makes its decisions in the opposite
order from your pseudocode: it decides on the color scheme, and *then*
decides whether color is enabled at all. It does this due to Julien's suggestion
a month or so ago that I mentioned above.

> Is there any reason to keep the --set-color-* options?

We have to keep them, because that is the only way that write_error_spec.m
has to access the chosen color scheme. However, we don't have to let users know
about them. We don't even need them to be specifiable on the command line;
the system should work without that. If we made them unspecifiable in that way,
then obviously they wouldn't be an input to the color selection process;
they would just record its output.

I made the other changes you pointed out. Thanks for the review.

Zoltan.