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

From: Stefan Haller <stefan@haller-berlin.de>
Date: Nov 29, 2020, 17:40 UTC
Message-ID: <1a42781d-0e4c-6478-f26d-5eccbd9c6205@haller-berlin.de>
In-Reply-To: <20201123114805.48800-1-stefan@haller-berlin.de>

On 23.11.20 12:48, Stefan Haller wrote:

Show 12 quoted lines

> 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