| Message ID | 20201123114805.48800-1-stefan@haller-berlin.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-gui: use gray selection background for inactive text widgets | expand |
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 >
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.
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.
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
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.
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. > >
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
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 } }