Allan Caffee <allan.caffee@xxxxxxxxx> writes:
> On Thu, Mar 19, 2009 at 5:48 PM, Nanako Shiraishi <nanako3@xxxxxxxxxxx> wrote:
>> Quoting Johannes Schindelin <Johannes.Schindelin@xxxxxx>:
>>
>>> I'd start like this:
>>>
>>> enum color_name {
>>> COLOR_RESET,
>>> COLOR_RED,
>>> COLOR_GREEN,
>>> COLOR_YELLOW,
>>> COLOR_BLUE,
>>> COLOR_MAGENTA,
>>> COLOR_CYAN,
>>> COLOR_WHITE
>>> };
>>
>> Looking for "COLOR_RED" in the archive gives:
>>
>> http://article.gmane.org/gmane.comp.version-control.git/109676
>>
>
> Duly noted. Perhaps those #defines should be relocated to color.h?
Heh, I did not even realize the above 109676 was referring to what I wrote
sometime ago.
> If we still wanted to provide a color_name type we could use
> GIT_COLOR_NAME_RESET et al. That would give us something like:
>
> #define GIT_COLOR_NORMAL ""
> #define GIT_COLOR_RESET "\033[m"
> #define GIT_COLOR_BOLD "\033[1m"
> #define GIT_COLOR_RED "\033[31m"
> #define GIT_COLOR_GREEN "\033[32m"
> #define GIT_COLOR_YELLOW "\033[33m"
> #define GIT_COLOR_BLUE "\033[34m"
> #define GIT_COLOR_CYAN "\033[36m"
> #define GIT_COLOR_BG_RED "\033[41m"
>
> enum color_name {
> GIT_COLOR_NAME_NORMAL
> GIT_COLOR_NAME_RESET,
> GIT_COLOR_NAME_RED,
> GIT_COLOR_NAME_GREEN,
> GIT_COLOR_NAME_YELLOW,
> GIT_COLOR_NAME_BLUE,
> GIT_COLOR_NAME_MAGENTA,
> GIT_COLOR_NAME_CYAN,
> GIT_COLOR_NAME_WHITE
> GIT_COLOR_NAME_BG_RED
> };
>
> /*
> * Map names to ANSI escape sequences. Consider putting this in color.c
> * and providing color_name_get_ansi_code(enum color_name).
> */
> const char* git_color_codes[] {
> GIT_COLOR_RESET,
> GIT_COLOR_BOLD,
> GIT_COLOR_RED,
> GIT_COLOR_GREEN,
> GIT_COLOR_YELLOW,
> GIT_COLOR_BLUE,
> GIT_COLOR_CYAN,
> GIT_COLOR_BG_RED,
> };
>
> That conveniently offers clients access to both the raw escape codes and
> a clear type for storing/handling colors.
Is git_color_codes[GIT_COLOR_NAME_FOO] supposed to give you GIT_COLOR_FOO?
Are you consolidating various pieces of physical color definition to one
place? That sounds sensible.
The corrent code does:
diff.c::
user says "meta" is "purple"
-> parse_diff_color_slot() says "meta" is slot 2
-> git_diff_basic_config() asks color_parse() to place the ANSI
representation of the "purple" in slot 2
-> code uses diff_get_color() to grab "meta" color from the slot
and sends it to the terminal
builtin-branch.c duplicates the exact same logic with a separate tables
and a set of slots. builtin-grep.c cheats and does not give the end user
any customizability, which needs to be fixed.
The "slots" are defined in terms of what the color is used for, the
meaning, e.g. "a line from the file before the change (DIFF_FILE_OLD)"; we
cannot avoid having application specific set of slots, but the parsing
should be able to share the code.
Once the slot number is known, we ask color_parse() to put the final
physical string (suitable for the terminal's consumption) to fill the
slot. But for that, I do not think git_color_codes[] nor GIT_COLOR_FOO
need to be exposed to the applications (i.e. "diff", "branch", "grep").
It is an implementation detail that color_parse() always uses ANSI escape
sequences right now, but we could encapsulate that in color.c and later
perhaps start looking up from the terminfo database, for example.
But that leaves the question of initialization. I think it would give a
better abstraction if we changed the type of values stored in a
color-table like diff.c::diff_colors[] from physical string sent to the
terminal to a color name (your enum color_name). Then the application
code can initialize their own color-table for each application-specific
slots with GIT_COLOR_NAME_RED, let the configuration mechanism to
customize it for the user. The codepath that currently assume the color
table contains strings that can be sent to the terminal need to be
modified to ask color_code_to_terminal_string(GIT_COLOR_NAME_YELLOW) or
something. Which means:
(1) Physical color representation should be known only to color.c. I.e.
#define GIT_COLOR_BOLD "\033[1m"
does not belong to color.h (public header for application consumption)
nor diff.c (application);
(2) Logical color name and the ways to convert it for terminal consumption
belongs to color.h. I.e.
enum color_name {
GIT_COLOR_NAME_YELLOW,
...
}
should go to color.h;
color_fprintf() should be changed to take "enum color_name color"
instead of "const char *color";
We would need strbuf variant for callers that prepare the string in
core before giving it to fprintf().
(3) "static const char *git_color_codes[]" would be an implementation
detail of the current "ANSI-only" one, hidden inside color.c, for
color_fprintf() and its strbuf cousin to look at.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
