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

From: Pratyush Yadav <me@yadavpratyush.com>
Date: Oct 7, 2020, 11:07 UTC
Message-ID: <20201007110751.237kem2mlnb7hbrk@yadavpratyush.com>
In-Reply-To: <20200926145443.15423-1-serg.partizan@gmail.com>

Hi Serg,

On 26/09/20 05:54PM, Serg Tereshchenko wrote:

Show 22 quoted lines

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

Show 16 quoted lines

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

Show 18 quoted lines

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

Show 5 quoted lines

> 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