Rewrite menu tracking using Mouse hooks

[jackburton]

Menu tracking should be rewritten using MouseMoved, MouseDown and MouseUp hooks. This would clean up the code a lot, moreover we would get rid of the use of GetMouse() which is not the best solution. This would also simplify the implementation of menu keyboard navigation.

[jackburton]

I'm already working on it, locally I already have menus working using mouse hooks.

[jackburton]

I have a preliminary patch. I'm attaching it here so that other people can have a look.

Working: Menubars and popup menus (tracker ones, too). Not working: Menus inside a menufield Other issues: Sometimes menubars keep the focus even when menus are closed, other issues

Menu tracking now works like this: The menu tracking "loop" starts with BMenu/BMenuBar::_Track(), usually called from a spawned thread (just like the old implementation). Here, the root menu just tries to acquire a semaphore. This new implementation does all the work in the Mouse hooks, and when it's time to stop the menu tracking, _StopTracking() is called, using a pointer to the root menu (called fTrackingMenu, maintained for every menu). The only menu which sets the event mask to receive mouse events happening outside its bounds rectangle is the tracking menu. Since we need to do some things on MouseDown, when the mouse pointer is outside any menu, we need to keep track of the position of the mouse pointer vs the menus. To do that, a menu increments an int32 in the root menu when the mouse enters its bounds, and decrements its when it exits. Hope I didn't forget anything.

[jackburton]

Replying to jackburton:

Other issues: Sometimes menubars keep the focus even when menus are closed, other issues

This is fixed now. I removed special handling of menu windows in the app_server. Since we can use SetEventMask() and MouseMoved hooks, the special handling is no longer needed.

[anevilyak]

Another small issue: if, for instance, you right click on the desktop to get a context menu, clicking someplace else will not dismiss the menu unless you first mouse into the menu. I suspect that the top most menu window should probably have its event mask set after being constructed to deal with this, but I'm not sure right now if that would have any unwanted side effects (this would also imply the atomic counter would be initialized to 1).

[jackburton]

Replying to anevilyak:

Another small issue: if, for instance, you right click on the desktop to get a context menu, clicking someplace else will not dismiss the menu unless you first mouse into the menu.

Weird.

I suspect that the top most menu window should probably have its event mask set after being constructed to deal with this, but I'm not sure right now if that would have any unwanted side effects (this would also imply the atomic counter would be initialized to 1).

The event mask is set when Track() is called, which would be right after the menu is shown on screen. So it should already have the event mask set when you click on the desktop without hovering on it. I think the problem might be caused by another thing. But thank you, I'll have a look :)

[jackburton]

Replying to jackburton:

The event mask is set when Track() is called, which would be right after the menu is shown on screen. So it should already have the event mask set when you click on the desktop without hovering on it.

I can confirm this: The event mask is already set. But for some reason the menu view doesn't get the B_MOUSE_DOWN notification if you don't mouse first into the menu. I'll look into it.

[jackburton]

Replying to jackburton:

I can confirm this: The event mask is already set. But for some reason the menu view doesn't get the B_MOUSE_DOWN notification if you don't mouse first into the menu. I'll look into it.

More freaky: If you just mouse into a disabled item (the menu separator, for example), then move away from the menu, the menu won't still receive mouse messages happening outside its bounds. It will only starts receiving those when you mouse over an item which can be selected. This looks like an app_server bug.

[jackburton]

Replying to jackburton:

More freaky: If you just mouse into a disabled item (the menu separator, for example), then move away from the menu, the menu won't still receive mouse messages happening outside its bounds. It will only starts receiving those when you mouse over an item which can be selected. This looks like an app_server bug.

Found the problem. Since the command queue isn't flushed immediately, the SetEventMask() command doesn't reach the app_server until the menuwindow actually flushes the command queue due to, for example, an update request event. I added a Flush() after SetEventMask() (in BMenu::_Track()), but maybe we should do the flushing in SetEventMask() itself, since this could expose bugs in other places. Updated patch coming.

cc-ing to Axel, we'll see if filing a bug for this is needed.

[jackburton]

With this version, everything seems to work well enough. Except MenuFields, which I didn't even try to work on yet.

Some things to keep in mind: With this implementation, I ignored nonsticky menus. IOW: Menus are always sticky. Do we still have to support non sticky menus ? At least, I find it weird that with some menus you have to keep the button pressed and with some, there is no such requirement. We inherited this from beos, but AFAIK no other os behave in such a way. Opinions ?

[diver]

Personally i like sticky menus, i even asked titer to make this in BeOS version of VideoLan and 3deyes in NaviTracker and they implemented it.

[axeld]

I guess you mean only removing support for menus that can only be non-sticky, right? (ie. you can still use the menu in a non-sticky way, but they would always also work sticky.) At least you have my blessing, then :-)

[anevilyak]

There isn't really much alternative is there? Removing the actual options to set it at all would break bin compat I'd assume. I'd agree that basically allowing apps to set the sticky bit but ignoring that setting in practice would be a good idea, I tend to hate non-sticky menus :)

[stippi]

Absolutely agreed with aneilyak. Non-sticky menus drive me nuts. I don't even know, where there is a way to configure such a substantial menu behaviour on the app level. If it was a user setting... I'd be ok with it. But to have two kinds of menu behaviour?!?

Also, consider tablet users. It's a PITA to keep the secondary button pressed on a pen.

[jackburton]

Patch against latest revision (22965), after changes in Menu.cpp

[jackburton]

Oh well this might as well be postponed, since last changes to menu tracking...

[anevilyak]

(In #3644) Duplicate of ticket #1621.

[anevilyak]

(In #4640) Duplicate of ticket #1621.

[leavengood]

For the moment I am working on Haiku code again. I have had a menu triggers item in my bug list for some time (#3259) and when I mentioned that, waddlesplash pointed me to this ticket.

I mostly had to manually apply the patch because quite a bit has changed since it was created. I suppose even back then enough had changed to make Stefano give up. Nonetheless much of the ugly menu tracking code it was replacing is still there 12 years later, so I am going to try to merge these changes with other changes that have happened since then.

[X512]

Note that MouseDown, MouseUp, MouseMoved can't be overriden for BMenu because this methods was not overriden in BeOS, this breaks binary compatibility.

After hrev53446 (mouse and keyboard message handling and hooks calling was moved from BWindow to BView) B_MOUSE_DOWN/B_MOUSE_MOVED/B_MOUSE_UP messages can be handled in BMenu::MessageReceived.

It is also required to get mouse down event outside of BMenu to hide menu when user click outside menu.

[X512]

My WIP work: ​https://review.haiku-os.org/c/haiku/+/2284.