threads / patch / 6715

patchMake gitk save and restore the user set window position.

Subject: [PATCH] Make gitk save and restore the user set window position.

## tl;dr

3 messages between Feb 7, 2007 and Feb 8, 2007. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Junio C Hamano· Feb 7, 2007, 03:21 UTC · lore
From: Mark Levedahl <mdl123@verizon.net>
Date: Mon, 05 Feb 2007 22:59:50 -0500

gitk was saving widget sizes and positions when the main window was destroyed, which is after all child widgets are destroyed. Tk resizes container widgets as children are destroyed, so the saved main window information was not as the user left the window. This patch causes the info to be saved before the first child widget is destroyed. Also, use wm geometry rather than winfo geometry (both work on Windows, on Linux they give different answers and the former is correct).

Signed-off-by: Mark Levedahl <mdl123@verizon.net>
---
 * Somehow this did not appeared on the list, so I am forwarding.
   After seeing what this patch has to do, I feel dirty, but
   that is not Mark's fault -- rather it is Tk's.
   I am tempted to suggest adding an explicit "Save window
   configuration" action on the menubar and forget about
   resurrecting the window configuration immediately before the
   end of the last session.
 gitk |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)
Show changes to gitk +18 −2
diff --git a/gitk b/gitk
index 8132812..beec6a9 100755
--- a/gitk
+++ b/gitk
@@ -725,7 +725,7 @@ proc makewindow {} {
     bind . <Control-KP_Add> {incrfont 1}
     bind . <Control-minus> {incrfont -1}
     bind . <Control-KP_Subtract> {incrfont -1}
-    bind . <Destroy> {savestuff %W}
+    bindalldestroy .
     bind . <Button-1> "click %W"
     bind $fstring <Key-Return> dofind
     bind $sha1entry <Key-Return> gotocommit
@@ -759,6 +759,22 @@ proc makewindow {} {
 	-command rmbranch
 }
 
+# The main window resizes itself while the windows are being destroyed,
+# we want to save the size before it is resized so we write out when the
+# first widget with no children is destroyed.  We don't know which this
+# will be, so we bind all child widgets (savestuff will save only once,
+# the remaining calls are ignored).
+proc bindalldestroy {w} {
+    set children [winfo children $w]
+    if {"$children" == ""} {
+        bind $w <Destroy> {savestuff %W}
+    } else {
+        foreach child $children {
+            bindalldestroy $child
+        }
+    }
+}
+
 # mouse-2 makes all windows scan vertically, but only the one
 # the cursor is in scans horizontally
 proc canvscan {op w x y} {
@@ -829,7 +845,7 @@ proc savestuff {w} {
 	puts $f [list set colors $colors]
 	puts $f [list set diffcolors $diffcolors]
 
-        puts $f "set geometry(main) [winfo geometry .]"
+	puts $f "set geometry(main) [wm geometry .]"
 	puts $f "set geometry(topwidth) [winfo width .tf]"
 	puts $f "set geometry(topheight) [winfo height .tf]"
 	puts $f "set geometry(canv) [expr {[winfo width $canv]-0}]"
-- 
1.5.0.rc3.24.g0c5e
Shawn O. Pearce· Feb 7, 2007, 05:40 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make gitk save and restore the user set window position.

Junio C Hamano <junkio@cox.net> wrote:
Show 7 quoted lines
>    After seeing what this patch has to do, I feel dirty, but
>    that is not Mark's fault -- rather it is Tk's.
> 
>    I am tempted to suggest adding an explicit "Save window
>    configuration" action on the menubar and forget about
>    resurrecting the window configuration immediately before the
>    end of the last session.

Maybe take a look at what git-gui does here, because its slightly saner and still saves the geometry on exit:

	set is_quitting 0
	proc do_quit {} {
		global is_quitting
		if {$is_quitting} return
		set is_quitting 1
		# save wm geometry
	}
	bind . <Destroy> do_quit

OK, its not that much saner. But it does bypass needing to setup some ugly bindings on every object in the UI. Though I recently took a slightly different approach in a dialog:

	proc do_quit {} {
		bind . <Destroy> {}
		# do cleanup
	}
	bind . <Destroy> do_quit

Yes the binding is firing in both cases for some arbitrary child widget in the window, but it doesn't matter. In the latter version setting the binding to the empty string removes the do_quit binding, allowing the other widgets to destroy without reinvoking do_quit and whacking whatever geometry data you may have saved before the widgets started to get deleted.

The only problem I seem to have in git-gui is the window position opens about 10 pixels lower and 2 pixels to the right than the last time it opened. I think there's a bug in Tk on Windows where the window position on the desktop doesn't include the titlebar when I get it, but expects to include it when I attempt to set it on the next start.

I should note I also see the same behavior with my day-time-job's Java apps on Windows however, so I don't think its a specific Tk or git-gui issue.

-- 
Shawn.
Mark Levedahl· Feb 8, 2007, 00:30 UTC · re: Shawn O. Pearce · lore

Re: [PATCH] Make gitk save and restore the user set window position.

Shawn O. Pearce wrote:
Show 53 quoted lines
> Junio C Hamano <junkio@cox.net> wrote:
>   
>>    After seeing what this patch has to do, I feel dirty, but
>>    that is not Mark's fault -- rather it is Tk's.
>>
>>    I am tempted to suggest adding an explicit "Save window
>>    configuration" action on the menubar and forget about
>>    resurrecting the window configuration immediately before the
>>    end of the last session.
>>     
>
> Maybe take a look at what git-gui does here, because its slightly
> saner and still saves the geometry on exit:
>
> 	set is_quitting 0
> 	proc do_quit {} {
> 		global is_quitting
> 		if {$is_quitting} return
> 		set is_quitting 1
> 		# save wm geometry
> 	}
> 	bind . <Destroy> do_quit
>
> OK, its not that much saner.  But it does bypass needing to setup
> some ugly bindings on every object in the UI.  Though I recently
> took a slightly different approach in a dialog:
>
> 	proc do_quit {} {
> 		bind . <Destroy> {}
> 		# do cleanup
> 	}
> 	bind . <Destroy> do_quit
>
> Yes the binding is firing in both cases for some arbitrary child
> widget in the window, but it doesn't matter.  In the latter version
> setting the binding to the empty string removes the do_quit binding,
> allowing the other widgets to destroy without reinvoking do_quit
> and whacking whatever geometry data you may have saved before the
> widgets started to get deleted.
>
>
> The only problem I seem to have in git-gui is the window position
> opens about 10 pixels lower and 2 pixels to the right than the last
> time it opened.  I think there's a bug in Tk on Windows where the
> window position on the desktop doesn't include the titlebar when I
> get it, but expects to include it when I attempt to set it on the
> next start.
>
> I should note I also see the same behavior with my day-time-job's
> Java apps on Windows however, so I don't think its a specific Tk
> or git-gui issue.
>
>   

What you suggest is essentially what gitk does before my patch. In git-gui, routine do_quit is invoked when Tk is ready to destroy the top level window: this is _after_ Tk has destroyed everything in the window, and thus the problem. I still haven't isolated which object(s) being destroyed from gitk cause the geometry reported from wm info to change, clearly my patch is binding widgets that do not need to be bound, but I haven't found which specific one needs to be bound.

With my patch, the correct position is saved and restored on both Linux and Cygwin. I think that should be the goal. A more elegant solution accomplishing that end is of course desirable, but there remains the issue of finding it. I think the objections to my patch are more theoretical than practical: I seriously doubt you can find any practically observable side effect of binding all the widgets other than that the window geometry _is_ correctly saved and restored.

As to being specific to Tk, I have many windows applications that successfully save and restore window state, this geometry issue is not endemic to Windows and I have never encountered it except with Tk.

Mark

← back to recent threads