Wrong wxAuiToolBar dropdown tool button behaviour

[RedTide]

It is not clear how dropdown buttons should work here, so I applied some changes in the auidemo to show the following issue:

In auibar.cpp:2419

        else if (item.m_kind == wxITEM_NORMAL)
        {
            // draw a regular button or dropdown button
            if (!item.m_dropDown)
                m_art->DrawButton(dc, this, item, item_rect);
            else
                m_art->DrawDropDownButton(dc, this, item, item_rect);
        }
        else if (item.m_kind == wxITEM_CHECK)
        {
            // draw either a regular or dropdown toggle button
            if (!item.m_dropDown)
                m_art->DrawButton(dc, this, item, item_rect);
            else
                m_art->DrawDropDownButton(dc, this, item, item_rect);
        }
        else if (item.m_kind == wxITEM_RADIO)
        {
            // draw a toggle button
            m_art->DrawButton(dc, this, item, item_rect);
        }

It seems that dropdown buttons can be added/removed only on tools with wxITEM_NORMAL and wxITEM_CHECK flags.
But trying to do so to wxITEM_RADIO, it 'works' anyway. Though we don't see the dropdown button, it's possible to click on it to show the menu (i.e. the dropdown button is invisible).

It seems that, when the dropdown is set and an eventhandler attached, both wxITEM_CHECK and wxITEM_RADIO work like regular wxITEM_NORMAL buttons. When a handler is not attached, they work as standard check/radio buttons, though the radiobutton still has an invisible dropdown.

So what should be done here? Do we need wxITEM_RADIO to show a dropdown too? (It's a strange feature to have, but as the code is now I don't see why it shouldn't be done.)

[RedTide]

The attached patch fixes the above issues and let to associate a dropdown button to wxITEM_NORMAL, wxITEM_CHECK and wxITEM_RADIO tool item kinds only.
It also include some changes to the auidemo sample to demonstrate how it works and related function documentation.

[vadz]

I think drop downs only make sense for normal buttons, not check/radio ones. IMO using drop down can only be confusing in this case.

So unless someone can think of an example when such functionality would be really useful, I'd prefer to forbid using dropdowns with check/radio items. Could you please update the patch to work like this? TIA!

[RedTide]

OK, I just did that way because it seems to me that it was a weird half implementation in current code, so after some suggestions I decided to add wxITEM_RADIO instead remove wxITEM_CHECK (allow a dropdown menu if 'item.IsButton()').

[RedTide]

Sorry Vadim but at this point, what can I do, just remove the other two types check or use wxITEM_DROPDOWN instead of wxITEM_NORMAL + bool m_dropdown? This maybe could break things in some user code and make unnecessary the changes in tbarbase.cpp in #14152 (I prefere just remove the other 2 kinds BTW, just asking to be sure also for the other ticket).

[vadz]

I don't know this code well so I can't be sure, but I think that removing m_dropDown and using wxITEM_DROPDOWN for such items would be indeed the most logical thing to do.

[RedTide]

Currently  SetHasDropDown(bool b) is used to switch the item from normal to dropdown type at runtime,  HasDropDown() is the getter to retrieve the current bool value, that's why wxITEM_NORMAL + m_dropDown are used instead.
So if we use wxITEM_DROPDOWN, keeping the current behaviour, we should replace the tool in the toolbar with the needed kind on the fly (not sure later if wxToolBar(Tool)Base permits to change the itemkind after creation...).

[RedTide]

I'm going to update this patch using wxITEM_DROPDOWN.
I suppose the result will be that previous users code will work but not displaying dropdown buttons at all, so they will have to change the item kind when calling AddTool() to get dropdown buttons to work again.
This mean also that HasDropDown will return just (m_kind == wxITEM_DROPDOWN)
Actually I have no idea about code needed to change the button kind at runtime to keep current behaviour recreating the button, so it will not be possible (at least for now), so SetHasDropDown() will do nothing ATM.
Let me know if this is OK, otherwise I'll limit my changes to use only wxITEM_KIND for dropdown buttons.

[vadz]

Indeed, it probably wouldn't be a good idea to allow changing the item kind dynamically at wxToolBarBase level as "real" wxToolBar is unlikely to support this on any platform (and this doesn't seem very useful anyhow).

So it would finally be better to do the minimal change and just check drop down support when the item is normal (and not check or radio).

[RedTide]

wxITEM_DROPDOWN replacement patch version

[RedTide]

Maybe is not so bad to do the right way where users have just to add/change the item kind to their code... :-P
(Otherwise I will update the simple version using wxITEM_NORMAL)

[vadz]

No, sorry, I'm afraid this is really bad as it's a silent breakage, i.e. the worst of its kind. We could consider removing SetHasDropDown() completely if it's not used much but I don't know how to check for this (Google Code Search, how I miss you...). So IMO it would be indeed better to just update the simple version.

[RedTide]

wxITEM_NORMAL patch version

[RedTide]

Original patch updated

[vadz]

Thanks, I'll apply this except for the very last change (addition of m_actionItem test) -- this seems to be unrelated to the rest and I don't know why is it needed, could you please explain? TIA!

[VZ]

(In [72944]) Disallow drop downs on AUI check buttons.

This doesn't make much sense and disallowing it makes check items consistent
with radio ones as previously you could set up a dropdown for the former but
not for the latter.

Also update the documentation of the relevant methods.

Closes #14795.

[RedTide]

Replying to vadz:

Thanks, I'll apply this except for the very last change (addition of m_actionItem test) -- this seems to be unrelated to the rest and I don't know why is it needed, could you please explain? TIA!

I noticed in some rare circumstances that a crash happens (I don't remember well if it happened when testing xrcdemo for wxAuiXmlHandler or testing #14152).
I'll add it eventually in #14152.