Opened 8 months ago

Closed 7 months ago

#15475 closed defect (fixed)

Bug when inserting an empty sizer in a wxGridBagSizer

Reported by: briceandre Owned by:
Priority: normal Milestone:
Component: GUI-all Version: 2.9.5
Keywords: wxGridBagSizer empty sizer Cc:
Blocked By: Blocking:
Patch: no

Description

Dear all,

I think that I found a bug when inserting an empty sizer in a wxGridBagSizer. If the wxGridBagSizer does not contain anything else, there is a crash at the next Layout performed on the sizer or one of its parents.

I tried to investigate the problem and I think that I have some clue of where the problem is located : when performing the layout, the function performs first a CalcMin function call. This function does not take into account the empty sizer when performing the computation as this sizer is empty, and thus, it resets the number of rows and cols to zero.

But, then, when the function performs the recalcSize, there is a discrepancy because there is a child in the sizer and it does not match the number of cols and rows. So, there is first an assert followed by a crash.

I joined a small example that generates the bug. The dialog displays a button. When the dialog opens, it displays an empty wxGridBagSizer which does not cause any trouble. When one click on the button, an empty wxGridBagSizer is added to the existing one and thus, the layout function causes the crash.

I use a wxWidgets 2.9.5 compiled under Windows (MSVC-2010) in static unicode multi-lib version.

Regards,
Brice

Attachments (5)

sample.cpp download (1.4 KB) - added by briceandre 8 months ago.
Sample dialog that generates the bug
patch1.diff download (2.8 KB) - added by briceandre 8 months ago.
patch of file sample.cpp that generates first bug
patch2.diff download (2.7 KB) - added by briceandre 8 months ago.
patch of file sample.cpp that generates assert only
gbsizer.cpp.diff download (1.6 KB) - added by briceandre 8 months ago.
patch of gbsizer.cpp that seems to at least partially solve the problem
gbsizer.cpp_2.diff download (984 bytes) - added by briceandre 8 months ago.
The patch that I would advice to use to solve the bug.

Download all attachments as: .zip

Change History (11)

Changed 8 months ago by briceandre

Sample dialog that generates the bug

comment:1 Changed 8 months ago by vadz

  • Priority changed from critical to normal

It would be great if you could modify the existing layout sample and make a patch with the changes necessary to reproduce the bug.

comment:2 Changed 8 months ago by briceandre

OK, then I perform two versions of the layout sample :

  • in the first, I reproduced exactly the same behaviour as in my app : I removed everything from the grid-bag sizer and inserted an empty sizer in it. I then added a layout of the sizer and it crashes after displaying an assert message. The first patch corresponds to this version. To generate the bug, simply launch the app and display the wxGridBagSizer example (F4 shortcut or 'File'->'Test gridbag sizer').
  • in the second, I let the wxTextCtrl that can be hidden and I removed everything else. I slightly changed the layout so that the buttons used to hide and show the wxTextCtrl are no more in the wxGridBagSizer. If you display the wxGridBagSizer example (F4 shortcut or 'File'->'Test gridbag sizer'), everything works properly, but if you click on the button 'Hide this item', you will get the same assert message, but the application will not crash. The second patch corresponds to that version.

I tried to patch the file gbsizer.cpp in order to solve the problem. The patch 'gbsizer.cpp.diff' seems to solve part of the problem, but even with this patch, I still have a crash in my main application. I am currently not sure if the remaining crash is because the patch is invalid or if it is an unrelated remaining bug in my app.

Regards,
Brice

Changed 8 months ago by briceandre

patch of file sample.cpp that generates first bug

Changed 8 months ago by briceandre

patch of file sample.cpp that generates assert only

Changed 8 months ago by briceandre

patch of gbsizer.cpp that seems to at least partially solve the problem

comment:3 Changed 8 months ago by vadz

  • Component changed from base to GUI-all
  • Status changed from new to confirmed

Thanks for the patches, looking at the code I see that there is definitely a problem here because the number of elements in m_colWidths is not actually equal to the number of columns (and the same for rows). So I think your patch is correct, whatever other problems remain.

So what exactly is the problem if this patch is applied?

P.S. I'm not really familiar with this code so I could well be missing something here but I have to wonder why do we bother calling Set{Rows,Cols}() in wxGridBagSizer::Add() when we recompute them later anyhow...

comment:4 Changed 8 months ago by briceandre

OK, I found the problem of my second crash, and it has nothing to do with the current bug : I use a wxSizer::Clear(false) method that previously worked because I had only windows in my sizer, but now that I have sizers, it deletes the sizer, which causes my second bug. I think that the doc could be updated because, when you read the description of wxSizer::Clear, it's not clear that windows are not deleted whilst sizers are !

Returning back to the bug described in this ticket, I am still not convinced by the patch I sent to you : I can understand that I am the first guy to report the bug when inserting an empty sizer because this is not a common usage of this wxGridBagSizer. But, on the other hand, I assume that a lot of people use the Hide() method, and the patch I proposed changes the behaviour of the program in this case. As I assume that there is no bug in the Hide mechanism, changing the behaviour of the program in this case without understanding exactly the effect of the change may cause undesired side effect...

From what I read of the code, I have the feeling that m_cols and m_rows do not count the total number of columns and rows, but the total number of displayed ones. And, in the same way, I am pretty sure that m_colWidths and m_rowHeights only store info of displayed cols and rows.

The problem is that with the patch I sent, the function CalcMin now takes into account the hidden elements to update those variables and this is why I really fear a side effect of my patch...

On another way, with the original code, the crash did not occur in the CalcMin function : it occurred in the RecalcSizes.

What I was wondering is why this code does not crash when the sizer is really empty (which should be equivalent to the case where all contained elements are not displayed). If you look at the code of the RecalcSizes, you will see that this function exists immediately if m_children is empty.

What I guess is that this function should also exit immediately if m_children contains only elements that are not displayed.

I thus propose a new patch that performs what is described above. With this new patch, I do not encounter any problem, neither in my small tests examples, nor in my real application.

If there is nobody with a strong knowledge of the wxGridBagSizer internal stuff to give a better solution, I would highly recommend using this second patch instead of the first one as I really think there will be less risk to have side-effect with it.

Regards,
Brice

Changed 8 months ago by briceandre

The patch that I would advice to use to solve the bug.

comment:5 Changed 7 months ago by VZ

(In [74809]) Mention that wxSizer::Clear() always deletes child sizers.

The existence of "delete_windows" argument could mislead people into thinking
that sizers were not deleted neither when it had false value, see #15475.

comment:6 Changed 7 months ago by VZ

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

(In [74810]) Don't crash when laying out wxGridBagSizer with only hidden elements.

wxGridBagSizer lay out algorithm needs at least a single row and a single
column to work, so simply don't run it at all if there is nothing to lay out.

Closes #15475.

Note: See TracTickets for help on using tickets.