patchgit-gui: Basic dark mode support

> I am new to tcl, so i happily accept any tips on how to improve code.
> 
> First things first: to properly support colors, would be nice to have
> them separated from app code, so i created new file lib/colored.tcl. Name
> is selected to be consistent with "lib/themed.tcl".

> Then, i extract hardcoded colors from git-gui.sh into namespace Color.
> Then, if option use_ttk is true, i update default colors for
> background/foreground from current theme.
> 
> How it was looking before:
>  - Dark theme (awdark): https://i.imgur.com/0lrfHyq.png
>  - Light theme (clam): https://i.imgur.com/1fsfayJ.png
> 
> Now looks like this:
>  - Dark theme (awdark): https://i.imgur.com/BISllEH.png
>  - Light theme (clam): https://i.imgur.com/WclSTa4.png

> I see some work is already done in that direction, like lib/themed.tcl:gold_frame.
> 
> 
> Kind Regards.
> 
> Signed-off-by: Serg Tereshchenko <serg.partizan@gmail.com>
> ---
>  git-gui.sh      | 33 +++++++++++++++++++--------------
>  lib/colored.tcl | 23 +++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 14 deletions(-)
>  create mode 100644 lib/colored.tcl
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index ca66a8e..cffd106 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -861,6 +861,7 @@ proc apply_config {} {
>  			set NS ttk
>  			bind [winfo class .] <<ThemeChanged>> [list InitTheme]
>  			pave_toplevel .
> +			Color::syncColorsWithTheme
>  		}
>  	}
>  }
> @@ -3273,9 +3274,13 @@ pack .vpane -anchor n -side top -fill both -expand 1
>  # -- Working Directory File List
>  
>  textframe .vpane.files.workdir -height 100 -width 200
> -tlabel .vpane.files.workdir.title -text [mc "Unstaged Changes"] \
> -	-background lightsalmon -foreground black
> -ttext $ui_workdir -background white -foreground black \
> +tlabel .vpane.files.workdir.title \
> +	-text [mc "Unstaged Changes"] \
> +	-background $Color::lightRed \
> +	-foreground $Color::textOnLight
> +ttext $ui_workdir \
> +	-background $Color::textBg \
> +	-foreground $Color::textColor \
>  	-borderwidth 0 \
>  	-width 20 -height 10 \
>  	-wrap none \
> @@ -3296,8 +3301,8 @@ pack $ui_workdir -side left -fill both -expand 1
>  textframe .vpane.files.index -height 100 -width 200
>  tlabel .vpane.files.index.title \
>  	-text [mc "Staged Changes (Will Commit)"] \
> -	-background lightgreen -foreground black
> -ttext $ui_index -background white -foreground black \
> +	-background $Color::lightGreen -foreground $Color::textOnLight
> +ttext $ui_index -background $Color::textBg -foreground $Color::textColor \
>  	-borderwidth 0 \
>  	-width 20 -height 10 \
>  	-wrap none \
> @@ -3432,7 +3437,7 @@ if {![is_enabled nocommit]} {
>  }
>  
>  textframe .vpane.lower.commarea.buffer.frame
> -ttext $ui_comm -background white -foreground black \
> +ttext $ui_comm -background $Color::textBg -foreground $Color::textColor \
>  	-borderwidth 1 \
>  	-undo true \
>  	-maxundo 20 \
> @@ -3519,19 +3524,19 @@ trace add variable current_diff_path write trace_current_diff_path
>  
>  gold_frame .vpane.lower.diff.header
>  tlabel .vpane.lower.diff.header.status \
> -	-background gold \
> -	-foreground black \
> +	-background $Color::lightGold \
> +	-foreground $Color::textOnLight \
>  	-width $max_status_desc \
>  	-anchor w \
>  	-justify left
>  tlabel .vpane.lower.diff.header.file \
> -	-background gold \
> -	-foreground black \
> +	-background $Color::lightGold \
> +	-foreground $Color::textOnLight \
>  	-anchor w \
>  	-justify left
>  tlabel .vpane.lower.diff.header.path \
> -	-background gold \
> -	-foreground blue \
> +	-background $Color::lightGold \
> +	-foreground $Color::lightBlue \
>  	-anchor w \
>  	-justify left \
>  	-font [eval font create [font configure font_ui] -underline 1] \
> @@ -3561,7 +3566,7 @@ bind .vpane.lower.diff.header.path <Button-1> {do_file_open $current_diff_path}
>  #
>  textframe .vpane.lower.diff.body
>  set ui_diff .vpane.lower.diff.body.t
> -ttext $ui_diff -background white -foreground black \
> +ttext $ui_diff -background $Color::textBg -foreground $Color::textColor \
>  	-borderwidth 0 \
>  	-width 80 -height 5 -wrap none \
>  	-font font_diff \
> @@ -3589,7 +3594,7 @@ foreach {n c} {0 black 1 red4 2 green4 3 yellow4 4 blue4 5 magenta4 6 cyan4 7 gr
>  $ui_diff tag configure clr1 -font font_diffbold
>  $ui_diff tag configure clr4 -underline 1
>  
> -$ui_diff tag conf d_info -foreground blue -font font_diffbold
> +$ui_diff tag conf d_info -foreground $Color::lightBlue -font font_diffbold
>  
>  $ui_diff tag conf d_cr -elide true
>  $ui_diff tag conf d_@ -font font_diffbold
> diff --git a/lib/colored.tcl b/lib/colored.tcl
> new file mode 100644
> index 0000000..fdb3f9c
> --- /dev/null
> +++ b/lib/colored.tcl
> @@ -0,0 +1,23 @@
> +# Color configuration support for git-gui.
> +
> +namespace eval Color {

> +	# static colors
> +	variable lightRed		lightsalmon
> +	variable lightGreen		green
> +	variable lightGold		gold
> +	variable lightBlue		blue
> +	variable textOnLight	black
> +	variable textOnDark		white

> +
> +	proc syncColorsWithTheme {} {
> +		set Color::interfaceBg	[ttk::style lookup Entry -background]
> +		set Color::textBg		[ttk::style lookup Treeview -background]
> +		set Color::textColor	[ttk::style lookup Treeview -foreground]
> +
> +		tk_setPalette $Color::interfaceBg
> +	}
> +}

>> I would be happy to move color definitions from git-gui.sh to
>> themed.tcl, so we can set it once, and not for each ttext call. Do you
>> think this is a good idea now or in the future?
>
>Do you mean to put the `-foreground` and `-background` options in the 
>function ttext in themed.tcl? If so how can a widget specify if it wants 
>a dark text or light for example?

> Hi Pratyush.
> 
> > Wouldn't having the contents of colored.tcl in themed.tcl be a good 
> > idea? The way I see it, colors are part of the theming of the 
> > application.
> 
> You are right, fixed this.
> 
> > You can set that in the function `rmsel_tag` in git-gui.sh on the line
> 
> Thanks, it worked!
> 
> >> I would be happy to move color definitions from git-gui.sh to
> >> themed.tcl, so we can set it once, and not for each ttext call. Do you
> >> think this is a good idea now or in the future?
> >
> >Do you mean to put the `-foreground` and `-background` options in the 
> >function ttext in themed.tcl? If so how can a widget specify if it wants 
> >a dark text or light for example?
> 
> Turns out ttext was always using black/white colors, so i just removed
> it from ttext calls and used `option add` to set default colors.

> And if some widget needs to different, it can be implemented like
> existing gold_frame.
> 
> Or like theoretical `ttext_inverse`, which just calls ttext with
> -background -foreground swapped. Or maybe we can come up with something
> better. Main idea is to keep all theme-related code in themed.tcl.
> 
> > Why have `textOnLight`, `textOnDark` and `textColor` separately? My 
> > guess is that it is for when you want to force light colors regardless 
> > of the theme? Am I right?
> 
> Something like that, i was using it for tlabel like this:
> > tlabel ... -background $Color::lightGreen -foreground $Color::textOnLight
> 
> But, it was actually not related to current task, so i just reverted
> that changes and focused only on getting basic dark theme support.

> > Nitpick: please use snake_case for variable names like the rest of the 
> > code does. Same for the function name below and the namespace name 
> > above.
> 
> Fixed. I was confused by InitTheme and InitEntryFrame.
> 
> --
> Regargs,
> Serg Tereshchenko
> 
> --- 8< ---
> Removed forced colors in ttext widget calls,
> instead using Text.Background/Foreground options.
> This way colors can be configured dependent on current theme, and even
> overriden by user via .Xresources.
> 
> Extracted colors for in_sel/in_diff tags into colors:: namespace,
> where they can be configured from current theme colors.

> Signed-off-by: Serg Tereshchenko <serg.partizan@gmail.com>
> ---
>  git-gui.sh     | 17 +++++++++++------
>  lib/themed.tcl | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+), 6 deletions(-)

> The commit message could be improved. It should first describe the 
> problem it is trying to solve, why it is worth solving, and then tell 
> the codebase to fix it. The details of how it is done can be learned 
> from the contents of the patch, so they are not as important.
> 
> How about the message below?
> 
>   The colors of some ttext widgets are hard-coded. These hard-coded 
>   colors are okay with a light theme but with a dark theme some widgets 
>   are dark colored and the hard-coded ones are still light. This defeats 
>   the purpose of applying the theme and makes the UI look very awkward.
> 
>   Remove the hard-coded colors in ttext calls and use colors from the 
>   theme for those widgets via Text.Background and Text.Foreground from 
>   the option database.
> 
>   Similarly, the highlighting for the currently selected file(s) in the 
>   "Staged Files" and "Unstaged Files" sections is also hard-coded. Pull 
>   the colors for that from the current theme to make sure it is in line 
>   with the rest of the theme colors.
> 
> No need to resend. I'll use this message when applying unless you have 
> any suggestions or objections.

> Removed forced colors in ttext widget calls,
> instead using Text.Background/Foreground options.
> This way colors can be configured dependent on current theme, and even
> overriden by user via .Xresources.
> 
> Extracted colors for in_sel/in_diff tags into colors:: namespace,
> where they can be configured from current theme colors.
> 
> Signed-off-by: Serg Tereshchenko <serg.partizan@gmail.com>
> ---
>  git-gui.sh     | 17 +++++++++++------
>  lib/themed.tcl | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+), 6 deletions(-)

> On 26/09/20 05:54PM, Serg Tereshchenko wrote:
>> Removed forced colors in ttext widget calls,
>> instead using Text.Background/Foreground options.
>> This way colors can be configured dependent on current theme, and even
>> overriden by user via .Xresources.
>>
>> Extracted colors for in_sel/in_diff tags into colors:: namespace,
>> where they can be configured from current theme colors.
>>
>> Signed-off-by: Serg Tereshchenko <serg.partizan@gmail.com>
>> ---
>>  git-gui.sh     | 17 +++++++++++------
>>  lib/themed.tcl | 38 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 49 insertions(+), 6 deletions(-)
> 
> Merged to git-gui/master. Thanks.

> This caused a regression: when selecting text in the diff pane or in 
> the
> commit message window, the selected text now has a black background 
> (on
> Mac and on Windows, I don't have a Linux system to test this). This
> looks quite ugly; it used to be light blue on both of these systems.
> 
> When setting gui.usettk to 0, it is light blue as before (as 
> expected).
> 
> I'm sorry that I can't give any suggestions how to fix this, because I
> have trouble understanding the code related to themes, even after
> staring at it for quite a while this afternoon.
> 
> Best,
> Stefan

> --- 8< ---
> 
> Added selected state colors for text widget.
> 
> Same colors for active and inactive selection, to match previous
> behaviour.

> Signed-off-by: Serg Tereshchenko <serg.partizan@gmail.com>
> ---
>  lib/themed.tcl | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/themed.tcl b/lib/themed.tcl
> index 83e3ac7..eda5f8c 100644
> --- a/lib/themed.tcl
> +++ b/lib/themed.tcl
> @@ -34,8 +34,10 @@ namespace eval color {
>  		}
>  		add_option *Text.Background $text_bg
>  		add_option *Text.Foreground $text_fg
> -		add_option *Text.HighlightBackground $base_bg
> -		add_option *Text.HighlightColor $select_bg
> +		add_option *Text.selectBackground $select_bg
> +		add_option *Text.selectForeground $select_fg
> +		add_option *Text.inactiveSelectBackground $select_bg
> +		add_option *Text.inactiveSelectForeground $select_fg
>  	}
>  }
>  
> 

> Preserving the previous behavior is probably a good idea when fixing a
> regression.
> 
> However, it would actually be nice to have different colors for active
> and inactive selection (could be a follow-up patch). In native Mac and
> Windows applications the active selection background is usually light
> blue, and the inactive one is light grey. This would not just be a
> cosmetic improvement that looks prettier (that wouldn't be worth it),
> but it would be a real usability improvement because it would make it
> much easier to tell which of the four main views has the keyboard 
> focus.
> 
> I couldn't find a way to query the inactive selection colors, though. 
> Do
> you know if there's a way to do that? If not, I guess one way to do 
> this
> is to numerically calculate a grey color with a similar brightness 
> from
> the active selection background. I could work on a patch if you think
> this is an approach that makes sense.

> I think calculating that gray color from current selection bg is too much work
> for just one color.
>
> We can just set inactiveSelectBackground to some neutral gray color like
> #707070 or #909090 which will work fine with both dark and light themes.

> On 22.11.20 18:16, serg.partizan@gmail.com wrote:
>>  I think calculating that gray color from current selection bg is 
>> too much work
>>  for just one color.
>> 
>>  We can just set inactiveSelectBackground to some neutral gray color 
>> like
>>  #707070 or #909090 which will work fine with both dark and light 
>> themes.
> 
> OK, fine with me. Here's a patch that does this (it sits on top of 
> yours). It
> almost works, except for one problem: on Mac, the inactive selection 
> background
> is white instead of lightgray, but only for the diff view; for the 
> commit editor
> it's correct. On Windows it's also correct for both views. I can't 
> figure out
> what's the difference on Mac; do you have an idea what could be wrong?
> 

> --- 8< ---
> 
> This makes it easier to see at a glance which of the four main views 
> has the
> keyboard focus.
> ---
>  git-gui.sh     | 25 +++++++++++++++++++++----
>  lib/themed.tcl | 13 +++++++++----
>  2 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 867b8ce..a8c5cad 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -721,8 +721,8 @@ proc rmsel_tag {text} {
>  		-foreground [$text cget -foreground] \
>  		-borderwidth 0
>  	$text tag conf in_sel\
> -		-background $color::select_bg \
> -		-foreground $color::select_fg
> +		-background $color::inactive_select_bg \
> +		-foreground $color::inactive_select_fg
>  	bind $text <Motion> break
>  	return $text
>  }
> @@ -3325,8 +3325,25 @@ if {!$use_ttk} {
>  foreach i [list $ui_index $ui_workdir] {
>  	rmsel_tag $i
>  	$i tag conf in_diff \
> -		-background $color::select_bg \
> -		-foreground $color::select_fg
> +		-background $color::inactive_select_bg \
> +		-foreground $color::inactive_select_fg
> +
> +	if {$use_ttk} {

> +		bind $i <FocusIn> {
> +			foreach tag [list in_diff in_sel] {
> +				%W tag conf $tag \
> +					-background $color::select_bg \
> +					-foreground $color::select_fg
> +			}
> +		}
> +		bind $i <FocusOut> {
> +			foreach tag [list in_diff in_sel] {

> +					-background $color::inactive_select_bg \
> +					-foreground $color::inactive_select_fg
> +			}
> +		}
> +	}

>  }
>  unset i
> 
> diff --git a/lib/themed.tcl b/lib/themed.tcl
> index eda5f8c..02b15f2 100644
> --- a/lib/themed.tcl
> +++ b/lib/themed.tcl
> @@ -6,8 +6,10 @@ namespace eval color {
>  	# Variable colors
>  	# Preffered way to set widget colors is using add_option.
>  	# In some cases, like with tags in_diff/in_sel, we use these colors.
> -	variable select_bg		lightgray
> -	variable select_fg		black
> +	variable select_bg				lightblue
> +	variable select_fg				black
> +	variable inactive_select_bg		lightgray
> +	variable inactive_select_fg		black
> 
>  	proc sync_with_theme {} {
>  		set base_bg		[ttk::style lookup . -background]
> @@ -16,6 +18,9 @@ namespace eval color {
>  		set text_fg		[ttk::style lookup Treeview -foreground]
>  		set select_bg	[ttk::style lookup Default -selectbackground]
>  		set select_fg	[ttk::style lookup Default -selectforeground]
> +		# We keep inactive_select_bg as the hard-coded light gray above, as
> +		# there doesn't seem to be a way to get it from the theme. Light 
> gray
> +		# should work well for light and dark themes.
> 
>  		set color::select_bg $select_bg
>  		set color::select_fg $select_fg
> @@ -36,8 +41,8 @@ namespace eval color {
>  		add_option *Text.Foreground $text_fg
>  		add_option *Text.selectBackground $select_bg
>  		add_option *Text.selectForeground $select_fg
> -		add_option *Text.inactiveSelectBackground $select_bg
> -		add_option *Text.inactiveSelectForeground $select_fg
> +		add_option *Text.inactiveSelectBackground 
> $color::inactive_select_bg
> +		add_option *Text.inactiveSelectForeground 
> $color::inactive_select_fg
>  	}
>  }
> 
> --
> 2.29.0.18.gf8c967e53c
> 

> 
> 
> On Mon, Nov 23, 2020 at 12:48, Stefan Haller <stefan@haller-berlin.de>
> wrote:
>> On 22.11.20 18:16, serg.partizan@gmail.com wrote:
>>>  I think calculating that gray color from current selection bg is too much work
>>>  for just one color.
>>>
>>>  We can just set inactiveSelectBackground to some neutral gray color like
>>>  #707070 or #909090 which will work fine with both dark and light themes.
>>
>> OK, fine with me. Here's a patch that does this (it sits on top of
>> yours). It almost works, except for one problem: on Mac, the
>> inactive selection background is white instead of lightgray, but
>> only for the diff view; for the commit editor it's correct. On
>> Windows it's also correct for both views. I can't figure out what's
>> the difference on Mac; do you have an idea what could be wrong?
>>
> I have no idea. Can confirm on linux it works as expected.

>> diff --git a/git-gui.sh b/git-gui.sh
>> index 867b8ce..a8c5cad 100755
>> --- a/git-gui.sh
>> +++ b/git-gui.sh
>> @@ -3325,8 +3325,25 @@ if {!$use_ttk} {
>>  foreach i [list $ui_index $ui_workdir] {
>>      rmsel_tag $i
>>      $i tag conf in_diff \
>> -        -background $color::select_bg \
>> -        -foreground $color::select_fg
>> +        -background $color::inactive_select_bg \
>> +        -foreground $color::inactive_select_fg
>> +
>> +    if {$use_ttk} {
> 
> I think this check can be safely removed. This is all standard tk
> widgets, and select_bg/fg only changed if use_ttk is true.

>> +        bind $i <FocusIn> {
>> +            foreach tag [list in_diff in_sel] {
>> +                %W tag conf $tag \
>> +                    -background $color::select_bg \
>> +                    -foreground $color::select_fg
>> +            }
>> +        }
>> +        bind $i <FocusOut> {
>> +            foreach tag [list in_diff in_sel] {
> 
> This two `foreach` can be combined into one?

>>>  +        bind $i <FocusIn> {
>>>  +            foreach tag [list in_diff in_sel] {
>>>  +                %W tag conf $tag \
>>>  +                    -background $color::select_bg \
>>>  +                    -foreground $color::select_fg
>>>  +            }
>>>  +        }
>>>  +        bind $i <FocusOut> {
>>>  +            foreach tag [list in_diff in_sel] {
>> 
>>  This two `foreach` can be combined into one?
> 
> I don't see how; any concrete suggestions? But I have other ideas how 
> to
> simplify the code (by using one function set_selection_colors that 
> takes
> a has_focus bool and is used for both bindings).

> 
>>>  +                %W tag conf $tag \
>> 
>>  And this `%W`, probably should be `$i`?
> 
> No, $i wouldn't work because we're inside curly braces, so $i wouldn't
> get expanded. It would be possible to work around this by using ""
> instead of {}, but why? Using %W seems to be the idiomatic way in
> bindings, we do this everywhere else too.

> On 22.11.20 18:16, serg.partizan@gmail.com wrote:
>> I think calculating that gray color from current selection bg is too much work
>> for just one color.
>>
>> We can just set inactiveSelectBackground to some neutral gray color like
>> #707070 or #909090 which will work fine with both dark and light themes.
> 
> OK, fine with me. Here's a patch that does this (it sits on top of yours). It
> almost works, except for one problem: on Mac, the inactive selection background
> is white instead of lightgray, but only for the diff view; for the commit editor
> it's correct. On Windows it's also correct for both views. I can't figure out
> what's the difference on Mac; do you have an idea what could be wrong?

> After spending quite a while single-stepping through lots of Tk code, 
> I
> found the reason. On Mac, disabled text widgets simply don't draw the
> selection background. [1]
> 
> I can see three options for solving this:
> 
> 1) Don't use "state focus" and "state !focus" on the text widgets, but
>    instead set the selection color manually using "text conf sel
>    -background". Disadvantage: have to calculate the disabled color
>    using a heuristic like I did for the file lists in my v2 patch.
> 
> 2) Don't use "configure -state disabled" to make the diff text widget
>    read-only; instead, use one of the other methods from [2].
>    Disadvantage: quite a big change, and seems complex to me.
> 
> 3) Enable the the diff widget when it loses focus, and disable it 
> again
>    when it gets focus. I tried this in a quick prototype, and it works
>    very well. It just *feels* wrong to enable a read-only text widget
>    while it is unfocused; but I couldn't find any situation where it
>    would behave wrong, because as soon as you try to interact with it,
>    the first thing that happens is that it gets disabled again.
> 
> I tend towards option 3, because it's reasonably simple and works. 
> I'll
> work on a patch tomorrow unless anybody has objections.
> 

> 
> 
> On Sun, Nov 29, 2020 at 18:40, Stefan Haller <stefan@haller-berlin.de>
> wrote:
> > After spending quite a while single-stepping through lots of Tk code, I
> > found the reason. On Mac, disabled text widgets simply don't draw the
> > selection background. [1]
> > 
> > I can see three options for solving this:
> > 
> > 1) Don't use "state focus" and "state !focus" on the text widgets, but
> >    instead set the selection color manually using "text conf sel
> >    -background". Disadvantage: have to calculate the disabled color
> >    using a heuristic like I did for the file lists in my v2 patch.
> > 
> > 2) Don't use "configure -state disabled" to make the diff text widget
> >    read-only; instead, use one of the other methods from [2].
> >    Disadvantage: quite a big change, and seems complex to me.
> > 
> > 3) Enable the the diff widget when it loses focus, and disable it again
> >    when it gets focus. I tried this in a quick prototype, and it works
> >    very well. It just *feels* wrong to enable a read-only text widget
> >    while it is unfocused; but I couldn't find any situation where it
> >    would behave wrong, because as soon as you try to interact with it,
> >    the first thing that happens is that it gets disabled again.
> > 
> > I tend towards option 3, because it's reasonably simple and works. I'll
> > work on a patch tomorrow unless anybody has objections.
> > 
> 
> I don't like any of this options, as it makes code complicated. I personally
> would prefer to not implement this feature at all, but that's just me.

> Maybe Pratyush can say something reasonable about this, as maintainer.
> 
> I propose to wait a week or two for other opinions, before starting to write
> a patch.
> 
> 

> On 30/11/20 03:41PM, serg.partizan@gmail.com wrote:
>> On Sun, Nov 29, 2020 at 18:40, Stefan Haller <stefan@haller-berlin.de>
>> wrote:
>>> After spending quite a while single-stepping through lots of Tk code, I
>>> found the reason. On Mac, disabled text widgets simply don't draw the
>>> selection background. [1]
>>>
>>> I can see three options for solving this:
>>>
>>> 1) Don't use "state focus" and "state !focus" on the text widgets, but
>>>    instead set the selection color manually using "text conf sel
>>>    -background". Disadvantage: have to calculate the disabled color
>>>    using a heuristic like I did for the file lists in my v2 patch.
>>>
>>> 2) Don't use "configure -state disabled" to make the diff text widget
>>>    read-only; instead, use one of the other methods from [2].
>>>    Disadvantage: quite a big change, and seems complex to me.
>>>
>>> 3) Enable the the diff widget when it loses focus, and disable it again
>>>    when it gets focus. I tried this in a quick prototype, and it works
>>>    very well. It just *feels* wrong to enable a read-only text widget
>>>    while it is unfocused; but I couldn't find any situation where it
>>>    would behave wrong, because as soon as you try to interact with it,
>>>    the first thing that happens is that it gets disabled again.
>>>
>>> I tend towards option 3, because it's reasonably simple and works. I'll
>>> work on a patch tomorrow unless anybody has objections.
>>>
>>
>> I don't like any of this options, as it makes code complicated. I personally
>> would prefer to not implement this feature at all, but that's just me.
>
> That is my first thought as well. All 3 alternatives are less than
> ideal. I don't think the problem is big enough to warrant adding hacks
> like this. They will come back to bite us sooner or later.
>
> If you _really_ want to fix this, maybe try convincing the Tk devs about
> fixing it.

> I think calculating that gray color from current selection bg is too
> much work for just one color.
> 
> We can just set inactiveSelectBackground to some neutral gray color like
> #707070 or #909090 which will work fine with both dark and light themes.

> The problem is that it needs to be recalculated when the
> theme changes, and I have trouble testing that because the
> <<ThemeChanged>> event doesn't appear to be sent on Mac, as far as I 
> can
> see.

> 
> 
> On Mon, Nov 23, 2020 at 20:03, Stefan Haller <stefan@haller-berlin.de>
> wrote:
>> The problem is that it needs to be recalculated when the
>> theme changes, and I have trouble testing that because the
>> <<ThemeChanged>> event doesn't appear to be sent on Mac, as far as I can
>> see.
> 
> How are you testing this?

> Second version; it simplifies the code to initialize and update the colors in
> the two file list views a bit, and it calculates a gray color for the inactive
> selection from the active selection. This looks a lot better in the themes I
> have tried.
> 
> The bug with the inactive diff selection background on Mac is still there,
> however.
> 
> --- 8< ---
> 
> This makes it easier to see at a glance which of the four main views has the
> keyboard focus.

> ---
>  git-gui.sh     | 18 ++++++++++++------
>  lib/themed.tcl | 21 +++++++++++++++++----
>  2 files changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 867b8ce..e818caa 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -720,9 +720,6 @@ proc rmsel_tag {text} {
>  		-background [$text cget -background] \
>  		-foreground [$text cget -foreground] \
>  		-borderwidth 0
> -	$text tag conf in_sel\
> -		-background $color::select_bg \
> -		-foreground $color::select_fg
>  	bind $text <Motion> break
>  	return $text
>  }
> @@ -3322,11 +3319,20 @@ if {!$use_ttk} {
>  	.vpane.files paneconfigure .vpane.files.index -sticky news
>  }
> 
> +proc set_selection_colors {w has_focus} {
> +	foreach tag [list in_diff in_sel] {
> +		$w tag conf $tag \
> +			-background [expr {$has_focus ? $color::select_bg : $color::inactive_select_bg}] \
> +			-foreground [expr {$has_focus ? $color::select_fg : $color::inactive_select_fg}]
> +	}
> +}
> +
>  foreach i [list $ui_index $ui_workdir] {
>  	rmsel_tag $i
> -	$i tag conf in_diff \
> -		-background $color::select_bg \
> -		-foreground $color::select_fg
> +
> +	set_selection_colors $i 0
> +	bind $i <FocusIn>	{ set_selection_colors %W 1 }
> +	bind $i <FocusOut>	{ set_selection_colors %W 0 }
>  }
>  unset i
> 
> diff --git a/lib/themed.tcl b/lib/themed.tcl
> index eda5f8c..db49085 100644
> --- a/lib/themed.tcl
> +++ b/lib/themed.tcl
> @@ -6,8 +6,10 @@ namespace eval color {
>  	# Variable colors
>  	# Preffered way to set widget colors is using add_option.
>  	# In some cases, like with tags in_diff/in_sel, we use these colors.
> -	variable select_bg		lightgray
> -	variable select_fg		black
> +	variable select_bg				lightgray
> +	variable select_fg				black
> +	variable inactive_select_bg		lightgray
> +	variable inactive_select_fg		black
> 
>  	proc sync_with_theme {} {
>  		set base_bg		[ttk::style lookup . -background]
> @@ -19,6 +21,8 @@ namespace eval color {
> 
>  		set color::select_bg $select_bg
>  		set color::select_fg $select_fg
> +		set color::inactive_select_bg [convert_rgb_to_gray $select_bg]
> +		set color::inactive_select_fg $select_fg
> 
>  		proc add_option {key val} {
>  			option add $key $val widgetDefault
> @@ -36,11 +40,20 @@ namespace eval color {
>  		add_option *Text.Foreground $text_fg
>  		add_option *Text.selectBackground $select_bg
>  		add_option *Text.selectForeground $select_fg
> -		add_option *Text.inactiveSelectBackground $select_bg
> -		add_option *Text.inactiveSelectForeground $select_fg
> +		add_option *Text.inactiveSelectBackground $color::inactive_select_bg
> +		add_option *Text.inactiveSelectForeground $color::inactive_select_fg

>  	}
>  }
> 
> +proc convert_rgb_to_gray {rgb} {
> +	# Simply take the average of red, green and blue. This wouldn't be good
> +	# enough for, say, converting a photo to grayscale, but for this simple
> +	# purpose of approximating the brightness of a color it's good enough.
> +	lassign [winfo rgb . $rgb] r g b

> +	set gray [expr {($r / 256 + $g / 256 + $b / 256) / 3}]
> +	return [format "#%2.2X%2.2X%2.2X" $gray $gray $gray]
> +}
> +
>  proc ttk_get_current_theme {} {
>  	# Handle either current Tk or older versions of 8.5
>  	if {[catch {set theme [ttk::style theme use]}]} {

> Hi,
> 
> On 24/11/20 10:23PM, Stefan Haller wrote:
>> Second version; it simplifies the code to initialize and update the colors in
>> the two file list views a bit, and it calculates a gray color for the inactive
>> selection from the active selection. This looks a lot better in the themes I
>> have tried.
>>
>> The bug with the inactive diff selection background on Mac is still there,
>> however.
>>
>> --- 8< ---
>>
>> This makes it easier to see at a glance which of the four main views has the
>> keyboard focus.
> 
> Missing Signed-off-by.

>> ---
>>  git-gui.sh     | 18 ++++++++++++------
>>  lib/themed.tcl | 21 +++++++++++++++++----
>>  2 files changed, 29 insertions(+), 10 deletions(-)
>>
>> diff --git a/git-gui.sh b/git-gui.sh
>> index 867b8ce..e818caa 100755
>> --- a/git-gui.sh
>> +++ b/git-gui.sh
>> @@ -720,9 +720,6 @@ proc rmsel_tag {text} {
>>  		-background [$text cget -background] \
>>  		-foreground [$text cget -foreground] \
>>  		-borderwidth 0
>> -	$text tag conf in_sel\
>> -		-background $color::select_bg \
>> -		-foreground $color::select_fg
>>  	bind $text <Motion> break
>>  	return $text
>>  }
>> @@ -3322,11 +3319,20 @@ if {!$use_ttk} {
>>  	.vpane.files paneconfigure .vpane.files.index -sticky news
>>  }
>>
>> +proc set_selection_colors {w has_focus} {
>> +	foreach tag [list in_diff in_sel] {
>> +		$w tag conf $tag \
>> +			-background [expr {$has_focus ? $color::select_bg : $color::inactive_select_bg}] \
>> +			-foreground [expr {$has_focus ? $color::select_fg : $color::inactive_select_fg}]
>> +	}
>> +}
>> +
>>  foreach i [list $ui_index $ui_workdir] {
>>  	rmsel_tag $i
>> -	$i tag conf in_diff \
>> -		-background $color::select_bg \
>> -		-foreground $color::select_fg
>> +
>> +	set_selection_colors $i 0
>> +	bind $i <FocusIn>	{ set_selection_colors %W 1 }
>> +	bind $i <FocusOut>	{ set_selection_colors %W 0 }
>>  }
>>  unset i
>>
>> diff --git a/lib/themed.tcl b/lib/themed.tcl
>> index eda5f8c..db49085 100644
>> --- a/lib/themed.tcl
>> +++ b/lib/themed.tcl
>> @@ -6,8 +6,10 @@ namespace eval color {
>>  	# Variable colors
>>  	# Preffered way to set widget colors is using add_option.
>>  	# In some cases, like with tags in_diff/in_sel, we use these colors.
>> -	variable select_bg		lightgray
>> -	variable select_fg		black
>> +	variable select_bg				lightgray
>> +	variable select_fg				black
>> +	variable inactive_select_bg		lightgray
>> +	variable inactive_select_fg		black
>>
>>  	proc sync_with_theme {} {
>>  		set base_bg		[ttk::style lookup . -background]
>> @@ -19,6 +21,8 @@ namespace eval color {
>>
>>  		set color::select_bg $select_bg
>>  		set color::select_fg $select_fg
>> +		set color::inactive_select_bg [convert_rgb_to_gray $select_bg]
>> +		set color::inactive_select_fg $select_fg
>>
>>  		proc add_option {key val} {
>>  			option add $key $val widgetDefault
>> @@ -36,11 +40,20 @@ namespace eval color {
>>  		add_option *Text.Foreground $text_fg
>>  		add_option *Text.selectBackground $select_bg
>>  		add_option *Text.selectForeground $select_fg
>> -		add_option *Text.inactiveSelectBackground $select_bg
>> -		add_option *Text.inactiveSelectForeground $select_fg
>> +		add_option *Text.inactiveSelectBackground $color::inactive_select_bg
>> +		add_option *Text.inactiveSelectForeground $color::inactive_select_fg
> 
> Nitpick: Do what is being done for select_bg and select_fg and create a 
> local variable for it.
> 
>>  	}
>>  }
>>
>> +proc convert_rgb_to_gray {rgb} {
>> +	# Simply take the average of red, green and blue. This wouldn't be good
>> +	# enough for, say, converting a photo to grayscale, but for this simple
>> +	# purpose of approximating the brightness of a color it's good enough.
>> +	lassign [winfo rgb . $rgb] r g b
> 
> Is there no simpler way to extract r, g, and b? This is a little cryptic 
> to be honest.

>> +	set gray [expr {($r / 256 + $g / 256 + $b / 256) / 3}]
>> +	return [format "#%2.2X%2.2X%2.2X" $gray $gray $gray]
>> +}
>> +
>>  proc ttk_get_current_theme {} {
>>  	# Handle either current Tk or older versions of 8.5
>>  	if {[catch {set theme [ttk::style theme use]}]} {
> 
> The patch looks good for the most part. I can fix the above nitpick 
> locally and merge it in tomorrow if you send me your signoff by then. I 
> don't want to hold off the PR to Junio too much longer. A simple reply 
> containing your Signed-off-by should be fine. Thanks.

> On 17.12.20 22:49, Pratyush Yadav wrote:
> > Hi,
> > 
> > On 24/11/20 10:23PM, Stefan Haller wrote:
> >> Second version; it simplifies the code to initialize and update the colors in
> >> the two file list views a bit, and it calculates a gray color for the inactive
> >> selection from the active selection. This looks a lot better in the themes I
> >> have tried.
> >>
> >> The bug with the inactive diff selection background on Mac is still there,
> >> however.
> >>
> >> --- 8< ---
> >>
> >> This makes it easier to see at a glance which of the four main views has the
> >> keyboard focus.
> > 
> > Missing Signed-off-by.
> 
> If you are willing to add this in yourself, that would be nice, I don't
> have time to send another patch today:
> 
> Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
> 
> >> ---
> >>  git-gui.sh     | 18 ++++++++++++------
> >>  lib/themed.tcl | 21 +++++++++++++++++----
> >>  2 files changed, 29 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/git-gui.sh b/git-gui.sh
> >> index 867b8ce..e818caa 100755
> >> --- a/git-gui.sh
> >> +++ b/git-gui.sh
> >> @@ -720,9 +720,6 @@ proc rmsel_tag {text} {
> >>  		-background [$text cget -background] \
> >>  		-foreground [$text cget -foreground] \
> >>  		-borderwidth 0
> >> -	$text tag conf in_sel\
> >> -		-background $color::select_bg \
> >> -		-foreground $color::select_fg
> >>  	bind $text <Motion> break
> >>  	return $text
> >>  }
> >> @@ -3322,11 +3319,20 @@ if {!$use_ttk} {
> >>  	.vpane.files paneconfigure .vpane.files.index -sticky news
> >>  }
> >>
> >> +proc set_selection_colors {w has_focus} {
> >> +	foreach tag [list in_diff in_sel] {
> >> +		$w tag conf $tag \
> >> +			-background [expr {$has_focus ? $color::select_bg : $color::inactive_select_bg}] \
> >> +			-foreground [expr {$has_focus ? $color::select_fg : $color::inactive_select_fg}]
> >> +	}
> >> +}
> >> +
> >>  foreach i [list $ui_index $ui_workdir] {
> >>  	rmsel_tag $i
> >> -	$i tag conf in_diff \
> >> -		-background $color::select_bg \
> >> -		-foreground $color::select_fg
> >> +
> >> +	set_selection_colors $i 0
> >> +	bind $i <FocusIn>	{ set_selection_colors %W 1 }
> >> +	bind $i <FocusOut>	{ set_selection_colors %W 0 }
> >>  }
> >>  unset i
> >>
> >> diff --git a/lib/themed.tcl b/lib/themed.tcl
> >> index eda5f8c..db49085 100644
> >> --- a/lib/themed.tcl
> >> +++ b/lib/themed.tcl
> >> @@ -6,8 +6,10 @@ namespace eval color {
> >>  	# Variable colors
> >>  	# Preffered way to set widget colors is using add_option.
> >>  	# In some cases, like with tags in_diff/in_sel, we use these colors.
> >> -	variable select_bg		lightgray
> >> -	variable select_fg		black
> >> +	variable select_bg				lightgray
> >> +	variable select_fg				black
> >> +	variable inactive_select_bg		lightgray
> >> +	variable inactive_select_fg		black
> >>
> >>  	proc sync_with_theme {} {
> >>  		set base_bg		[ttk::style lookup . -background]
> >> @@ -19,6 +21,8 @@ namespace eval color {
> >>
> >>  		set color::select_bg $select_bg
> >>  		set color::select_fg $select_fg
> >> +		set color::inactive_select_bg [convert_rgb_to_gray $select_bg]
> >> +		set color::inactive_select_fg $select_fg
> >>
> >>  		proc add_option {key val} {
> >>  			option add $key $val widgetDefault
> >> @@ -36,11 +40,20 @@ namespace eval color {
> >>  		add_option *Text.Foreground $text_fg
> >>  		add_option *Text.selectBackground $select_bg
> >>  		add_option *Text.selectForeground $select_fg
> >> -		add_option *Text.inactiveSelectBackground $select_bg
> >> -		add_option *Text.inactiveSelectForeground $select_fg
> >> +		add_option *Text.inactiveSelectBackground $color::inactive_select_bg
> >> +		add_option *Text.inactiveSelectForeground $color::inactive_select_fg
> > 
> > Nitpick: Do what is being done for select_bg and select_fg and create a 
> > local variable for it.
> > 
> >>  	}
> >>  }
> >>
> >> +proc convert_rgb_to_gray {rgb} {
> >> +	# Simply take the average of red, green and blue. This wouldn't be good
> >> +	# enough for, say, converting a photo to grayscale, but for this simple
> >> +	# purpose of approximating the brightness of a color it's good enough.
> >> +	lassign [winfo rgb . $rgb] r g b
> > 
> > Is there no simpler way to extract r, g, and b? This is a little cryptic 
> > to be honest.
> 
> Actually, I find this pretty elegant, and from what I have seen, it's
> idiomatic Tcl. A less cryptic way would be (untested):
> 
>   set components [winfo rgb . $rgb]
>   set r [lindex $components 0]
>   set g [lindex $components 1]
>   set b [lindex $components 2]
> 
> But I much prefer the one-line version.

> >> +	set gray [expr {($r / 256 + $g / 256 + $b / 256) / 3}]
> >> +	return [format "#%2.2X%2.2X%2.2X" $gray $gray $gray]
> >> +}
> >> +
> >>  proc ttk_get_current_theme {} {
> >>  	# Handle either current Tk or older versions of 8.5
> >>  	if {[catch {set theme [ttk::style theme use]}]} {
> > 
> > The patch looks good for the most part. I can fix the above nitpick 
> > locally and merge it in tomorrow if you send me your signoff by then. I 
> > don't want to hold off the PR to Junio too much longer. A simple reply 
> > containing your Signed-off-by should be fine. Thanks.
> 
> Awesome, thanks for that.
> 
> -Stefan

> On 17/12/20 11:14PM, Stefan Haller wrote:
>> On 17.12.20 22:49, Pratyush Yadav wrote:
>>> Hi,
>>>
>>> On 24/11/20 10:23PM, Stefan Haller wrote>>>> +proc convert_rgb_to_gray {rgb} {
>>>> +	# Simply take the average of red, green and blue. This wouldn't be good
>>>> +	# enough for, say, converting a photo to grayscale, but for this simple
>>>> +	# purpose of approximating the brightness of a color it's good enough.
>>>> +	lassign [winfo rgb . $rgb] r g b
>>>
>>> Is there no simpler way to extract r, g, and b? This is a little cryptic 
>>> to be honest.
>>
>> Actually, I find this pretty elegant, and from what I have seen, it's
>> idiomatic Tcl. A less cryptic way would be (untested):
>>
>>   set components [winfo rgb . $rgb]
>>   set r [lindex $components 0]
>>   set g [lindex $components 1]
>>   set b [lindex $components 2]
>>
>> But I much prefer the one-line version.
> 
> I agree. Using lassign is much neater. But that is not my point. I am 
> talking about the "[winfo rgb . $rgb]". This call generates the list 
> that is then assigned to the 3 variables. This part is a little cryptic. 
> Is there no simpler way to separate out the r, g, and b values?

> This makes it easier to see at a glance which of the four main views has the
> keyboard focus.
> 
> Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
> ---
> Hi Pratyush,
> 
> here's a rerolled version with added Signed-off-by and using local variables for
> inactive_select_bg and inactive_select_bg, hoping this might save you some time.
> Let me know if this is what you had in mind, I wasn't totally sure.

> This makes it easier to see at a glance which of the four main views has the
> keyboard focus.
> 
> Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
> ---
> Hi Pratyush,
> 
> here's a rerolled version with added Signed-off-by and using local variables for
> inactive_select_bg and inactive_select_bg, hoping this might save you some time.
> Let me know if this is what you had in mind, I wasn't totally sure.
> 
>  git-gui.sh     | 18 ++++++++++++------
>  lib/themed.tcl | 35 +++++++++++++++++++++++++----------
>  2 files changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index cc6c2aa..201524c 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -720,9 +720,6 @@ proc rmsel_tag {text} {
>  		-background [$text cget -background] \
>  		-foreground [$text cget -foreground] \
>  		-borderwidth 0
> -	$text tag conf in_sel\
> -		-background $color::select_bg \
> -		-foreground $color::select_fg
>  	bind $text <Motion> break
>  	return $text
>  }
> @@ -3328,11 +3325,20 @@ if {!$use_ttk} {
>  	.vpane.files paneconfigure .vpane.files.index -sticky news
>  }
> 
> +proc set_selection_colors {w has_focus} {
> +	foreach tag [list in_diff in_sel] {
> +		$w tag conf $tag \
> +			-background [expr {$has_focus ? $color::select_bg : $color::inactive_select_bg}] \
> +			-foreground [expr {$has_focus ? $color::select_fg : $color::inactive_select_fg}]
> +	}
> +}
> +
>  foreach i [list $ui_index $ui_workdir] {
>  	rmsel_tag $i
> -	$i tag conf in_diff \
> -		-background $color::select_bg \
> -		-foreground $color::select_fg
> +
> +	set_selection_colors $i 0
> +	bind $i <FocusIn>	{ set_selection_colors %W 1 }
> +	bind $i <FocusOut>	{ set_selection_colors %W 0 }
>  }
>  unset i
> 
> diff --git a/lib/themed.tcl b/lib/themed.tcl
> index 244c061..0f6575a 100644
> --- a/lib/themed.tcl
> +++ b/lib/themed.tcl
> @@ -6,19 +6,25 @@ namespace eval color {
>  	# Variable colors
>  	# Preffered way to set widget colors is using add_option.
>  	# In some cases, like with tags in_diff/in_sel, we use these colors.
> -	variable select_bg		lightgray
> -	variable select_fg		black
> +	variable select_bg				lightgray
> +	variable select_fg				black
> +	variable inactive_select_bg		lightgray
> +	variable inactive_select_fg		black
> 
>  	proc sync_with_theme {} {
> -		set base_bg		[ttk::style lookup . -background]
> -		set base_fg		[ttk::style lookup . -foreground]
> -		set text_bg		[ttk::style lookup Treeview -background]
> -		set text_fg		[ttk::style lookup Treeview -foreground]
> -		set select_bg	[ttk::style lookup Default -selectbackground]
> -		set select_fg	[ttk::style lookup Default -selectforeground]
> +		set base_bg		        [ttk::style lookup . -background]
> +		set base_fg		        [ttk::style lookup . -foreground]
> +		set text_bg		        [ttk::style lookup Treeview -background]
> +		set text_fg		        [ttk::style lookup Treeview -foreground]
> +		set select_bg	        [ttk::style lookup Default -selectbackground]
> +		set select_fg	        [ttk::style lookup Default -selectforeground]
> +		set inactive_select_bg	[convert_rgb_to_gray $select_bg]
> +		set inactive_select_fg 	$select_fg

> Stefan, please check if this fixes select colors for you.
> 
> --- 8< ---
> 
> Added selected state colors for text widget.
> 
> Same colors for active and inactive selection, to match previous
> behaviour.
> 
> Signed-off-by: Serg Tereshchenko <serg.partizan@gmail.com>
> ---
>  lib/themed.tcl | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

> Hi all.
> 
> I want to use dark themes with git citool, and here is my first attempt
> to do so.
> 
> I am new to tcl, so i happily accept any tips on how to improve code.
> 
> First things first: to properly support colors, would be nice to have
> them separated from app code, so i created new file lib/colored.tcl. Name
> is selected to be consistent with "lib/themed.tcl".
> 
> Then, i extract hardcoded colors from git-gui.sh into namespace Color.
> Then, if option use_ttk is true, i update default colors for
> background/foreground from current theme.
> 
> How it was looking before:
>  - Dark theme (awdark): https://i.imgur.com/0lrfHyq.png
>  - Light theme (clam): https://i.imgur.com/1fsfayJ.png
> 
> Now looks like this:
>  - Dark theme (awdark): https://i.imgur.com/BISllEH.png
>  - Light theme (clam): https://i.imgur.com/WclSTa4.png

> > How do you tell git-gui which theme to use? I had some trouble setting 
> > the theme and ended up adding code to source the theme files and then 
> > set the theme via `ttk::style theme use`. I hope there is a better way 
> > than that.
> 
> Yes, there is. To change theme on the fly use:
> 
>     echo '*TkTheme: clam' | xrdb -merge -
> 
> To set theme, add "*TkTheme: clam" to ~/.Xresources and run
> 
>     xrdb -merge ~/.Xresources
> 
> There is lack of dark themes in default tk installs right now,
> i'm using awdark: https://sourceforge.net/projects/tcl-awthemes/
> 
> To install theme you need to unpack it somewhere like ~/.local/share/tk-themes/awthemes
> And tell tcl where to find it.
> 
>     export TCLLIBPATH=$HOME/.local/share/tk-themes
> 
> I had to modify version numbers inside awthemes package to make in work,
> but hope it'll be fixed upstream.
> 
> Here is blog post which explains this in greater detail:
> http://blog.serindu.com/2019/03/07/applying-tk-themes-to-git-gui/