Pat Thoyts writes:
> This patch improves the appearence of gitk on Windows XP and Vista
> by making use of the themed widgets that are provided in Tk 8.5
> and above. For good Vista support 8.6 will be needed.
>
> Signed-off-by: Pat Thoyts <patthoyts@xxxxxxxxxxxxxxxxxxxxx>
Thanks for the patch. It does seem to do a bit more than the commit
description says, though:
- It adds a toggle-fullscreen function. I'd prefer that was done in a
separate patch.
- It makes various changes to the layout in the non-ttk case - in
particular various message widgets get turned into label widgets.
Are label widgets entirely equivalent to message widgets?
Also, the patch has been corrupted by your mailer: on lines containing
only a "+", the "+" has been deleted.
I have a few questions about specific things you've done in the patch:
> +proc ttk_toplevel {w args} {
> + variable use_ttk
> + eval [linsert $args 0 ::toplevel $w]
> + if {$use_ttk} {
> + place [ttk::frame $w._toplevel_background] -x 0 -y 0 -relwidth 1 -relheight 1
What is the effect of this line, or what would happen if it wasn't
there?
> proc show_error {w top msg} {
> + variable use_ttk
> + set ttk [expr {$use_ttk ? "ttk" : ""}]
Is there a strong reason for using variable here rather than global,
or is it just habit?
It looks to me as though $ttk might as well be a global variable,
rather than computing it from $use_ttk everywhere that we need it.
> @@ -1945,8 +1975,10 @@ proc makewindow {} {
> }
> . configure -menu .bar
>
> + place [${ttk}::frame ._main_background] -x 0 -y 0 -relwidth 1 -relheight 1
Once again, what's the reason for using place and the extra frame?
> + if {$use_ttk} {
> + #set p1 [expr {[winfo screenwidth .] - (40 * $charspc)}]
> + #set p0 [expr {[winfo screenwidth .] - (100 * $charspc)}]
> + #.tf.histframe.pwclist sashpos 0 585
> + #.tf.histframe.pwclist sashpos 1 868
> + } else {
> + eval .tf.histframe.pwclist sash place 0 $geometry(pwsash0)
> + eval .tf.histframe.pwclist sash place 1 $geometry(pwsash1)
> + }
Looks like that could be cleaned up a bit.
> - set gm [tk_optionMenu .tf.lbar.gdttype gdttype \
> - [mc "containing:"] \
> - [mc "touching paths:"] \
> - [mc "adding/removing string:"]]
> + if {$use_ttk} {
> + set values [list [mc "containing:"] [mc "touching paths:"] \
> + [mc "adding/removing string:"]]
> + set gm [ttk::combobox .tf.lbar.gdttype -width 10\
> + -values $values -textvariable gdtype]
> + } else {
> + set gm [tk_optionMenu .tf.lbar.gdttype gdttype \
> + [mc "containing:"] \
> + [mc "touching paths:"] \
> + [mc "adding/removing string:"]]
> + }
We could profitably use a helper function here that would take the
list of alternatives and make the combobox/optionMenu.
> - $top.tohead conf -state readonly
> + $top.tohead configure -state readonly
Do all the other instances of conf need to be changed to configure,
and if so, why?
> - checkbutton $top.showlocal -text [mc "Show local changes"] \
> - -font optionfont -variable showlocalchanges
> + ${ttk}::checkbutton $top.showlocal -text [mc "Show local changes"] \
> + -variable showlocalchanges
Why do we lose the -font optionfont?
Thanks,
Paul.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
