[PATCH] git-gui: Basic dark mode support

[PATCH] git-gui: Basic dark mode support

[Serg Tereshchenko]

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

One problem that i can't yet fix: gray background for files in
changelists. Any advice on this?


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?

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
+	# theme colors
+	variable interfaceBg	lightgray
+	variable textBg			white
+	variable textColor		black
+
+	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
+	}
+}
-- 
2.28.0

Re: [PATCH] git-gui: Basic dark mode support

[Matthias Aßhauer]

 >One problem that i can't yet fix: gray background for files 
inchangelists. Any advice on this?

If I understand the issue and the tcl/tk docs correctly, you can change 
that color using -highlightcolor on the ttext widget.

I've cc'ed in Pratyush, the current git-gui maintainer. He's probably 
best suited to review this patch.


Best regards


Matthias

Re: [PATCH] git-gui: Basic dark mode support

[Pratyush Yadav]

Hi Serg,

Thanks for the patch and sorry for taking so long in getting to it.

On 24/08/20 06:48PM, Serg Tereshchenko wrote:
> Hi all.
> 
> I want to use dark themes with git citool, and here is my first attempt
> to do so.

Like I said in the previous email this part doesn't really belong in the 
commit message. In fact, while this entire text is a good description of 
the patch and your efforts, it is not a good commit message.
 
> 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".

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.
 
> 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

This is quite an improvement :-)
 
> One problem that i can't yet fix: gray background for files in
> changelists. Any advice on this?

You can set that in the function `rmsel_tag` in git-gui.sh on the line

  $text tag conf in_sel -background lightgray
 
> 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?
 
> 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 {

FWIW I don't mind if you just put all this in the global namespace, but 
I'll leave it up to you.

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

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?

> +	# theme colors
> +	variable interfaceBg	lightgray
> +	variable textBg			white
> +	variable textColor		black

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.

> +
> +	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
> +	}
> +}

Most of the patch looks good to me apart from my small suggestions. 
Thanks for working on this.

-- 
Regards,
Pratyush Yadav

[PATCH v2] git-gui: Basic dark mode support

[Serg Tereshchenko]

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(-)

diff --git a/git-gui.sh b/git-gui.sh
index d18b902..867b8ce 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -720,7 +720,9 @@ proc rmsel_tag {text} {
 		-background [$text cget -background] \
 		-foreground [$text cget -foreground] \
 		-borderwidth 0
-	$text tag conf in_sel -background lightgray
+	$text tag conf in_sel\
+		-background $color::select_bg \
+		-foreground $color::select_fg
 	bind $text <Motion> break
 	return $text
 }
@@ -863,6 +865,7 @@ proc apply_config {} {
 			set NS ttk
 			bind [winfo class .] <<ThemeChanged>> [list InitTheme]
 			pave_toplevel .
+			color::sync_with_theme
 		}
 	}
 }
@@ -3272,7 +3275,7 @@ pack .vpane -anchor n -side top -fill both -expand 1
 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 \
+ttext $ui_workdir \
 	-borderwidth 0 \
 	-width 20 -height 10 \
 	-wrap none \
@@ -3294,7 +3297,7 @@ 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 \
+ttext $ui_index \
 	-borderwidth 0 \
 	-width 20 -height 10 \
 	-wrap none \
@@ -3321,7 +3324,9 @@ if {!$use_ttk} {
 
 foreach i [list $ui_index $ui_workdir] {
 	rmsel_tag $i
-	$i tag conf in_diff -background [$i tag cget in_sel -background]
+	$i tag conf in_diff \
+		-background $color::select_bg \
+		-foreground $color::select_fg
 }
 unset i
 
@@ -3429,7 +3434,7 @@ if {![is_enabled nocommit]} {
 }
 
 textframe .vpane.lower.commarea.buffer.frame
-ttext $ui_comm -background white -foreground black \
+ttext $ui_comm \
 	-borderwidth 1 \
 	-undo true \
 	-maxundo 20 \
@@ -3558,7 +3563,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 \
 	-borderwidth 0 \
 	-width 80 -height 5 -wrap none \
 	-font font_diff \
diff --git a/lib/themed.tcl b/lib/themed.tcl
index 88b3119..83e3ac7 100644
--- a/lib/themed.tcl
+++ b/lib/themed.tcl
@@ -1,6 +1,44 @@
 # Functions for supporting the use of themed Tk widgets in git-gui.
 # Copyright (C) 2009 Pat Thoyts <patthoyts@users.sourceforge.net>
 
+
+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
+
+	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 color::select_bg $select_bg
+		set color::select_fg $select_fg
+
+		proc add_option {key val} {
+			option add $key $val widgetDefault
+		}
+		# Add options for plain Tk widgets
+		# Using `option add` instead of tk_setPalette to avoid unintended
+		# consequences.
+		if {![is_MacOSX]} {
+			add_option *Menu.Background $base_bg
+			add_option *Menu.Foreground $base_fg
+			add_option *Menu.activeBackground $select_bg
+			add_option *Menu.activeForeground $select_fg
+		}
+		add_option *Text.Background $text_bg
+		add_option *Text.Foreground $text_fg
+		add_option *Text.HighlightBackground $base_bg
+		add_option *Text.HighlightColor $select_bg
+	}
+}
+
 proc ttk_get_current_theme {} {
 	# Handle either current Tk or older versions of 8.5
 	if {[catch {set theme [ttk::style theme use]}]} {
-- 
2.28.0

Re: [PATCH v2] git-gui: Basic dark mode support

[Pratyush Yadav]

Hi Serg,

On 26/09/20 05:54PM, Serg Tereshchenko wrote:
> 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.

Ok. Sounds like a good idea.
 
> 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.

Ok.
 
> > 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.

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.
 
> 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 rest of the patch looks good. Will apply. Thanks.

-- 
Regards,
Pratyush Yadav

Re: [PATCH] git-gui: Basic dark mode support

[Serg Tereshchenko]

> 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.

I have no objections, please use this message.

I'll try to write better commit messages in the future.

--
Regards,
Serg Tereshchenko

Re: [PATCH v2] git-gui: Basic dark mode support

[Pratyush Yadav]

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.

-- 
Regards,
Pratyush Yadav

Re: [PATCH v2] git-gui: Basic dark mode support

[Stefan Haller]

On 08.10.20 15:07, Pratyush Yadav wrote:
> 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

Re: [PATCH v2] git-gui: Basic dark mode support

[serg.partizan]

On Sat, Nov 21, 2020 at 18:47, Stefan Haller <stefan@haller-berlin.de> 
wrote:
> 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

Looks like it uses inversed text colors for select colors. It works the 
same way on linux too.

I'll try to figure out why and how it can be fixed.

[PATCH] git-gui: Fix selected text colors

[Serg Tereshchenko]

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(-)

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
 	}
 }
 
-- 
2.29.2

Re: [PATCH] git-gui: Fix selected text colors

[Stefan Haller]

On 22.11.20 14:32, Serg Tereshchenko wrote:
> Stefan, please check if this fixes select colors for you.

Yes, this works. Thanks for the quick fix! I tested on Mac in both light
and dark mode, and on Windows.

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

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.

-Stefan


> 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
>  	}
>  }
>  
> 

Re: [PATCH] git-gui: Fix selected text colors

[serg.partizan]

On Sun, Nov 22, 2020 at 16:41, Stefan Haller <stefan@haller-berlin.de> 
wrote:
> 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'm using this code in `wish` to query widget for available options:

 > text .t
 > .t configure

And it shows this widget has `-inactiveselectbackground` option. 
However, it doesn't have `-inactiveselectforeground` as I was thinking 
in previous patch.

 > .t configure -inactiveselectbackground
-inactiveselectbackground inactiveSelectBackground Foreground #c3c3c3 
#c3c3c3

But I have no idea how to get this colors from ttk::style. Looking at 
awdark theme, it set's inactiveselectbackground in function 
setTextColors, which is used on text widget directly. And we cannot use 
it here.

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.

And, because we're using "widgetDefault" priority - themes can override 
this, when they want to explicitly set this color.

[PATCH] git-gui: use gray selection background for inactive text widgets

[Stefan Haller]

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] {
+				%W tag conf $tag \
+					-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

Re: [PATCH] git-gui: use gray selection background for inactive text widgets

[serg.partizan]

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.
> --- 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} {

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?

> +				%W tag conf $tag \

And this `%W`, probably should be `$i`?

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

And maybe this new code should be grouped into function like 
"bind_tag_selection_handlers" to improve readability?

>  }
>  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
> 

Re: [PATCH] git-gui: use gray selection background for inactive text widgets

[Stefan Haller]

On 23.11.20 14:13, serg.partizan@gmail.com wrote:
> 
> 
> 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.

That's too bad, as I don't think the patch is acceptable with this
defect. I could maybe see if I can find something by reading the Tk
sources, but I'm not really sure where to start, to be honest. Any
suggestions appreciated.


>> 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.

I only added this check because I initialize the select_fg color to
lightblue in non-ttk mode, so the file lists would switch color even
though the text fields don't, and I wanted to avoid that. Of course, if
I initialize select_fg to lightgray as before, this is not an issue, and
the behavior is unchanged in non-ttk mode. I'll change that in v2.

>> +        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.

Re: [PATCH] git-gui: use gray selection background for inactive text widgets

[serg.partizan]

On Mon, Nov 23, 2020 at 20:03, Stefan Haller <stefan@haller-berlin.de> 
wrote:
>>>  +        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).

I tried to do this, and now i understand why my suggestion was wrong, i 
was looking at this as "cycle inside cycle", but it's actually "cycle 
inside event handler".

> 
>>>  +                %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.

Oh, now i see it's used in the same way in other places!

 > %W  The path name of the window to which the event was reported (the 
window field from the event).

Now I understand it.

Re: [PATCH] git-gui: use gray selection background for inactive text widgets

[Stefan Haller]

On 23.11.20 12:48, Stefan Haller 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?

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.

-Stefan

[1] https://github.com/tcltk/tk/blob/main/generic/tkTextDisp.c#L847
[2] https://wiki.tcl-lang.org/page/Read-only+text+widget

Re: [PATCH] git-gui: use gray selection background for inactive text widgets

[serg.partizan]

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.

Re: [PATCH] git-gui: use gray selection background for inactive text?? widgets

[Pratyush Yadav]

Hi,

I have not had the time to go through these patches. I'll try to do it 
in a couple days.

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.
 
> 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.
> 
> 

-- 
Regards,
Pratyush Yadav

Re: [PATCH] git-gui: use gray selection background for inactive text widgets

[Stefan Haller]

On 30.11.20 19:08, Pratyush Yadav wrote:
> 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.

Yeah, maybe. Just for the record, here's a patch that does it (in the next
message), and frankly, I don't think it's so bad. I do think it's enough of an
improvement that it's worth having. I guess I'll have to maintain it in my local
build if you don't like it.

-Stefan

[PATCH] git-gui: keep showing selection when diff view gets deactivated on Mac

[Stefan Haller]

On Mac, Tk text widgets don't draw the selection when they are inactive and
disabled [1]. This causes the diff selection to disappear on Mac when the diff
view loses focus.

To work around that, we configure text views to be enabled when they become
deactivated. While this feels wrong, there's not problem with it because as soon
as the user tries to interact with the view, the first thing that happens is
that it gets disabled again.

[1] https://github.com/tcltk/tk/blob/main/generic/tkTextDisp.c#L847

Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
---
 lib/themed.tcl | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/lib/themed.tcl b/lib/themed.tcl
index db49085..eeb5bf8 100644
--- a/lib/themed.tcl
+++ b/lib/themed.tcl
@@ -302,6 +302,38 @@ proc tspinbox {w args} {
 	}
 }

+proc focus_text {w} {
+	global text_states
+
+	[winfo parent $w] state focus
+
+	if {[is_MacOSX]} {
+		# Restore the disabled state that we remembered for this widget when it
+		# got deactivated last. If there's no remembered state, then this is the
+		# first time we are being activated right after construction, and
+		# there's no need to change the state.
+		if {[info exists text_states($w)]} {
+			$w configure -state $text_states($w)
+			unset text_states($w)
+		}
+	}
+}
+
+proc unfocus_text {w} {
+	global text_states
+
+	[winfo parent $w] state !focus
+
+	if {[is_MacOSX]} {
+		# On Mac, the selection is not drawn when a text widget is inactive and
+		# disabled. To work around that, set the disabled state back to normal
+		# when deactivating the widget. Remember the disabled state so that we
+		# can restore it when we become active again.
+		set text_states($w) [lindex [$w configure -state] end]
+		$w configure -state normal
+	}
+}
+
 # Create a text widget with any theme specific properties.
 proc ttext {w args} {
 	global use_ttk
@@ -315,8 +347,8 @@ proc ttext {w args} {
 	set w [eval [linsert $args 0 text $w]]
 	if {$use_ttk} {
 		if {[winfo class [winfo parent $w]] eq "EntryFrame"} {
-			bind $w <FocusIn> {[winfo parent %W] state focus}
-			bind $w <FocusOut> {[winfo parent %W] state !focus}
+			bind $w <FocusIn> {focus_text %W}
+			bind $w <FocusOut> {unfocus_text %W}
 		}
 	}
 	return $w
--
2.29.0.21.g59e7d82785.dirty

Re: [PATCH] git-gui: Fix selected text colors

[Stefan Haller]

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.

I tested this, but it doesn't work well enough, in my opinion. An
#888888 gray is too dark for normal mode, but too bright for dark mode
on Mac.

Calculating a gray color is not really so difficult, so I'll just do
that in v2. 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.

-Stefan

Re: [PATCH] git-gui: Fix selected text colors

[serg.partizan]

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?

If I put `puts "InitTheme"` into InitTheme which is called on 
ThemeChanged, i can see it being called multiple times after git-gui 
starts, but when I change theme using "echo '*TkTheme: awdark' | xrdb 
-merge -", it is not called.

I suppose that signal is called only when theme is changed inside app. 
Yes, i just tested this, and that event is sent when you change theme 
from the app.

So you can safely put your code inside "color::sync_with_theme".

And We should move call to sync_with_theme from git-gui.sh into 
InitTheme. I don't know why I have not put it there before.

Re: [PATCH] git-gui: Fix selected text colors

[Stefan Haller]

On 23.11.20 21:50, serg.partizan@gmail.com wrote:
> 
> 
> 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?

By changing the Appearance setting from Light to Dark or back in Mac's
preferences window. The git gui window does update dynamically when you
do this.

However, I think I was wrong when I assumed that this would change the
theme; there's only one theme on Mac, the "aqua" theme. It just changes
its colors, it seems.

> So you can safely put your code inside "color::sync_with_theme".

Will do; I'll send out v2 in a moment.

> And We should move call to sync_with_theme from git-gui.sh into
> InitTheme. I don't know why I have not put it there before.

Yes, I was wondering this too. But as it doesn't seem to make a
difference in practice, I'll leave this for someone else to fix at some
point.

[PATCH v2] git-gui: use gray background for inactive text widgets

[Stefan Haller]

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]}]} {
--
2.29.0.21.g59e7d82785.dirty

Re: [PATCH v2] git-gui: use gray background for inactive text widgets

[Pratyush Yadav]

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.

-- 
Regards,
Pratyush Yadav

Re: [PATCH v2] git-gui: use gray background for inactive text widgets

[Stefan Haller]

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

Re: [PATCH v2] git-gui: use gray background for inactive text widgets

[Pratyush Yadav]

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:
> >> 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.

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?
 
> >> +	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

-- 
Regards,
Pratyush Yadav

Re: [PATCH v2] git-gui: use gray background for inactive text widgets

[Stefan Haller]

On 18.12.20 13:50, Pratyush Yadav wrote:
> 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?
According to the winfo man page this is the only way to do this;
see [1].

Instead of the lassign, you can also do

  foreach {r g b} [winfo rgb . $rgb] {}

but I don't think that's better.

-Stefan


[1] https://www.tcl.tk/man/tcl8.6/TkCmd/winfo.htm

[PATCH v3] git-gui: use gray background for inactive text widgets

[Stefan Haller]

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

 		set color::select_bg $select_bg
 		set color::select_fg $select_fg
+		set color::inactive_select_bg $inactive_select_bg
+		set color::inactive_select_fg $inactive_select_fg

 		proc add_option {key val} {
 			option add $key $val widgetDefault
@@ -36,11 +42,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 $inactive_select_bg
+		add_option *Text.inactiveSelectForeground $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]}]} {
--
2.29.0.21.g59e7d82785.dirty

Re: [PATCH v3] git-gui: use gray background for inactive text widgets

[Pratyush Yadav]

On 18/12/20 10:43AM, Stefan Haller wrote:
> 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.

Yes, this is what I wanted. Thanks. Will apply.

-- 
Regards,
Pratyush Yadav

Re: [PATCH v3] git-gui: use gray background for inactive text widgets

[Pratyush Yadav]

On 18/12/20 10:43AM, Stefan Haller wrote:
> 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

This has mixed tabs and spaces for alignment. Changed to all tabs 
locally. Applied to git-gui/master with this change. Thanks.

-- 
Regards,
Pratyush Yadav

Re: [PATCH] git-gui: Fix selected text colors

[Pratyush Yadav]

On 22/11/20 03:32PM, Serg Tereshchenko wrote:
> 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(-)

Applied to git-gui/master. Thanks.

-- 
Regards,
Pratyush Yadav

Re: [PATCH] git-gui: Basic dark mode support

[Pratyush Yadav]

On 24/08/20 06:48PM, Serg Tereshchenko wrote:
> 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.

-- 
Regards,
Pratyush Yadav

Re: [PATCH] git-gui: Basic dark mode support

[Serg Tereshchenko]

> 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/

--
Regards,
Serg Tereshchenko

Re: [PATCH] git-gui: Basic dark mode support

[Pratyush Yadav]

On 08/10/20 11:20AM, Serg Tereshchenko wrote:
> > 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/

Thanks. This is a bit complicated to be honest. I don't think we can do 
much about the "installing Tk themes" part, but we can certainly make it 
easier to select an installed theme in git-gui. A config option like 
gui.tktheme would be good. Something to consider in the future...

-- 
Regards,
Pratyush Yadav

Re: [PATCH] git-gui: Basic dark mode support

[Serg Tereshchenko]

> Thanks. This is a bit complicated to be honest. I don't think we can do 
> much about the "installing Tk themes" part, but we can certainly make it 
> easier to select an installed theme in git-gui. A config option like 
> gui.tktheme would be good. Something to consider in the future...

I think application should not be responsible for setting theme.

Of course, it is simpler for user to set git-gui theme in app config,
but right way to do it - is to set theme on system level.

On mac it is already using aqua (with dark colors if set), and user even
don't know about it.

On windows it uses some windows-like theme by default.

On linux, yes, we must change ~/.Xresources, but this is just how we set themes
for tk apps. It's systemwide, and all tk apps will resect this choice.

--
Regards,
Serg Tereshchenko