[m-rev.] for review: color schemes
Peter Wang
novalazy at gmail.com
Mon Jun 3 16:27:46 AEST 2024
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)
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.
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.
And maybe you were right that "lightmode" and "darkmode" would be less
ambiguous names. Change it if you like.
I don't have suggestions for 256 color modes yet.
> Add draft support for color schemes.
>
> compiler/write_error_spec.m:
> Delete the code that fills in the default colors for each role.
>
> compiler/globals.m:
> Move that code here, into the implementation of
> convert_color_spec_options, which is the function that
> write_error_spec.m uses to set up the color database it uses
> by looking up the right options in the option_table.
>
> The main part of this diff is the addition of a new predicate,
> record_color_scheme_in_options, whose job is to *set up* those
> options.
>
> It does so by allowing the user to specify a *color scheme*,
> which may be a builtin scheme (of which we now support four,
> {dark,light}{16,256}), or a scheme that the user specifies directly
> using a string of the form
>
> specified at subject=C:correct=C:incorrect=C:possible_cause:C
>
> where each C is either a color name, or an SGR color number in 0..255.
> (And possibly something like rgb-R-G-B later.)
>
> There is also the pseudo-color-scheme named none, which is not a
> color scheme at all, but rather a way to turn off all use of color.
>
> 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.
> Add a mechanism to avoid getting stuck in an infinite loop
> in the presence of errors in the values of environment variables
> that we use to set up the first globals structure.
>
> compiler/options.m:
> Add the options needed by the new code in handle_options.m.
> Some are not yet documented, the rest are intended never to be documented,
> so there is no corresponding change to doc/user_guidet.texi (yet).
>
> tools/bootcheck:
> Set the MERCURY_COLOR_SCHEME environment variable to specify
> the colors now in .err_exp files.
> @@ -825,6 +859,195 @@ convert_line_number_range(RangeStr, line_number_range(MaybeMin, MaybeMax)) :-
>
> %---------------------------------------------------------------------------%
>
> +record_color_scheme_in_options(Source, SchemeName, Specs, !OptionTable) :-
> + % Any error message we generate cannot use color, because the existence
> + % of such errors would mean that we cannot set up the colors we would
> + % want to use for that. :-(
> + ( if
> + ( SchemeName = "none"
> + ; SchemeName = ""
> + )
> + then
> + map.set(use_color_diagnostics, bool(no), !OptionTable),
> + Specs = []
> + else if
> + % XXX COLOR While we have agreed on the names of the standard schemes,
> + % the colors in those schemes are just placeholders for now.
> + (
> + SchemeName = "dark16",
> + Subject = "0",
> + Correct = "0",
> + Incorrect = "0",
> + Cause = "0"
> + ;
> + SchemeName = "dark256",
> + Subject = "0",
> + Correct = "0",
> + Incorrect = "0",
> + Cause = "0"
> + ;
> + SchemeName = "light16",
> + Subject = "0",
> + Correct = "0",
> + Incorrect = "0",
> + Cause = "0"
> + ;
> + SchemeName = "light256",
> + Subject = "0",
> + Correct = "0",
> + Incorrect = "0",
> + Cause = "0"
> + )
> + then
> + map.set(set_color_subject, string(Subject), !OptionTable),
> + map.set(set_color_correct, string(Correct), !OptionTable),
> + map.set(set_color_incorrect, string(Incorrect), !OptionTable),
> + map.set(set_color_possible_cause, string(Cause), !OptionTable),
> + Specs = []
> + 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.
> + Settings = string.split_at_char(':', SettingsStr),
> + MaybeColorStrs0 = maybe_color_strings(no, no, no, no),
> + parse_color_specifications(Source, Settings,
> + MaybeColorStrs0, MaybeColorStrs, [], SettingSpecs),
> + (
> + SettingSpecs = [],
> + MaybeColorStrs = maybe_color_strings(MaybeSubject, MaybeCorrect,
> + MaybeIncorrect, MaybeCause),
> + ( if
> + MaybeSubject = yes(Subject),
> + MaybeCorrect = yes(Correct),
> + MaybeIncorrect = yes(Incorrect),
> + MaybeCause = yes(Cause)
> + then
> + map.set(set_color_subject, string(Subject), !OptionTable),
> + map.set(set_color_correct, string(Correct), !OptionTable),
> + map.set(set_color_incorrect, string(Incorrect), !OptionTable),
> + map.set(set_color_possible_cause, string(Cause), !OptionTable),
> + Specs = []
> + else
> + (
> + MaybeSubject = no,
> + MissingRoles1 = [words("subject")]
> + ;
> + MaybeSubject = yes(_),
> + MissingRoles1 = []
> + ),
> + (
> + MaybeCorrect = no,
> + MissingRoles2 = MissingRoles1 ++ [words("correct")]
> + ;
> + MaybeCorrect = yes(_),
> + MissingRoles2 = MissingRoles1
> + ),
> + (
> + MaybeIncorrect = no,
> + MissingRoles3 = MissingRoles2 ++ [words("incorrect")]
> + ;
> + MaybeIncorrect = yes(_),
> + MissingRoles3 = MissingRoles2
> + ),
> + (
> + MaybeIncorrect = no,
> + MissingRoles = MissingRoles3 ++ [words("possible cause")]
> + ;
> + MaybeIncorrect = yes(_),
> + MissingRoles = MissingRoles3
> + ),
> + 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.
> +:- 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.
> +:- 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.
> diff --git a/compiler/handle_options.m b/compiler/handle_options.m
> index 1bdcfccc5..ba14b7667 100644
> --- a/compiler/handle_options.m
> +++ b/compiler/handle_options.m
> @@ -210,8 +212,25 @@ convert_option_table_result_to_globals(ProgressStream, DefaultOptionTable,
> quote_list_to_pieces("and", OpModeStrs) ++ [suffix("."), nl],
> add_error(phase_options, OpModePieces, !Specs)
> ),
> - (
> - !.Specs = [],
> + raw_lookup_bool_option(OptionTable, default_globals, DefaultGlobals),
> + % If we are generating the default globals, then executing
> + % the else-part would result in infinite recursion, which
> + % may or may not cause unbounded stack growth that can cause
> + % the machine to thrash itself to death. Ask me how I know :-(
> + %
> + % This problem will arise if even check_option_values returns
> + % errors *even for the default option table*. I (zs) cannot remember
> + % this happening with our usual everthing-turned-off-by-default
everything
> + % option table, but check_option_values now also checks the values
> + % of environment variables, and if they contain some errors, then
> + % retrying with the empty list of command-line arguments won't cure
> + % those errors. (We cannot unset the environment variable causing
> + % the problem, because that won't work on Java.)
> + ( if
> + ( !.Specs = []
> + ; DefaultGlobals = yes
> + )
> + then
> convert_options_to_globals(ProgressStream,
> DefaultOptionTable, OptionTable, OptTuple, OpMode, Target,
> WordSize, GC_Method, TermNorm, Term2Norm,
> @@ -220,8 +239,7 @@ convert_option_table_result_to_globals(ProgressStream, DefaultOptionTable,
> @@ -804,6 +815,83 @@ get_all_obj_extensions(Ext, AllExtA, MaybeAllExtB) :-
> MaybeAllExtB = no
> ).
>
> + % Check whether the color scheme chosen by the user contains
> + % any errors, and records the colors in theoption table
> + % if there are none.
the option
> + %
> + % Note that handle_colors, which is invoked only *after* our caller
> + % check_option_values has finished, will handle the separate question
> + % of whether the use of colors is *enabled*.
> + %
> +:- pred check_color_option_values(option_table::in, option_table::out,
> + list(error_spec)::in, list(error_spec)::out, io::di, io::uo) is det.
> +
> +check_color_option_values(!OptionTable, !Specs, !IO) :-
> + io.environment.get_environment_var("MERCURY_COLOR_SCHEME",
> + MaybeColorEnvVar, !IO),
> + (
> + MaybeColorEnvVar = yes(EnvVarColorScheme),
> + EnvVarSource = [words("the value of the"),
> + quote("MERCURY_COLOR_SCHEME"), words("environment variable")],
> + record_color_scheme_in_options(EnvVarSource, EnvVarColorScheme,
> + EnvVarColorSchemeSpecs, !OptionTable),
> + (
> + EnvVarColorSchemeSpecs = [],
> + EnvVarColorDone = yes
> + ;
> + EnvVarColorSchemeSpecs = [_ | _],
> + raw_lookup_bool_option(!.OptionTable, default_globals,
> + DefaultGlobals),
> + (
> + DefaultGlobals = no,
> + !:Specs = EnvVarColorSchemeSpecs ++ !.Specs,
> + EnvVarColorDone = no
> + ;
> + DefaultGlobals = yes,
> + % Do NOT add EnvVarColorSchemeSpecs to !Specs,
> + % and do not try to set up colors any other way.
> + EnvVarColorDone = yes
> + )
> + )
> + ;
> + MaybeColorEnvVar = no,
> + EnvVarColorDone = no
> + ),
> + (
> + EnvVarColorDone = yes,
> + ColorSchemeDone = yes
> + ;
> + EnvVarColorDone = no,
> + raw_lookup_maybe_string_option(!.OptionTable, color_scheme,
> + MaybeOptionColorScheme),
> + (
> + MaybeOptionColorScheme = yes(OptionColorScheme),
> + OptionSource = [words("the value of the"),
> + quote("--color-scheme"), words("option")],
> + record_color_scheme_in_options(OptionSource, OptionColorScheme,
> + ColorSchemeSpecs, !OptionTable),
> + !:Specs = ColorSchemeSpecs ++ !.Specs,
> + ColorSchemeDone = yes
> + ;
> + MaybeOptionColorScheme = no,
> + ColorSchemeDone = no
> + )
> + ),
> + (
> + ColorSchemeDone = yes
> + ;
> + ColorSchemeDone = no,
> + MaybeColorSpecs = convert_color_spec_options(!.OptionTable),
> + (
> + MaybeColorSpecs = ok1(_)
> + ;
> + MaybeColorSpecs = error1(ColorSpecs),
> + !:Specs = ColorSpecs ++ !.Specs
> + )
> + ).
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').
3. The --set-color-* options override choices for individual roles
made by the color scheme.
4. Enable color diagnostics if at least one role has a color assigned.
Is there any reason to keep the --set-color-* options?
Peter
-------------- next part --------------
types.m:013: Error: predicate [38;5;14m`p'/1[39;49m [38;5;9mhas no clauses.[39;49m
types.m:019: In clause for predicate `q'/0:
types.m:019: error: call to [38;5;9mundefined[39;49m predicate [38;5;14m`zzzzzzzz'/0.[39;49m
types.m:020: In clause for predicate `q'/0:
types.m:020: in argument 1 of call to predicate `types.p'/1:
types.m:020: type error: [38;5;14margument[39;49m has type
types.m:020: [38;5;9mint;[39;49m
types.m:020: expected type was
types.m:020: [38;5;10mpred.[39;49m
types.m:020: The partial type assignment was:
types.m:020: V_1: (pred)
types.m:020: V_2: int
types.m:021: In clause for predicate `q'/0:
types.m:021: error: [38;5;9mwrong number of arguments[39;49m ([38;5;9m0[39;49m; should be [38;5;10m1[39;49m)
types.m:021: in call to predicate [38;5;14m`p'.[39;49m
types.m:023: Error: clause for predicate [38;5;14m`r'/0[39;49m [38;5;9mwithout[39;49m a corresponding
types.m:023: `:- pred' declaration.
types.m:023: Warning: [38;5;9mnon-contiguous clauses[39;49m for predicate [38;5;14m`r'/0.[39;49m
types.m:023: Gap in clauses of predicate `r'/0 starts after this clause.
types.m:029: Gap in clauses of predicate `r'/0 ends with this clause.
types.m:024: In clause for predicate `r'/0:
types.m:024: error: call to [38;5;9mundefined[39;49m predicate [38;5;14m`s'/0.[39;49m
types.m:024: [38;5;11m(Did you mean `<', `>', `a', `p', `q', `r' or `z'?)[39;49m
types.m:026: Error: clause for predicate [38;5;14m`a'/1[39;49m [38;5;9mwithout[39;49m a corresponding
types.m:026: `:- pred' declaration.
types.m:027: In clause for predicate `a'/1:
types.m:027: error: call to [38;5;9mundefined[39;49m predicate [38;5;14m`b'/1.[39;49m
types.m:027: [38;5;11m(Did you mean `<', `>', `a', `p', `q', `r' or `z'?)[39;49m
types.m:046: In clause for predicate `bar'/1:
types.m:046: type error in unification of variable `X'
types.m:046: and constant `0'.
types.m:046: [38;5;14mVariable `X'[39;49m has type
types.m:046: [38;5;9msome [BarTypeParam] (BarTypeParam),[39;49m
types.m:046: [38;5;14mconstant `0'[39;49m has type
types.m:046: [38;5;10mint.[39;49m
types.m:046: The partial type assignment was:
types.m:046: some [BarTypeParam_1]
types.m:046: X_2: BarTypeParam
types.m:053: Error: [38;5;14mabstract declaration for type `t'/2[39;49m [38;5;9mhas no corresponding[39;49m
types.m:053: [38;5;9mdefinition.[39;49m
types.m:053: Error: the type [38;5;14m`t'/2[39;49m has this declaration, but [38;5;9mit has no[39;49m
types.m:053: [38;5;9mdefinition.[39;49m
types.m:055: Error: predicate [38;5;14m`bar2'/1[39;49m [38;5;9mhas no clauses.[39;49m
types.m:072: In clause for predicate `repeated_arity'/3:
types.m:072: error: [38;5;9mwrong number of arguments[39;49m ([38;5;9m2[39;49m; should be [38;5;10m3[39;49m)
types.m:072: in call to predicate [38;5;14m`append'.[39;49m
-------------- next part --------------
types.m:013: Error: predicate [38;5;6m`p'/1[39;49m [38;5;9mhas no clauses.[39;49m
types.m:019: In clause for predicate `q'/0:
types.m:019: error: call to [38;5;9mundefined[39;49m predicate [38;5;6m`zzzzzzzz'/0.[39;49m
types.m:020: In clause for predicate `q'/0:
types.m:020: in argument 1 of call to predicate `types.p'/1:
types.m:020: type error: [38;5;6margument[39;49m has type
types.m:020: [38;5;9mint;[39;49m
types.m:020: expected type was
types.m:020: [38;5;2mpred.[39;49m
types.m:020: The partial type assignment was:
types.m:020: V_1: (pred)
types.m:020: V_2: int
types.m:021: In clause for predicate `q'/0:
types.m:021: error: [38;5;9mwrong number of arguments[39;49m ([38;5;9m0[39;49m; should be [38;5;2m1[39;49m)
types.m:021: in call to predicate [38;5;6m`p'.[39;49m
types.m:023: Error: clause for predicate [38;5;6m`r'/0[39;49m [38;5;9mwithout[39;49m a corresponding
types.m:023: `:- pred' declaration.
types.m:023: Warning: [38;5;9mnon-contiguous clauses[39;49m for predicate [38;5;6m`r'/0.[39;49m
types.m:023: Gap in clauses of predicate `r'/0 starts after this clause.
types.m:029: Gap in clauses of predicate `r'/0 ends with this clause.
types.m:024: In clause for predicate `r'/0:
types.m:024: error: call to [38;5;9mundefined[39;49m predicate [38;5;6m`s'/0.[39;49m
types.m:024: [38;5;3m(Did you mean `<', `>', `a', `p', `q', `r' or `z'?)[39;49m
types.m:026: Error: clause for predicate [38;5;6m`a'/1[39;49m [38;5;9mwithout[39;49m a corresponding
types.m:026: `:- pred' declaration.
types.m:027: In clause for predicate `a'/1:
types.m:027: error: call to [38;5;9mundefined[39;49m predicate [38;5;6m`b'/1.[39;49m
types.m:027: [38;5;3m(Did you mean `<', `>', `a', `p', `q', `r' or `z'?)[39;49m
types.m:046: In clause for predicate `bar'/1:
types.m:046: type error in unification of variable `X'
types.m:046: and constant `0'.
types.m:046: [38;5;6mVariable `X'[39;49m has type
types.m:046: [38;5;9msome [BarTypeParam] (BarTypeParam),[39;49m
types.m:046: [38;5;6mconstant `0'[39;49m has type
types.m:046: [38;5;2mint.[39;49m
types.m:046: The partial type assignment was:
types.m:046: some [BarTypeParam_1]
types.m:046: X_2: BarTypeParam
types.m:053: Error: [38;5;6mabstract declaration for type `t'/2[39;49m [38;5;9mhas no corresponding[39;49m
types.m:053: [38;5;9mdefinition.[39;49m
types.m:053: Error: the type [38;5;6m`t'/2[39;49m has this declaration, but [38;5;9mit has no[39;49m
types.m:053: [38;5;9mdefinition.[39;49m
types.m:055: Error: predicate [38;5;6m`bar2'/1[39;49m [38;5;9mhas no clauses.[39;49m
types.m:072: In clause for predicate `repeated_arity'/3:
types.m:072: error: [38;5;9mwrong number of arguments[39;49m ([38;5;9m2[39;49m; should be [38;5;2m3[39;49m)
types.m:072: in call to predicate [38;5;6m`append'.[39;49m
-------------- next part --------------
A non-text attachment was scrubbed...
Name: b-on-w-dark16.gif
Type: image/gif
Size: 80024 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20240603/67b7dc64/attachment-0004.gif>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: b-on-w-light16.gif
Type: image/gif
Size: 81241 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20240603/67b7dc64/attachment-0005.gif>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w-on-b-dark16.gif
Type: image/gif
Size: 80886 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20240603/67b7dc64/attachment-0006.gif>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w-on-b-light16.gif
Type: image/gif
Size: 80220 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20240603/67b7dc64/attachment-0007.gif>
More information about the reviews
mailing list