Opened 12 years ago

Last modified 4 months ago

#1621 assigned enhancement

Rewrite menu tracking using Mouse hooks

Reported by: jackburton Owned by: leavengood
Priority: low Milestone: Unscheduled
Component: Kits/Interface Kit Version:
Keywords: Cc: axeld@…, niels.egberts@…
Blocked By: Blocking: #3644, #4640
Has a Patch: yes Platform: All

Description

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.

Attachments (1)

menus.diff (27.1 KB ) - added by jackburton 12 years ago.
Patch against latest revision (22965), after changes in Menu.cpp

Download all attachments as: .zip

Change History (22)

comment:1 by jackburton, 12 years ago

Status: newassigned

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

comment:2 by jackburton, 12 years ago

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.

in reply to:  2 comment:3 by jackburton, 12 years ago

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.

comment:4 by anevilyak, 12 years ago

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

in reply to:  4 ; comment:5 by jackburton, 12 years ago

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

in reply to:  5 ; comment:6 by jackburton, 12 years ago

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.

in reply to:  6 ; comment:7 by jackburton, 12 years ago

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.

in reply to:  7 comment:8 by jackburton, 12 years ago

Cc: axeld@… added

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.

comment:9 by jackburton, 12 years ago

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 ?

comment:10 by diver, 12 years ago

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.

comment:11 by axeld, 12 years ago

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

comment:12 by anevilyak, 12 years ago

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

comment:13 by stippi, 12 years ago

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.

by jackburton, 12 years ago

Attachment: menus.diff added

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

comment:14 by jackburton, 12 years ago

Priority: normallow

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

comment:15 by anevilyak, 11 years ago

Blocking: 3644 added

(In #3644) Duplicate of ticket #1621.

comment:16 by anevilyak, 10 years ago

Blocking: 4640 added

(In #4640) Duplicate of ticket #1621.

comment:17 by NielsE, 10 years ago

Cc: niels.egberts@… added

comment:18 by jackburton, 10 years ago

Owner: changed from jackburton to nobody
Status: in-progressassigned

comment:19 by pulkomandy, 2 years ago

Component: User InterfaceKits/Interface Kit

comment:20 by pulkomandy, 22 months ago

Has a Patch: set

comment:21 by leavengood, 4 months ago

Owner: changed from nobody to 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.

Note: See TracTickets for help on using tickets.