Opened 6 years ago

Closed 4 years ago

#10008 closed defect (fixed)

Not using negative sizes in wxBoxSizer breaks AUI layout

Reported by: hartwigw Owned by: vadz
Priority: normal Milestone: 2.9.1
Component: GUI-all Version: stable-latest
Keywords: wxBoxSizer negative sizes Cc: jjn, biwillia76, dghart@…
Blocked By: Blocking:
Patch: no

Description

Assume the following code:

wxBoxSizer boxSizerPtr(new wxBoxSizer(wxHORIZONTAL));

boxSizerPtr->Add(firstWindow,0,0,0);
boxSizerPtr->AddStretchSpacer();
boxSizerPtr->Add(secondWindow,0,0,0);

this->SetSizer(boxSizerPtr);
...
Now, if the surrounding window's horizontal size is smaller than the size of its children, namely

this->GetSize().GetWidth() < firstWindow.GetSize().GetWidth()+secondWindow.GetSize().GetWidth()

than inside the box sizer's calculation method the stretch spacer's size becomes negative (at least temporarily). The width of the stretch spacer is adjusted later to zero but the remaining calculation is done with the previously calculated negative size.

Hartwig

Patch:

--- sizer_old.cpp 2008-05-08 09:02:13.000000000 +0200
+++ sizer_new.cpp 2008-09-28 10:03:12.000000000 +0200
@@ -1750,7 +1750,7 @@

item->SetDimension( child_pos, child_size );


  • pt.y += height;

+ pt.y += item->GetSize().GetHeight();

}
else
{

@@ -1788,7 +1788,7 @@

item->SetDimension( child_pos, child_size );


  • pt.x += width;

+ pt.x += item->GetSize().GetWidth();

}

}


Attachments (5)

boxsizer_diff.txt download (532 bytes) - added by hartwigw 6 years ago.
aui_problem.jpg download (116.7 KB) - added by jjn 6 years ago.
minimalText.diff download (1.0 KB) - added by dghart 4 years ago.
minimalTree.diff download (1.0 KB) - added by dghart 4 years ago.
box-delta.diff download (3.4 KB) - added by vadz 4 years ago.
Fix layout when there is not enough space for even min sizes

Download all attachments as: .zip

Change History (17)

Changed 6 years ago by hartwigw

comment:1 Changed 6 years ago by vadz

  • Resolution set to fixed
  • Status changed from new to closed

I think it's wrong to use negative delta at all in this code so I've solved it differently in r56010. Please let me know if you have any problems with this fix, thanks!

comment:2 Changed 6 years ago by jjn

  • Cc jjn added
  • Resolution fixed deleted
  • Status changed from closed to reopened

This fix breaks the layout system of wxAUI.
Run auidemo and drag the vertical toolbar to the bottom. (screenshot attached)

Changed 6 years ago by jjn

comment:3 Changed 6 years ago by vadz

  • Cc biwillia76 added
  • Patch unset
  • Status changed from reopened to confirmed
  • Summary changed from wxBoxSizer is calculating item's position incorrectly to Not using negative sizes in wxBoxSizer breaks AUI layout

The change still looks correct to me but it did indeed break wxAUI layout (interestingly enough, in different ways under MSW and GTK: under MSW the neighbouring pane keeps its size but starts encroaching on the status bar while under GTK it is resized when the toolbar is moved).

I can't find the problem myself but it seems that it is due to calling SetMinSize() with too big values somewhere, apparently because the code doesn't take into the account the fact that the toolbar size is smaller now that it doesn't show some buttons.

Ben, I'd be grateful for any help with this one because I am just too lost in wxAUI code. And I don't want to revert r56010 because it really seems like the correct thing to do.

TIA!

comment:4 Changed 6 years ago by jjn

The problem occurs in the vertical direction only. When you drag a horizontal toolbar to the right, the minimum width also exceeds the real one, but nothing happens. Therefore SetMinSize() might be not a real problem.

I think it is hard to fix the AUI layout algorithm, even if Ben is not busy :) Alternatively don't you add something like the flag that allows negative delta to wxBoxSizer?

comment:5 Changed 6 years ago by vadz

  • Component changed from GUI-all to wxAui
  • Milestone changed from 2.8.9 to 2.9.0
  • Version changed from 2.8.x to 2.9-svn

Thanks for this observation, it's indeed an argument against my theory. But now I'm even more puzzled as I don't understand where does this difference between vertical and horizontal directions come from.

As for the special flag: this would be awfully ugly. It really shouldn't be needed and I hope this can be fixed before the official 2.9.0 release to avoid the need for it.

comment:6 Changed 5 years ago by vadz

  • Milestone 2.9.0 deleted

Resetting 2.9.0 milestone as it's too late for it

comment:7 Changed 4 years ago by dghart

  • Cc dghart@… added
  • Component changed from wxAui to GUI-all

Hi Vadim,

This is in response to your answer in http://groups.google.co.uk/group/wx-users/browse_frm/thread/a456b54e2ea074f2. I'm posting here rather than in #11311 as the cause of the problem is actually #56010.

Both CodeLite and (to a lesser extent) 4Pane have been bitten by this patch; unfortunately I've failed fully to replicate their misbehaviour in a minimal sample. However here are two diffs to the 'minimal' sample that show related regressions since 2.8.

If you apply minimalTree.diff in 2.8 (or 2.9 with #56010 reverted), you'll see a normal expanded treectrl, with a scrollbar. In 2.9 the scrollbar is absent and never appears.

Apply minimalText.diff in 2.8 (or 2.9 with #56010 reverted) and then reduce the sample's height; both arrows stay visible until the textctrl disappears altogether. In 2.9, the bottom arrow disappears much earlier. In the sample, this just looks odd (and happens as a result of stupid behaviour). In the real-life instances of CodeLite and 4Pane it sometimes makes treectrls unusable, as it is impossible to scroll down to the bottom of the tree.

While I agree it's undesirable to have overlapping controls in a sizer, IMO it's at least as undesirable to break existing code in this way; especially as overlapping shouldn't happen in real code involving controls that can scroll, and #56010 doesn't distinguish between those that can and can't.

Would it not be possible to add another wxSizer flag to provide the #56010 effect for those who want it, with the 2.8 behaviour as the default?

Changed 4 years ago by dghart

Changed 4 years ago by dghart

comment:8 Changed 4 years ago by vadz

  • Milestone set to 2.9.1
  • Owner set to vadz
  • Status changed from confirmed to accepted

Thanks David for the patches reproducing the problem! I still have no idea how could r56010 break this as it seems obvious that we never want to deal with negative sizes but clearly it did and having these patches will make debugging how exactly this happened much easier. I'll try to look at this a.s.a.p.

comment:9 Changed 4 years ago by vadz

There is definitely a bug here but reverting r56010 is definitely not the right thing to do as the old behaviour was even more broken. I think the attached patch is the right fix for this problem and it does make David's examples to work correctly. And it also fixes AUI layout problems if I replace wxAuiProportionalBoxSizer with wxBoxSizer in src/aui/framemanager.cpp (otherwise these changes have no effect on wxAUI as it doesn't use wxBoxSizer at all any longer).

Please let me know if you find any problems with this patch.

TIA!

Changed 4 years ago by vadz

Fix layout when there is not enough space for even min sizes

comment:10 Changed 4 years ago by vadz

P.S. There is still a bug in wxTreeCtrl::GetBestSize() which returns huge sizes, it shouldn't do this at all -- but it's a different bug.

comment:11 Changed 4 years ago by dghart

Hi Vadim,

Both the 4Pane and the main CodeLite problem are fixed by box-delta.diff. Thanks!

As far as I can tell, the patch doesn't create any new issues (well, there is one, but that has a user-code cause).

From our experience, it should be safe to apply the patch.

comment:12 Changed 4 years ago by VZ

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [63704]) Ensure that size in the major direction of box sizer doesn't exceed the total.

After fixing the problem with "growing items by negative proportion" in r56010
(which still was the correct thing to do as it fixed such indefensibly broken
behaviour as shrinking items with larger proportion by more than "smaller"
items when there was not enough space) the items in a box sizer could become
larger than the total space allocated to the sizer resulting in only parts of
them being visible.

Fix this by truncating the items to the (remaining) total size even if this
means making them less than their minimal sizes -- because there is nothing
else we can do when the total space is smaller than the sum of minimal sizes
anyhow.

Closes #10008.

Note: See TracTickets for help on using tickets.