Opened 12 years ago

Last modified 16 months ago

#1621 assigned enhancement

Rewrite menu tracking using Mouse hooks

Reported by: jackburton Owned by: nobody
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 (21)

comment:1 Changed 12 years ago by jackburton

Status: newassigned

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

comment:2 Changed 12 years ago by 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.

comment:3 in reply to:  2 Changed 12 years ago by 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.

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

comment:5 in reply to:  4 ; Changed 12 years ago by 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 :)

comment:6 in reply to:  5 ; Changed 12 years ago by 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.

comment:7 in reply to:  6 ; Changed 12 years ago by 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.

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

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 Changed 12 years ago by 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 ?

comment:10 Changed 12 years ago by 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.

comment:11 Changed 12 years ago by 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 :-)

comment:12 Changed 12 years ago by 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 :)

comment:13 Changed 12 years ago by 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.

Changed 12 years ago by jackburton

Attachment: menus.diff added

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

comment:14 Changed 11 years ago by jackburton

Priority: normallow

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

comment:15 Changed 10 years ago by anevilyak

Blocking: 3644 added

(In #3644) Duplicate of ticket #1621.

comment:16 Changed 10 years ago by anevilyak

Blocking: 4640 added

(In #4640) Duplicate of ticket #1621.

comment:17 Changed 10 years ago by NielsE

Cc: niels.egberts@… added

comment:18 Changed 9 years ago by jackburton

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

comment:19 Changed 23 months ago by pulkomandy

Component: User InterfaceKits/Interface Kit

comment:20 Changed 16 months ago by pulkomandy

Has a Patch: set
Note: See TracTickets for help on using tickets.