Keyboard navigating context menu

[humdinger]

This is hrev33939.

Keyboard navigation of Tracker context menus is quite broken. At the moment not every CursorRight opens a submenu/folder, sometimes you have to press CursorDown for that. I don't think I have to go into the details of how it should work, just have the cursor keys follow the little arrow symbols to open submenu/folders.

Collapsing a submenu/folder with ESC is a good idea, IMO. Better than CursorLeft/Right.

After a bit of playing around with keyboard navigation, the context menu locks up and isn't redrawn. Clicking folders in Tracker still works, just the context menu is toast.

[Ziusudra]

For submenus not opening the only thing that makes any sense to me is that ​fLayout == B_ITEMS_IN_ROW is evaluated as true when it seems it shouldn't be.

I have this problem with the Applications and Preferences in home/config/be as well as bin, include and lib in boot/common with both Tracker and Deskbar.

In #5996 stippi reports having the problem with Move to but not Copy to with Tracker.

A possible busy loop was fixed in hrev36813 but I still get the occasional deadlock (thread unresponsive with no activity.)

[Ziusudra]

backtraces for deadlock

[Ziusudra]

Finally took a look at submenus not opening again and I was wrong.

It is in ​_OkToProceed() which gets called for longer submenus by ​AttachedToWindow(). For Applications and Preferences on the Leaf menu the if condition is true because the mouse is not over the respective menu item. For other folder submenus the condition is true because stickyMode is false (this is true for Applications and Preferences when viewed at home/config/be).

Though even if stickyMode were true they would still not be under the mouse. One way I can think to address that would be to add a fKeyDown member variable that would be added to that if condition.

[stippi]

I wonder what _OkToProceed() is supposed to prevent in the first place. Perhaps it's a condition that no longer applies and the check can be removed entirely?

[jackburton]

Replying to stippi:

I wonder what _OkToProceed() is supposed to prevent in the first place. Perhaps it's a condition that no longer applies and the check can be removed entirely?

It's part of the dynamic menu api used by Tracker & Deskbar menus. It's a way to add many items to a menu without blocking forever. Periodically that loop checks if the user clicked the mouse button, or moved the mouse elsewhere, as a way to stop the menu from loading.

[Ziusudra]

This patch adds the member variable fKeyDown which prevents _OkToProceed() from aborting because the mouse pointer is not over the item. If the user presses another key, fKeyDown will be set back to false so that _OkToProceed() can abort if the menu is still attaching.

This also sets stickyMode to true when opening a submenu with the keyboard. (Though I intend to find out why the dynamic menus are set sticky when using the mouse but not when using the keyboard.)

The interesting thing is that while testing this I have not had the menus lock up despite trying desperately.

Mouse navigation of menus is completely unaffected.

[stippi]

Have you checked sizeof(BMenu) to be the same as before? When adding new members to the class in such a way that it changes size, you will break binary compatibility. Due to padding of one byte sized members, the size may stay the same, but you need to check to make sure.

All in all, I find this solution rather ugly. Why not pass the key-down condition to the method that needs to know it instead?

[Ziusudra]

No, I have not, did not consider it.

The problem is that _OkToProceed () is called by AttachedToWindow() which can't be passed anything, AFAIK.

[stippi]

I don't understand why that would matter. Could fKeydown be true in this case? If I understand things correctly, KeyDown() can only be invoked on a BMenu that is already attached to a BWindow, since it's a hook method that is called when the BMenu receives a message, which it can only receive when it's attached. That in turn means that when _OkToProceed() is called from within AttachedToWindow(), fKeyDown could never be true. So _OkToProceed() could instead get a boolean parameter which defaults to false, and AttachedToWindow() would not pass anything and thus use the default value. From the other place, where fKeyDown would be true, you can then pass this parameter, overriding the default value.

[Ziusudra]

AttachedToWindow() calls the supermenu's _OkToProceed() which currently is only called from this one place.

I could move or copy the AddDynamicItem() loop from AttachedToWindow() to _Show() so that the submenu only gets attatched after it's built. That would allow passing via KeyDown() to _SelectItem() to subMenu->_Show().

[Ziusudra]

OK, hows this one look.

After verifying that _Show() was always called for menus, as it should be, I moved the AddDynamicItem() loop into _Show() before it calls BMenuWindow->AttachMenu(). This allows for passing the bool keyDown (which defaults to false if not passed) from KeyDown() through the chain into _OkToProceed().

This has the benefit that if user actions abort the submenu it never gets attached rather than attaching and detaching.

I tested everything I could think of involving mouse and keyboard navigation; all Tracker, Deskbar and application menus function as desired. All dynamic filesystem menus open with both keyboard and mouse.

And again, while testing I didn't get any lockups I would normally have gotten while using the keyboard, so that has at least been greatly reduced.

[stippi]

Sounds and looks good! Would be great to also get a comment from jackburton.

[Ziusudra]

I just realized another benefit; fAttachAborted is now only used in _Show() and is reset on each use, so it does not need to be a member variable.

I'm thinking we should rename the member to fReserved and change _Show() to use a local.

[stippi]

Sounds good to me.

[jackburton]

Replying to stippi:

Sounds and looks good! Would be great to also get a comment from jackburton.

Looks fine to me, at least in principle (haven't tested). The only problem I can think of is that someone implements a special kind of menu and overrides BMenu::Show(). In that case, I guess he wouldn't call the base version (while BMenu::AttachedToWindow() is safe to be called), since that would produce any kind of problems, so he would lose the dynamic menu feature.

[Ziusudra]

Replying to jackburton:

The only problem I can think of is that someone implements a special kind of menu and overrides BMenu::Show(). In that case, I guess he wouldn't call the base version (while BMenu::AttachedToWindow() is safe to be called), since that would produce any kind of problems, so he would lose the dynamic menu feature.

One thing I noticed before making this change was that if you used the mouse to open a long dynamic window and left the supermenu open, you could now open the submenu with the keyboard. So it seems AddDynamicItem() realizes the submenu has already been populated. Thus it should be possible to have _Show() run the AddDynamicItem loop when keyDown is passed true and have AttachedToWindow() also run the loop.

I'll try it out tommorow.

[stippi]

I think in fact it's more likely that a subclass forgets to call the base class AttachedToWindow(), than that it forgets to call Show(), since BMenu::Show() is actually mandatory, since otherwise the menu will simply not show at all. This mistake would be immediately effective, while the effects of not calling BMenu::AttachedToWindow() could be more subtle.

[Ziusudra]

Well, regardless, the newly attached patch works just as well as the previous.

AttachedToWindow() has been reverted and is unchanged from what's currently in trunk. _Show() runs it's own AddDynamicItem() loop when called from KeyDown(). Tracker's AddDynamicItem() does if fact keep track of if the menu has been built and won't rebuild it while the menu chain is still open. Deskbar uses Tracker for it's dynamic menus.

[jackburton]

I give it a green light. But regardless what I wrote in my previous comment, I like the first version more. Less code duplication anyway.

[jackburton]

Applied the patch (with some more changes) in hrev37261. Thanks!

[Ziusudra]

_Show() needs to pass keyDown to _AddDynamicItems() so that it can pass it to _OkToProceed().

[jackburton]

Replying to Ziusudra:

_Show() needs to pass keyDown to _AddDynamicItems() so that it can pass it to _OkToProceed().

Yeah, sorry, I applied the patch manually because I had many local changes and it didn't apply correctly. I'll do that as soon as I get home.

[Ziusudra]

The latest change looks good.

One more thing, AttachedToWindow() should set fAttachAborted true when _AddDynamicItems() returns true. Otherwise, _Show() will always act as if the attach succeeded.

Personally, I would rename the attachAborted locals to addAborted as it more clearly reflects their purpose, but that's not important.

[Ziusudra]

In _AddDynamicItems() the super assignments could done inside the if clause, as they are not needed if the menu has already been built, or was so short it built in one pass.

[jackburton]

Just wanted to say I didn't forget about this. I already fixed the two issues you pointed out, but currently I don't have access to my development pc. Will try to commit the code this weekend.

[jackburton]

Can we close this ticket now, or there is something else to do here ?

[Ziusudra]

We can close this and #4291.

[humdinger]

Thanks for fixing this[[BR]] There's one issue left, however: After you have navigated thru the submenus and found your target, and pressed RETURN to launch it, only the last submenu containing your target is closed. The whole "z-menu" of the parent submenus stay open until you move the mouse.