| Message ID | 20201122133233.7077-1-serg.partizan@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 62aed982fdc8a961ae8addfad339e8dfcd28b248 |
| Headers | show |
| Series | git-gui: Fix selected text colors | expand |
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 > } > } > >
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.
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
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.
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.
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.
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 } }
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(-)