Re: [PATCH] gitk: make use of themed widgets where available

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

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.

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.