Bug: in PopupMenu::Window::selectNextItem()

#1

Hi,

Working with PopupMenu, I’ve found a bug in the current version of PopupMenu::Window::selectNextItem() checked from the git. The loop used to select the next item was written to select only items which can be triggered (which are activated). It does so by finding the current item, then looping to find a suitable item to select. In the worst case, the loop needs to scan all the items to find the current item, then scan all the items again to find the next item to select. Unfortunately, the current loop only go through the items once. That means a disabled item will be occasionally selected.

Reproducible: Always.
Step to reproduce:
Create a Popup menu. Add a disabled item, 3 enabled items, and a disabled item. Then use your keyboard to go up or down. The disabled items will be occasionally selected.
(Quick) Fix:
In PopupMenu::Window::selectNextItem(), instead of:

Write:

#2

Thanks - I’ll sort that out!

#3

Hi,

I’ve checked the last git commit. Thank you for the quick fix, it solves my issue.

Unfortunately, with this fix we lose the ability to cycle between the top and the bottom of the PopupMenu with the keyboard. It doesn’t matter much to me, but it’s something widely expected as all the popup menus I know can do it. Do you know about this regression?

#4

Is it widely expected? That’s not how menus work in OSX, but maybe under Windows (?) TBH I didn’t notice that the old code wrapped around, so any change in behaviour wasn’t deliberate - I’ll add it back, as it probably does make slightly more sense to wrap rather than stopping at the ends.

#5

Thank you again for the fix. I pulled it and it works.

I can confirm that the wrap around is present under both Windows and Linux (Qt and Gtk). I thought it was an universal expected behaviour, but then I didn’t know about MacOS menus.