Opened 22 months ago

Closed 22 months ago

Last modified 22 months ago

#14795 closed defect (fixed)

Wrong wxAuiToolBar dropdown tool button behaviour

Reported by: RedTide Owned by:
Priority: low Milestone:
Component: wxAui Version: stable-latest
Keywords: wxAuiToolBar dropdown behaviour Cc: hanmac@…
Blocked By: Blocking:
Patch: yes

Description

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.)

Attachments (3)

auidemo.diff download (4.9 KB) - added by RedTide 22 months ago.
auibar_item_dropdown.diff download (10.6 KB) - added by RedTide 22 months ago.
wxITEM_DROPDOWN replacement patch version
auibar_dropdown.diff download (5.2 KB) - added by RedTide 22 months ago.
wxITEM_NORMAL patch version

Download all attachments as: .zip

Change History (18)

Changed 22 months ago by RedTide

comment:1 Changed 22 months ago by RedTide

  • Patch set

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.

comment:2 Changed 22 months ago by vadz

  • Priority changed from normal to low
  • Status changed from new to confirmed

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!

comment:3 Changed 22 months ago by 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()').

comment:4 Changed 22 months ago by 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).

comment:5 Changed 22 months ago by 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.

comment:6 Changed 22 months ago by Hanmac

  • Cc hanmac@… added

comment:7 Changed 22 months ago by 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...).

comment:8 Changed 22 months ago by 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.

comment:9 Changed 22 months ago by 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).

Changed 22 months ago by RedTide

wxITEM_DROPDOWN replacement patch version

comment:10 Changed 22 months ago by 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)

comment:11 Changed 22 months ago by 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.

Changed 22 months ago by RedTide

wxITEM_NORMAL patch version

comment:12 Changed 22 months ago by RedTide

Original patch updated

comment:13 follow-up: Changed 22 months ago by 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!

comment:14 Changed 22 months ago by VZ

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

(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.

comment:15 in reply to: ↑ 13 Changed 22 months ago by 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.

Note: See TracTickets for help on using tickets.