From: Johannes Sixt <j6t@kdbg.org>
Date: Thu, 18 Sep 2025 17:57:58 GMT
Subject: Re: [PATCH] gitk: make the 'Tags and Heads' window geometry sticky
Message-ID: <3fd3f64f-6ab7-4b4a-a834-a9c5c1e89d5b@kdbg.org>
In-Reply-To: <20250913181153.46575-1-rappazzo@gmail.com>

Am 13.09.25 um 20:10 schrieb Michael Rappazzo:
> Currently, the Tags and Heads window always opens at a default position
> and size, requiring users to reposition it each time. 
> 
> This change saves and restores the Tags and Heads window size and position
> relative to the main gitk window. The geometry is stored in the config file
> as `geometry(showrefs)` and persists between gitk sessions. The window
> position is stored relative to the main window, so it maintains the same
> spatial relationship when the main window is moved or when gitk is restarted
> on different monitors.

Thank you for this patch. In general, I like the direction this is
going. I am baffled, though, by the sheer number of lines that must be
added to achieve the goal.

I tested the patch Windows and Linux (KDE), and found some deficiencies
on both.

During a session, on Windows, size and position are retained and on
Linux, only the size is retained (the position is always the default).
In both environments, persistence across session happens only when Gitk
is closed while the Tags and Heads window is open, but not when it is
closed via any of the means available.

To reduce the code a bit, would it be possible to set the global
geometry(showrefs) from a handler of the Configure event? Then it does
not have to be set on any other occasion.

> 
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
>  gitk | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 2 deletions(-)

> +proc parse_geometry {geom_string} {
> +    if {[regexp {^(\d+)x(\d+)\+(-?\d+)\+(-?\d+)$} $geom_string -> w h x y]} {
> +        return [list $w $h $x $y]
> +    }
> +    return {}
> +}

Are there any occasions where it is expected that the regular expression
does not match? If not, let's fail with an error. Then we don't have to
verify the return value at the call sites.

> +proc restore_showrefs_geometry {top} {
> +    global geometry
> +
> +    if {![info exists geometry(showrefs)] || ![winfo exists $top] || ![winfo exists .]} return
> +
> +    set saved_geom [parse_geometry $geometry(showrefs)]
> +    set main_geom [parse_geometry [wm geometry .]]
> +    if {[llength $saved_geom] == 4 && [llength $main_geom] == 4} {
> +        lassign $saved_geom w h rel_x rel_y
> +        lassign $main_geom mw mh mx my
> +
> +        set abs_x [expr {$mx + $rel_x}]
> +        set abs_y [expr {$my + $rel_y}]
> +
> +        # Ensure window stays on screen
> +        set screen_w [winfo screenwidth .]
> +        set screen_h [winfo screenheight .]
> +        if {$abs_x < 0} { set abs_x 0 }
> +        if {$abs_y < 0} { set abs_y 0 }
> +        if {$abs_x + $w > $screen_w} { set abs_x [expr {$screen_w - $w}] }
> +        if {$abs_y + $h > $screen_h} { set abs_y [expr {$screen_h - $h}] }

Consider the case where the stored height exceeds $screen_h. The title
bar is moved out of the screen due to the order of these corrections.
Let's correct the lower and right bounds first, and the upper and left
bounds second. Then the title bar stays on the screen.

> +
> +        wm geometry $top "${w}x${h}+${abs_x}+${abs_y}"
> +    }
> +    bind $top <Map> {}
> +}
-- Hannes