wxSizer assertion with expand and center options

[moki]

Hi,

Since 3.1.0 version of wxWidgets a warning is triggered in debug when using Expand() and CenterHorizontal() at the same time in a vertical wxBoxSizer (and also with CenterVertical() and a horizontal wxBoxSizer):

assert "!(flags & (wxALIGN_RIGHT | wxALIGN_CENTRE_HORIZONTAL))" failed in DoInsert(): Horizontal alignment flags are ignored with wxEXPAND

But when setting a max size to a control, doing the latter has an effect.

Here a little example that produces a warning, but center the main_window:

wxDialog example_dlg(NULL, wxID_ANY, "example", wxDefaultPosition, wxSize(800, 600));

wxWindow *main_window = new wxWindow(&example_dlg, wxID_ANY);
main_window->SetBackgroundColour(*wxRED);
main_window->SetMaxSize(wxSize(400, wxDefaultCoord));

wxBoxSizer *main_sizer = new wxBoxSizer(wxVERTICAL);
main_sizer->Add(main_window, wxSizerFlags(1).Expand().CenterHorizontal());

example_dlg.SetSizer(main_sizer);
example_dlg.ShowModal();

If I remove .CenterHorizontal(), the warning goes away but the main_window is not centered anyway.
Obviously, I could use an horizontal sizer for this example, but in some cases it seems not to be the easiest way (because wxSizer do not have max size capabilities).

Shouldn't this warning be removed ?

Regards

[Vadim Zeitlin]

On Thu, 26 May 2016 01:22:56 -0700 (PDT) moki wrote:

m> Since 3.1.0 version of wxWidgets a warning is triggered in debug when using
m> Expand() and CenterHorizontal() at the same time in a vertical wxBoxSizer (and
m> also with CenterVertical() and a horizontal wxBoxSizer):
m>
m> assert "!(flags & (wxALIGN_RIGHT | wxALIGN_CENTRE_HORIZONTAL))" failed in
m> DoInsert(): Horizontal alignment flags are ignored with wxEXPAND

This message is meant to warn you that the flags you specify don't work as
you probably think they do.

m> But when setting a max size to a control, doing the latter has an effect.

Sorry, I don't understand what is "the latter" here?

m> Here a little example that produces a warning, but center the main_window:
m>
m> wxDialog example_dlg(NULL, wxID_ANY, "example", wxDefaultPosition,
m> wxSize(800, 600));
m>
m> wxWindow *main_window = new wxWindow(&example_dlg, wxID_ANY);
m> main_window->SetBackgroundColour(*wxRED);
m> main_window->SetMaxSize(wxSize(400, wxDefaultCoord));
m>
m> wxBoxSizer *main_sizer = new wxBoxSizer(wxVERTICAL);
m> main_sizer->Add(main_window, wxSizerFlags(1).Expand().CenterHorizontal());

How do you expand this to work? You can't both expand and centre the
window. If you want to centre it, just remove Expand().

m> If I remove .CenterHorizontal(), the warning goes away but the main_window
m> is not centered anyway.

Err, of course it isn't. Again, I am just totally failing to see how else
could it possibly behave.

m> Shouldn't this warning be removed ?

IMO the answer is definitely no, but I don't even understand the argument
for removing it. You will really need to explain what do you mean better if
you think there is something wrong with this assert.

Regards,
VZ

--
TT-Solutions: wxWidgets consultancy and technical support
http://www.tt-solutions.com/

[Eric Jensen]

Hello Vadim,

Thursday, May 26, 2016, 6:31:04 PM, you wrote:

VZ> On Thu, 26 May 2016 01:22:56 -0700 (PDT) moki wrote:

m>> wxDialog example_dlg(NULL, wxID_ANY, "example", wxDefaultPosition,
m>> wxSize(800, 600));
m>>
m>> wxWindow *main_window = new wxWindow(&example_dlg, wxID_ANY);
m>> main_window->SetBackgroundColour(*wxRED);
m>> main_window->SetMaxSize(wxSize(400, wxDefaultCoord));
m>>
m>> wxBoxSizer *main_sizer = new wxBoxSizer(wxVERTICAL);
m>> main_sizer->Add(main_window, wxSizerFlags(1).Expand().CenterHorizontal());

VZ> How do you expand this to work? You can't both expand and centre the
VZ> window. If you want to centre it, just remove Expand().

The point is: *If* a control is restricted by a maxmium size so that
it can't fill the whole width (or height) of a sizer, any alignment
flag in combination with EXPAND suddenly makes sense again.

Eric

[Vadim Zeitlin]

On Thu, 26 May 2016 19:21:56 +0200 Eric Jensen wrote:

EJ> Thursday, May 26, 2016, 6:31:04 PM, you wrote:
EJ>
EJ> VZ> On Thu, 26 May 2016 01:22:56 -0700 (PDT) moki wrote:
...
EJ> m>> wxBoxSizer *main_sizer = new wxBoxSizer(wxVERTICAL);
EJ> m>> main_sizer->Add(main_window, wxSizerFlags(1).Expand().CenterHorizontal());
EJ>
EJ> VZ> How do you expand this to work? You can't both expand and centre the
EJ> VZ> window. If you want to centre it, just remove Expand().
EJ>
EJ> The point is: If a control is restricted by a maxmium size so that
EJ> it can't fill the whole width (or height) of a sizer, any alignment
EJ> flag in combination with EXPAND suddenly makes sense again.

Ah, I finally see it, thanks Eric for the explanation. So the OP wants the
item to be _up_to_ (but not necessarily as wide as) 400px wide and centered
if the window width is greater than 400px, right?

If I understand the goal correctly now, I still don't think it would be
right to remove the assert to accommodate this use. IME specifying both
wxEXPAND and wxALIGN_CENTER_XXX together by mistake is much more likely
than doing it on purpose as in this case and the workaround with an extra
nested horizontal sizer is simple enough to put in place.

Sorry for not understanding the point initially and thanks again Eric for
explaining it,

[Eric Jensen]

Hello Vadim,

Thursday, May 26, 2016, 7:38:28 PM, you wrote:

VZ> On Thu, 26 May 2016 19:21:56 +0200 Eric Jensen wrote:

EJ>> Thursday, May 26, 2016, 6:31:04 PM, you wrote:
EJ>>
EJ>> VZ> On Thu, 26 May 2016 01:22:56 -0700 (PDT) moki wrote:

VZ> ...

EJ>> m>> wxBoxSizer *main_sizer = new wxBoxSizer(wxVERTICAL);
EJ>> m>> main_sizer->Add(main_window, wxSizerFlags(1).Expand().CenterHorizontal());
EJ>>
EJ>> VZ> How do you expand this to work? You can't both expand and centre the
EJ>> VZ> window. If you want to centre it, just remove Expand().
EJ>>
EJ>> The point is: If a control is restricted by a maxmium size so that
EJ>> it can't fill the whole width (or height) of a sizer, any alignment
EJ>> flag in combination with EXPAND suddenly makes sense again.

VZ> Ah, I finally see it, thanks Eric for the explanation. So the OP wants the
VZ> item to be _up_to_ (but not necessarily as wide as) 400px wide and centered
VZ> if the window width is greater than 400px, right?
Yes.


VZ> If I understand the goal correctly now, I still don't think it would be
VZ> right to remove the assert to accommodate this use. IME specifying both
VZ> wxEXPAND and wxALIGN_CENTER_XXX together by mistake is much more likely
VZ> than doing it on purpose as in this case and the workaround with an extra
VZ> nested horizontal sizer is simple enough to put in place.

Ok, fair enough. It is however worth mentioning that the current sizer
code - coincidentally or not - does the right thing in the above case.
It's just the Assert that gets in the way.

Eric



--

[Catalin]

VZ> If I understand the goal correctly now, I still don't think it would be
VZ> right to remove the assert to accommodate this use.

Maybe the assert condition can be modified. I think adding the following to it will do:

|| (item->IsWindow() && item->GetWindow()->GetMaxSize() != wxDefaultSize)

Regards,
C

[Vadim Zeitlin]

On Thu, 26 May 2016 18:28:19 +0000 (UTC) 'Catalin' via wx-users wrote:

Cvwu> VZ> If I understand the goal correctly now, I still don't think it would be
Cvwu> VZ> right to remove the assert to accommodate this use.
Cvwu>
Cvwu> Maybe the assert condition can be modified. I think adding the following to it will do:
Cvwu>
Cvwu> || (item->IsWindow() && item->GetWindow()->GetMaxSize() != wxDefaultSize)

I don't think it's a good idea. What if it's not a window but a sizer
containing a window with a max size? And nothing says that even if the
window does have a max size, the two flags have been used together
intentionally, it could still be a mistake.

To answer Eric's point, I'm pretty sure that the fact that it works
correctly now is just a fluke, the code wasn't written to handle both flags
in a sensible way and I believe I've changed the behaviour when adding
those asserts. In fact, I wonder if this also worked as intended in 2.8,
I'm far from being sure about it.

Regards,

[Catalin]

On Friday, May 27, 2016 12:09 AM, Vadim Zeitlin wrote:

On Thu, 26 May 2016 18:28:19 +0000 (UTC) 'Catalin' wrote:

Cvwu> || (item->IsWindow() && item->GetWindow()->GetMaxSize() != wxDefaultSize)

VZ> What if it's not a window but a sizer containing a window with a max size?

I think that is still ok. A sizer cannot have a max size so it will have to expand and the 2 flags would indeed make no sense. This situation would be managed between the window and it's immediate containing sizer, not between 2 sizers.


VZ> And nothing says that even if the window does have a max size, the two flags have been used together

intentionally, it could still be a mistake.

That's true, but then there is "showing an assert for a valid case" vs "not showing an assert for an invalid case". The former looks slightly better.

VZ> I'm pretty sure that the fact that it works correctly now is just a fluke, the code wasn't written to handle both flags in a sensible way

Then maybe this should be established first, considering grid sizers too ?


Regards,
C

[moki]

Thanks for your replies and sorry for my poor english.

A quick look in sizer.cpp shows that this particular case is correctly implemented for wxBoxSizer (in release 3.1.0), if I am not wrong :

        wxCoord minorSize = GetSizeInMinorDir(sizeThis);
        const int flag = item->GetFlag();
        if ( (flag & (wxEXPAND | wxSHAPED)) || (minorSize > totalMinorSize) )
        {
            // occupy all the available space if wxEXPAND was given and also if
            // the item is too big to fit -- in this case we truncate it below
            // its minimal size which is bad but better than not showing parts
            // of the window at all
            minorSize = totalMinorSize;

            // do not allow the size in the minor direction to grow beyond the max
            // size of the item in the minor direction
            const wxCoord maxMinorSize = GetSizeInMinorDir(item->GetMaxSizeWithBorder());
            if ( maxMinorSize >= 0 && minorSize > maxMinorSize )
                minorSize = maxMinorSize;
        }

        if ( flag & (IsVertical() ? wxALIGN_RIGHT : wxALIGN_BOTTOM) )
        {
            PosInMinorDir(posChild) += totalMinorSize - minorSize;
        }
        // NB: wxCENTRE is used here only for backwards compatibility,
        //     wxALIGN_CENTRE should be used in new code
        else if ( flag & (wxCENTER | (IsVertical() ? wxALIGN_CENTRE_HORIZONTAL
                                                   : wxALIGN_CENTRE_VERTICAL)) )
        {
            PosInMinorDir(posChild) += (totalMinorSize - minorSize) / 2;
        }

Regards,