From: Pratyush Yadav <me@yadavpratyush.com>
To: Serg Tereshchenko <serg.partizan@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] git-gui: Basic dark mode support
Date: Tue, 22 Sep 2020 16:34:19 +0530 [thread overview]
Message-ID: <20200922110419.ymqj4ol76kg6qshf@yadavpratyush.com> (raw)
In-Reply-To: <20200824154835.160749-1-serg.partizan@gmail.com>
Hi Serg,
Thanks for the patch and sorry for taking so long in getting to it.
On 24/08/20 06:48PM, Serg Tereshchenko wrote:
> Hi all.
>
> I want to use dark themes with git citool, and here is my first attempt
> to do so.
Like I said in the previous email this part doesn't really belong in the
commit message. In fact, while this entire text is a good description of
the patch and your efforts, it is not a good commit message.
> I am new to tcl, so i happily accept any tips on how to improve code.
>
> First things first: to properly support colors, would be nice to have
> them separated from app code, so i created new file lib/colored.tcl. Name
> is selected to be consistent with "lib/themed.tcl".
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.
> Then, i extract hardcoded colors from git-gui.sh into namespace Color.
> Then, if option use_ttk is true, i update default colors for
> background/foreground from current theme.
>
> How it was looking before:
> - Dark theme (awdark): https://i.imgur.com/0lrfHyq.png
> - Light theme (clam): https://i.imgur.com/1fsfayJ.png
>
> Now looks like this:
> - Dark theme (awdark): https://i.imgur.com/BISllEH.png
> - Light theme (clam): https://i.imgur.com/WclSTa4.png
This is quite an improvement :-)
> One problem that i can't yet fix: gray background for files in
> changelists. Any advice on this?
You can set that in the function `rmsel_tag` in git-gui.sh on the line
$text tag conf in_sel -background lightgray
> 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?
> I see some work is already done in that direction, like lib/themed.tcl:gold_frame.
>
>
> Kind Regards.
>
> Signed-off-by: Serg Tereshchenko <serg.partizan@gmail.com>
> ---
> git-gui.sh | 33 +++++++++++++++++++--------------
> lib/colored.tcl | 23 +++++++++++++++++++++++
> 2 files changed, 42 insertions(+), 14 deletions(-)
> create mode 100644 lib/colored.tcl
>
> diff --git a/git-gui.sh b/git-gui.sh
> index ca66a8e..cffd106 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -861,6 +861,7 @@ proc apply_config {} {
> set NS ttk
> bind [winfo class .] <<ThemeChanged>> [list InitTheme]
> pave_toplevel .
> + Color::syncColorsWithTheme
> }
> }
> }
> @@ -3273,9 +3274,13 @@ pack .vpane -anchor n -side top -fill both -expand 1
> # -- Working Directory File List
>
> textframe .vpane.files.workdir -height 100 -width 200
> -tlabel .vpane.files.workdir.title -text [mc "Unstaged Changes"] \
> - -background lightsalmon -foreground black
> -ttext $ui_workdir -background white -foreground black \
> +tlabel .vpane.files.workdir.title \
> + -text [mc "Unstaged Changes"] \
> + -background $Color::lightRed \
> + -foreground $Color::textOnLight
> +ttext $ui_workdir \
> + -background $Color::textBg \
> + -foreground $Color::textColor \
> -borderwidth 0 \
> -width 20 -height 10 \
> -wrap none \
> @@ -3296,8 +3301,8 @@ pack $ui_workdir -side left -fill both -expand 1
> textframe .vpane.files.index -height 100 -width 200
> tlabel .vpane.files.index.title \
> -text [mc "Staged Changes (Will Commit)"] \
> - -background lightgreen -foreground black
> -ttext $ui_index -background white -foreground black \
> + -background $Color::lightGreen -foreground $Color::textOnLight
> +ttext $ui_index -background $Color::textBg -foreground $Color::textColor \
> -borderwidth 0 \
> -width 20 -height 10 \
> -wrap none \
> @@ -3432,7 +3437,7 @@ if {![is_enabled nocommit]} {
> }
>
> textframe .vpane.lower.commarea.buffer.frame
> -ttext $ui_comm -background white -foreground black \
> +ttext $ui_comm -background $Color::textBg -foreground $Color::textColor \
> -borderwidth 1 \
> -undo true \
> -maxundo 20 \
> @@ -3519,19 +3524,19 @@ trace add variable current_diff_path write trace_current_diff_path
>
> gold_frame .vpane.lower.diff.header
> tlabel .vpane.lower.diff.header.status \
> - -background gold \
> - -foreground black \
> + -background $Color::lightGold \
> + -foreground $Color::textOnLight \
> -width $max_status_desc \
> -anchor w \
> -justify left
> tlabel .vpane.lower.diff.header.file \
> - -background gold \
> - -foreground black \
> + -background $Color::lightGold \
> + -foreground $Color::textOnLight \
> -anchor w \
> -justify left
> tlabel .vpane.lower.diff.header.path \
> - -background gold \
> - -foreground blue \
> + -background $Color::lightGold \
> + -foreground $Color::lightBlue \
> -anchor w \
> -justify left \
> -font [eval font create [font configure font_ui] -underline 1] \
> @@ -3561,7 +3566,7 @@ bind .vpane.lower.diff.header.path <Button-1> {do_file_open $current_diff_path}
> #
> textframe .vpane.lower.diff.body
> set ui_diff .vpane.lower.diff.body.t
> -ttext $ui_diff -background white -foreground black \
> +ttext $ui_diff -background $Color::textBg -foreground $Color::textColor \
> -borderwidth 0 \
> -width 80 -height 5 -wrap none \
> -font font_diff \
> @@ -3589,7 +3594,7 @@ foreach {n c} {0 black 1 red4 2 green4 3 yellow4 4 blue4 5 magenta4 6 cyan4 7 gr
> $ui_diff tag configure clr1 -font font_diffbold
> $ui_diff tag configure clr4 -underline 1
>
> -$ui_diff tag conf d_info -foreground blue -font font_diffbold
> +$ui_diff tag conf d_info -foreground $Color::lightBlue -font font_diffbold
>
> $ui_diff tag conf d_cr -elide true
> $ui_diff tag conf d_@ -font font_diffbold
> diff --git a/lib/colored.tcl b/lib/colored.tcl
> new file mode 100644
> index 0000000..fdb3f9c
> --- /dev/null
> +++ b/lib/colored.tcl
> @@ -0,0 +1,23 @@
> +# Color configuration support for git-gui.
> +
> +namespace eval Color {
FWIW I don't mind if you just put all this in the global namespace, but
I'll leave it up to you.
> + # static colors
> + variable lightRed lightsalmon
> + variable lightGreen green
> + variable lightGold gold
> + variable lightBlue blue
> + variable textOnLight black
> + variable textOnDark white
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?
> + # theme colors
> + variable interfaceBg lightgray
> + variable textBg white
> + variable textColor black
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.
> +
> + proc syncColorsWithTheme {} {
> + set Color::interfaceBg [ttk::style lookup Entry -background]
> + set Color::textBg [ttk::style lookup Treeview -background]
> + set Color::textColor [ttk::style lookup Treeview -foreground]
> +
> + tk_setPalette $Color::interfaceBg
> + }
> +}
Most of the patch looks good to me apart from my small suggestions.
Thanks for working on this.
--
Regards,
Pratyush Yadav
next prev parent reply other threads:[~2020-09-22 11:04 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-24 15:48 [PATCH] git-gui: Basic dark mode support Serg Tereshchenko
2020-08-25 19:01 ` Matthias Aßhauer
2020-09-22 11:04 ` Pratyush Yadav [this message]
2020-09-26 14:54 ` [PATCH v2] " Serg Tereshchenko
2020-10-07 11:07 ` Pratyush Yadav
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=20200922110419.ymqj4ol76kg6qshf@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).