These patches replace a suggested workaround from D11790.

After a couple of hours of debugging, I almost understand what's happening there. Check my commit messages for explanation. Initially I thought it would be easier but Qt seems to be buggy here.

Hopefully, after this fix we can finally merge the branch...

In D11829#238090, @rade wrote:

another QCloseEvent event for the submenu after ~200ms through QTimer
after which the active item is reset.

Can't have been easy figuring out that out!
Have we been extremely unlucky running in to three different Qt5 bugs developing this feature or is Qt5 just this buggy?

Well, this is not a surprise to me. We are controlling menus through the code + make usual interaction. They were designed for just mouse and keyboard interaction. It will be nice if someone reports all these bugs to Qt. If you have time, please do this.

I do have some serious reservations regarding this fix. Using Qtimer to restore the active action feels very much like deliberately making a race condition. But that might be due to my lack of understanding what the Qtimer event is that is triggering the removal of focus and how we know that our Qtimer event will run after it (e.g is Qtimer deterministic so if two events are scheduled for time 0, the event scheduled last will always run last or something). Could you tell me what/how the Qt bug works and why this fix fixes it without causing a race condition?

No race condition happens. UI is running in a single thread in Qt and perceptional parallelism is governed by a so called event loop. This is a good read. QTimer::singleShot(0, this, ...) is a pretty standard pattern to add an event to spontaneous event queue which runs after posted event queue we are currently in (reference). I haven't debugged Qt to find out who's resetting the focus, but I found it's done within the posted event queue when the third Close event is received.

I found this code quite hard too understand. As I had quite a bit number notes for how it should be changed I decided it was easier to give you patches to be applied on top of your patch rather then inline comments with vague directions as too what I wanted changed. All the patches should be applied on top of each other fyi.

First off, I feel that the if statements should be moved inside each other too more clearly show that one or the other will run

justmovedifstatment.patch2 KBDownload

I agree.

Secondly I don't like how _quicksearchMenu get's assigned values all over the place. This makes needlessly difficult too follow the logic of the code as all the if statements depend on the value of quicksearchmenu. As an example, when I first started reading this code I expected quicksearchmenu too be null after opening and closing a submenu since we didn't receive a new Qevent::show, I didn't realise it also got assigned a value whenever we searched.

{F5780757}

There is a logic here, it's not that I put it all over the place to make it just work. _quicksearchMenu is assigned in 2 cases:

1. Menu is shown, so we about to start searching.
2. Search is started - this is needed in case submenu is closed but Show event is not sent for parent menu as this menu is visible already.

_quicksearchMenu is reset when the tracked menu is closed. Pretty straightforward, IMO.

Your patch reduces number of lines but the logic will be lost.

and finally I feel that there are an excessive amount of debug statements than can be removed. An argument can be made too keep them until this feature is complete and ready to be merged, but they should definitely be removed before then

removeddebug.patch1 KBDownload

Debug prints are needed to see what's happening as we can't debug menus with a regular debugger in X. It may be useful for bug investigation on a user side later. I'll keep them as they support the logic I described above.

I found a bug. It happens with the original patch you submitted as well as with my patch.

2. Open menu
3. Hover the mouse over the 'popular urls' bookmark causing it to open, do not move mouse again after this.
4. use keyboard to close menu, e.g by pressing left arrow
5. type a letter that will match multiple bookmarks e.g 't' that matches 'TV' and 'Telemundo' Expected outcome: the bookmark 'Telemundo' gets highlighted Actual outcome: 'Popular urls' gets highlighted and the submenu opens again.

I confirm this minor issue. I'd say it's another bug in the library. The item under the cursor should not be highlighted. I don't think it's a blocker for the branch merge though.

To be clear, you patch should be working fine, this discussion is more about a taste. I prefer keeping the _quickSearchMenu as meaningful as possible. If we search or about to search in a menu, the var is tracking this menu, however when the menu is closed, we mark that we suspended tracking by setting the var to zero.

Checking if (_quickSearchMenu) is needed as we access memory by the _quickSearchMenu pointer. If we replace the condition with if (_quickSearchText().length() != 0) the first-time reader need to go through the entire code too and ensure that _quickSearchMenu != 0 only if _quickSearchText().length() != 0. Plus, later we may change some other logic and it will no longer be the case.

I appreciate that you take time to explain your way of thinking in detail. Discussions like this lead to a better code. I will wait for Martin or other devs to express their opinions. If I face no objection, I'll merge the current patch.

Thanks Martin! I've incorporated your suggestions.