git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Pratyush Yadav <me@yadavpratyush.com>
To: Serg Tereshchenko <serg.partizan@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2] git-gui: Basic dark mode support
Date: Wed, 7 Oct 2020 16:37:51 +0530
Message-ID: <20201007110751.237kem2mlnb7hbrk@yadavpratyush.com> (raw)
In-Reply-To: <20200926145443.15423-1-serg.partizan@gmail.com>

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.

-- 
Regards,
Pratyush Yadav

  reply	other threads:[~2020-10-07 11:08 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-24 15:48 [PATCH] " Serg Tereshchenko
2020-08-25 19:01 ` Matthias Aßhauer
2020-09-22 11:04 ` Pratyush Yadav
2020-09-26 14:54   ` [PATCH v2] " Serg Tereshchenko
2020-10-07 11:07     ` Pratyush Yadav [this message]
2020-10-08  8:24       ` [PATCH] " Serg Tereshchenko
2020-10-08 13:07     ` [PATCH v2] " Pratyush Yadav
2020-11-21 17:47       ` Stefan Haller
2020-11-22 12:30         ` serg.partizan
2020-11-22 13:32         ` [PATCH] git-gui: Fix selected text colors Serg Tereshchenko
2020-11-22 15:41           ` Stefan Haller
2020-11-22 17:16             ` serg.partizan
2020-11-23 11:48               ` [PATCH] git-gui: use gray selection background for inactive text widgets Stefan Haller
2020-11-23 13:13                 ` serg.partizan
2020-11-23 19:03                   ` Stefan Haller
2020-11-23 20:08                     ` serg.partizan
2020-11-29 17:40                 ` Stefan Haller
2020-11-30 13:41                   ` serg.partizan
2020-11-30 18:08                     ` [PATCH] git-gui: use gray selection background for inactive text?? widgets Pratyush Yadav
2020-11-30 20:18                       ` [PATCH] git-gui: use gray selection background for inactive text widgets Stefan Haller
2020-11-30 20:18                         ` [PATCH] git-gui: keep showing selection when diff view gets deactivated on Mac Stefan Haller
2020-11-23 19:03               ` [PATCH] git-gui: Fix selected text colors Stefan Haller
2020-11-23 20:50                 ` serg.partizan
2020-11-24 21:19                   ` Stefan Haller
2020-11-24 21:23                     ` [PATCH v2] git-gui: use gray background for inactive text widgets Stefan Haller
2020-12-17 21:49                       ` Pratyush Yadav
2020-12-17 22:14                         ` Stefan Haller
2020-12-18 12:50                           ` Pratyush Yadav
2020-12-18 13:01                             ` Stefan Haller
2020-12-18  9:43                         ` [PATCH v3] " Stefan Haller
2020-12-18 12:51                           ` Pratyush Yadav
2020-12-18 19:46                           ` Pratyush Yadav
2020-12-17 20:23           ` [PATCH] git-gui: Fix selected text colors Pratyush Yadav
2020-10-07 11:13 ` [PATCH] git-gui: Basic dark mode support Pratyush Yadav
2020-10-08  8:20   ` Serg Tereshchenko
2020-10-08  8:28     ` Pratyush Yadav
2020-10-08  8:44       ` Serg Tereshchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201007110751.237kem2mlnb7hbrk@yadavpratyush.com \
    --to=me@yadavpratyush.com \
    --cc=git@vger.kernel.org \
    --cc=serg.partizan@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git