Tk Source Code

View Ticket
Login
Ticket UUID: d6b95ce49207c8231f1ac9cfd7391f0748c73ce1
Title: tk frame does not shrink to 1 height if last children unpacked
Type: Bug Version: 8.6.6
Submitter: oehhar Created on: 2016-09-20 13:01:56
Subsystem: 48. Geometry Management Assigned To: nobody
Priority: 5 Medium Severity: Important
Status: Closed Last Modified: 2016-11-07 07:53:56
Resolution: Wont Fix Closed By: oehhar
    Closed on: 2016-11-07 07:53:56
Description: (text/x-fossil-wiki)
<h1>Issue</h1>
An empty packed or gridded (tk/ttk/label)frame has zero height and width:
<verbatim>
pack [frame .f]
</verbatim>
When children are packed, it resizes to its size:
<verbatim>
pack [entry .f.e]
</verbatim>
When the last children is unpacked, the frame widget keeps the size of the last child:
<verbatim>
pack forget .f.e
</verbatim>
IMHO it should resize to 0 size.
IMHO this is a bug.
<h1>Workarounds</h1>
The page [http://wiki.tcl.tk/frame] contains some work-arounds.
<h1>Discussion</h1>
On the core list, there is a discussion thread about this issue: [http://code.activestate.com/lists/tcl-core/16363/]
with an explanatory post by Joe English.
<h1>Solutions</h1>
Emiliano has a solution sketch in this Ticket: [2863003fff].

Looking to the discussion, this feals like a broader issue.

Nevertheless, I find this issue quite annoying for dynamic setups and the workarounds all have issues not allowing a smooth solution.

Thank you all,
Harald
User Comments: oehhar added on 2016-11-07 07:53:56: (text/x-fossil-wiki)
In addition to the last message:
Koens post: [http://code.activestate.com/lists/tcl-core/16544/]
I did not find Brians post.

Now, the TIP is decided as withdrawn.
The issue may be fixed by patch in ticket [2863003f] (add an additional event).

I close this bug as discussion may continue there.

oehhar added on 2016-10-27 09:09:46: (text/x-fossil-wiki)
After the positive vote of TIP 454, the following issues are reported on the core list.

<h2>Koen Dankart: flicker</h2>

I have to say though that I'm getting less sure about this TIP. I found some comments in the code indicating that the old behaviour was not so much a design choice, but rather an implementation issue. Specifically, this comment in tkGrid.c:1735.
<verbatim>
    /*
     * If the master has no slaves anymore, then don't do anything at all:
     * just leave the master's size as-is. Otherwise there is no way to
     * "relinquish" control over the master so another geometry manager can
     * take over.
     */
</verbatim>
The current patch for TIP 454 bypasses this by doing the Tk_GeometryRequest() immediately instead of at idle time. The result is that another geometry manager can still take over, but it introduces some flickering (collapse + expand):
<verbatim>
  pack [text .t]
  pack forget .t; grid .t
  grid forget .t; pack .t
</verbatim>

<h2>Brian Griffin: Additional Configure-Event</h2>

It turns out that this "flicker" is also what is causing our tests to hang.  Our UI is a complex set of nested and tabbed panes where the implementation that manages the panes relies on a complex dance of <Configure>, <Map> and <Unmap> events to make the right things happen.  The consequence of adding one more <Configure> event is causing a hang at a [tkwait visibility $win] where $win never appears.  $win is a single child in a frame that is unmanaged and (re)managed based on various conditions.

The hang is easily solved, but that means that the behavior is different.  (the difference is not right or wrong, just different*)  This difference is also demonstrated in the textWind failures.  It could be asserted that these tk tests are confirming that the comment in the tkGrid.c has been faithfully implemented.

I cannot write just one piece of code that works in both 8.6.6 and 8.6.7.  I contend that this sort of thing is forbidden in a minor patch release, and questionable in a major patch relase (e.g. 8.7).  I'm struggling with this, but I think this kind of change might have to be deferred to 9.0.

-Brian

*: the change in our code means removing some code.  From a perspective of "less is more", the patch is "better".

<h2>Harald Oehlmann: test textWind-11.1 and textWind-11.2</h2>

New test failures:
<verbatim>
==== textWind-11.1 EmbWinDisplayProc procedure, geometry transforms FAILED
==== Contents of test case:

     .t insert 1.0 "Some sample text"
     pack forget .t
     place .t -x 30 -y 50
     frame .f -width 30 -height 20 -bg $color
     .t window create 1.12 -window .f
     update
     winfo geom .f

---- Result was:
1x1+0+0
---- Result should have been (exact matching):
30x20+119+55
==== textWind-11.1 FAILED



==== textWind-11.2 EmbWinDisplayProc procedure, geometry transforms FAILED
==== Contents of test case:

     .t insert 1.0 "Some sample text"
     pack forget .t
     place .t -x 30 -y 50
     frame .t.f -width 30 -height 20 -bg $color
     .t window create 1.12 -window .t.f
     update
     winfo geom .t.f

---- Result was:
1x1+0+0
---- Result should have been (exact matching):
30x20+89+5
==== textWind-11.2 FAILED
</verbatim>

<h3>Koens explanation</h3>

These 2 tests are failing because after the [pack forget] the window has collapsed to 1x1, as expected. The tests then use [place .t] to put the text widget back, but the [place] window manager does not propagate sizes to its master window. So, to  make the tests succeed again, the root window size should be configured before [place], e.g. with [. configure -width 400 -height 200].

<h2>Core List References</h2>
The core list archive does not have the messages jet, so:
   *   Title: RE: [TCLCORE] RE : CFV: TIP #454: Automatically Resize Frames After Last Child Removed
   *   Date: all 2016-10-26
   *   Timestamps (GMT): HaO: 8:44 , Koen: 15:53, Brian: 16:46

oehhar added on 2016-09-22 16:48:45: (text/x-fossil-wiki)
This bug is now subject of TIP 454 [http://tip.tcl.tk/454]

emiliano added on 2016-09-22 13:18:41:
I personally don't think the actual behaviour of the propagating
geometry managers or the container widgets are actual bugs, but more
like an overlook in the design.

The problem with the proposed solution of requesting a 1x1 (*) size after the
last managed widget goes away is that it sets a policy, not a mechanism.
People might want it to be left alone, or set to another geometry, or destroy
it, or ... you get the idea.

This is why I proposed the solution of sending a virtual event, so the
user can get a notification and do whatever it wants with the frame in a
few lines of code. I know is not a fully cooked idea, but IMHO is an step
in the right direction. Forcing the geometry to be 1x1 is as bad as what
we have now. Or worse, since it's a potential incompatibility.

oehhar added on 2016-09-22 09:17:49: (text/x-fossil-wiki)
Thank you Koen, you are just impressive !
Added to bug-branch by commit [bd3aa3c9].

danckaert added on 2016-09-22 08:49:44: (text/html)
Here is a revised patch. Apparently the check on DONT_PROPAGATE was not needed, since ALLOCED_MASTER is also not set in that case. In [grid] there are two places where the master can become empty.
<verbatim>
diff --git a/generic/tkGrid.c b/generic/tkGrid.c
index 2a88b76..8cb02c9 100644
--- a/generic/tkGrid.c
+++ b/generic/tkGrid.c
@@ -2780,6 +2780,7 @@ Unlink(
      */
 
     if ((masterPtr->slavePtr == NULL) && (masterPtr->flags & ALLOCED_MASTER)) {
+        Tk_GeometryRequest(masterPtr->tkwin, 0, 0);
        TkFreeGeometryMaster(masterPtr->tkwin, "grid");
        masterPtr->flags &= ~ALLOCED_MASTER;
     }
@@ -3507,6 +3508,7 @@ ConfigureSlaves(
      */
 
     if (masterPtr->slavePtr == NULL && masterPtr->flags & ALLOCED_MASTER) {
+        Tk_GeometryRequest(masterPtr->tkwin, 0, 0);
        TkFreeGeometryMaster(masterPtr->tkwin, "grid");
        masterPtr->flags &= ~ALLOCED_MASTER;
     }
diff --git a/generic/tkPack.c b/generic/tkPack.c
index 88a4b2d..f26712a 100644
--- a/generic/tkPack.c
+++ b/generic/tkPack.c
@@ -1363,6 +1363,7 @@ Unlink(
      */
 
     if (masterPtr->slavePtr == NULL && masterPtr->flags & ALLOCED_MASTER) {
+        Tk_GeometryRequest(masterPtr->tkwin, 0, 0);
        TkFreeGeometryMaster(masterPtr->tkwin, "pack");
        masterPtr->flags &= ~ALLOCED_MASTER;
     }
</verbatim>

oehhar added on 2016-09-21 14:50:22: (text/x-fossil-wiki)
'grid' works for me to:
<verbatim>
 % pack [frame .c]
% winfo height .c
1
% grid [entry .c.e]
% winfo height .c
19
% grid forget .c.e
% winfo height .c
1
</verbatim>

Great, thank you, Francois,
Harald

fvogel added on 2016-09-21 13:48:59:
Propagated Koen's patch from pack to the grid geometry manager.

See branch bug-d6b95ce492-alt

oehhar added on 2016-09-21 09:12:16: (text/x-fossil-wiki)
Sorry Koen, for pack it works correctly, thank you:
<verbatim>
% pack [frame .f]
% winfo height .f
1
% pack [entry .f.e]
% winfo height .f
19
% pack forget .f.e
% winfo height .f
1
</verbatim>

Thank you, that looks simple and great !
Harald

danckaert added on 2016-09-21 09:11:51: (text/html)
This was just a partial patch for [pack] alone. Try:
<verbatim>
  pack [frame .f]
  pack [entry .f.e]
  pack forget .f.e
</verbatim>

oehhar added on 2016-09-21 08:51:48: (text/x-fossil-wiki)
Koen,
I have tried your patch (commit [64fe74c3] in branch [bug-d6b95ce4]).

For me, the frame does not resize to 0x0 after this sequence:
<verbatim>
% pack [frame .f]
% grid [entry .f.e]
% grid forget .f.e
(</verbatim>

Sorry, but it does not work for me.

Thanks,
Harald

oehhar added on 2016-09-21 08:39:13: (text/x-fossil-wiki)
Koen Dankart contributed by private E-Mail:

The question is whether the current behaviour is a bug or not.
Some people say that it is not a bug because the frame loses its geometry manager after the last child is unpacked, and it keeps its last computed size. But the geometry manager might as well recompute the size as its last action before disconnecting from the frame. It think the latter behaviour would be less surprising than the current one, which many seem to consider as a bug indeed.

Now, if it is a bug, we can probably find a better fix than delivering a virtual event.
In think the following would do it for 'pack':

<verbatim>
diff --git a/generic/tkPack.c b/generic/tkPack.c
index 88a4b2d..80cf320 100644
--- a/generic/tkPack.c
+++ b/generic/tkPack.c
@@ -1363,6 +1363,9 @@ Unlink(
      */
 
     if (masterPtr->slavePtr == NULL && masterPtr->flags & ALLOCED_MASTER) {
+        if (!(masterPtr->flags & DONT_PROPAGATE)) {
+            Tk_GeometryRequest(masterPtr->tkwin, 0, 0);
+        }
        TkFreeGeometryMaster(masterPtr->tkwin, "pack");
        masterPtr->flags &= ~ALLOCED_MASTER;
     }
</verbatim>

oehhar added on 2016-09-21 07:25:39: (text/x-fossil-wiki)
Discussion now in ticket [2863003f].