| Message ID | 20200926145443.15423-1-serg.partizan@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 01121d6132135e62d4bf7c2e67e749ae3037d0ba |
| Headers | show |
| Series | [v2] git-gui: Basic dark mode support | expand |
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.
> 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
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.
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
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.
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]}]} {