Re: [PATCH] gitk: support config the color of linkfgcolor via Gitk Preferences

[Junio C Hamano <gitster@pobox.com>]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 26.02.26 um 13:33 schrieb Gary Wang via GitGitGadget:
>> From: Wang Zichong <wangzichong@deepin.org>
>> 
>> As a dark-theme user, I use the Preferences dialog to set colors
>> for gitk, the only color I cannot change via that dialog is the
>> link foreground color, which will lead me to use the default link
>> color on a dark background that make it not really readable.
>> 
>> This patch makes the link foreground color also configurable in the
>> Gitk Preferences dialog's Color tab, so user won't need to dig into
>> the code/manual to know if the link color is configurable and can
>> simply set the color there.
>
> Makes sense.
>
> Our usual style is to not say "This patch does X to...", but to write in
> imperative mood "Do X to...".

A bit of tangent, but I wonder if it would help new comers if we add
what I sometimes send (e.g., a recent one found in

    https://lore.kernel.org/git/xmqq343ehu4o.fsf@gitster.g/

) somewhere more prominent, like MyFirstContribution?

>> CC: Mark Levedahl <mlevedahl@gmail.com>, Paul Mackerras <paulus@samba.org>

It is unusual to see multiple people listed on a single Cc: trailer.

>> Signed-off-by: Wang Zichong <wangzichong@deepin.org>
>> ---
>>     gitk: support config the color of linkfgcolor via Gitk Preferences
>
> In the subject line, perhaps:
>
>    gitk: support link color in the Preferences dialog
>
>> +    label $page.linkfg -padx 40 -relief sunk -background $linkfgcolor
>> +    ttk::button $page.linkfgbut -text [mc "Link color"] \
>> +        -command [list choosecolor linkfgcolor {} $page [mc "link color"]]
>
> This text "link color" is used in the title of the color selection
> dialog. It then reads awkwardly "Gitk: choose color for link color".
> Let's just use the text "links" for this purpose, and then also just
> "Links" as the label on the button.
>
>> @@ -11891,6 +11896,7 @@ proc prefspage_set_colorswatches {page} {
>>      $page.hunksep configure -background [lindex $diffcolors 2]
>>      $page.markbgsep configure -background $markbgcolor
>>      $page.selbgsep configure -background $selectbgcolor
>> +    $page.linkfg configure -background $linkfgcolor
>
> The variable's value is already serialized in the configuration and
> needs no additional treatment. Good.
>
> -- Hannes