FancyMenu: better key navigation

[gfgit]

Closes #1986

I wonder if there is more standard Qt code to do this. Also are comments clear enough or does it need better explanation of edge cases?

[gfgit]

Actually, watched is always mSearchEdit. This is because inside constructor we have:

// Ensure all key presses go to search box
setFocusProxy(mSearchEdit);
mAppView->setFocusProxy(mSearchEdit);
mCategoryView->setFocusProxy(mSearchEdit);

Also keyboard navigation does not really move focus, just internally redirects keyboard events and does not cover tool buttons below.

[tsujan]

does not cover tool buttons below.

The focus is changed to them by the Tab key, and then they can be activated by the Space key. However, the user would expect Enter/Return should also work when the focus is changed to them by Tab. That needs to be fixed.

[tsujan]

To prevent any misunderstanding, I mean this:

It's good that the focus is changed to the buttons by Tab and they can be activated by Space. However, when those buttons are focused, Enter/Return should also activate them.

[gfgit]

Ok, I can set focus to buttons by pressing tab and activate them with space. The problem is buttons (at least with KDE Plasma theme) do not draw focused state so you have no hint of which button will be activated on space press

[tsujan]

The problem is buttons (at least with KDE Plasma theme) do not draw focused state

That's not your problem to solve: ) A Qt widget style should show the focus state of a button (like Kvantum does) — an LXQt theme should do that too.

[stefonarch]

It's somehow hard to understand where the focus actually is, with rectangles enabled in kvantum both categories and items have focus and before you press an arrow you don't know where you are.

Ideally the first item in the appview should be only activated when switching focus ~by tab or~ arrow key. If the cursor by chance is involved you can have even 3 "focused" items:

) A Qt widget style should show the focus state of a button (like Kvantum does) — an LXQt theme should do that too.

Well, I'm afraid no theme does this atm but this can easily be fixed by copying #FancyMenu QToolButton:hover { values to ... QToolButton:focus

[gfgit]

both categories and items have focus

Technically selected and focused/hoverd states are independent. But the style I think is drawing them in similar way so you can not distinguish them

[tsujan]

Ideally the first item in the appview should be only activated when switching focus by tab or arrow key

No, Tab should only switch the focus between buttons and search entry, as it already does. As @gfgit implicitly mentioned, arrow keys do not move the focus to the views; they just select view-items and set the "current" item. This behavior is OK because we need the search entry to remain focused as long as Tab isn't used.

[stefonarch]

But the style I think is drawing them in similar way so you can not distinguish them

I tried with #FancyMenu QListView::item:focus { to change colors for focussed item but I doesn't work at all, no idea why.

[tsujan]

But the style I think is drawing them in similar way so you can not distinguish them

Yes, that should be fixed in the style. It has nothing to do with the code.

[gfgit]

This behavior is OK because we need the search entry to remain focused as long as Tab isn't used.

We could switch focus also to item views because key press are sent to line edit also by the event filter

[stefonarch]

But the style I think is drawing them in similar way so you can not distinguish them

Yes, that should be fixed in the style. It has nothing to do with the code.

See comment above, with kvantum with 3 items selected as above there are still 2 with focus rectangles.

[tsujan]

I tried with #FancyMenu QListView::item:focus...

We can talk about it at lxqt-themes, instead of here (item:selected, item:hover).

[tsujan]

We could switch focus also to item views

There's no need to change or complicate a code that works so well. We just want to give more jobs to the arrow keys; that isn't related to giving the focus to the view.

[tsujan]

@gfgit I haven't reviewed the code yet, but I compiled and tested it. There's a usability issue:

When the user chooses a category by using arrows, the first item of the app view is selected. That's quite confusing because the user doesn't know what will happen if the up/down arrow is pressed.

Giving focus to the views is not the solution because some Qt styles may not distinguish it clearly enough. IMO, the solution is this:

1. When the up/down arrow works on categories, no item should be selected in the app view until the user presses the right/left arrow. In this way, and by seeing a selected app item, he/she can be sure that the up/down arrow will work in the app view.
2. After that, when the left/right arrow is pressed once, the app view item should be deselected. In this way the user knows that the up/down arrows will work in the categories view.
3. If the user selects a category by clicking or auto-selecting, the first item of the app view should be selected. Again the user knows that the up/down arrow will work in the app view.

I think there's no ambiguity in the above scheme.

[stefonarch]

That's exactly what I mentioned above, there is no style which can fix this.

Ideally the first item in the appview should be only activated when switching focus by arrow key.

[tsujan]

Oh, there's also a regression: the selected item in the app view isn't launched on pressing Enter anymore.

[tsujan]

That's exactly what I mentioned above

Sorry, then I misread your comment — I thought you said we should give the focus to the view....

Anyway, as I implied in the issue page, this is more about finding a logical way than coding. I think the logic is found now.

[gfgit]

Oh, there's also a regression: the selected item in the app view isn't launched on pressing Enter anymore.

I can not reproduce this. Pressing Enter works here. Maybe there is a difference between keyboard and keypad enter/return keys which are mapped differently in Qt enum.

Now while navigating categories, app view has no selection but pressing enter still opens invisibly selected app (usually first item when category has just changed). Did not test if auto select interferes with this PR

[gfgit]

Try now with the enter key

[tsujan]

You're almost there, but I'm afraid there are still UX issues:

1. When a category is selected by mouse (by clicking or auto-selecting), the first app isn't selected, while it is launched after pressing Enter/Return. That's quite confusing to the user.
2. When the up/down arrow works in the categories view, Enter/Return will launch the first app while it isn't (and shouldn't be) selected. Again, it's confusing.
3. When the search entry is cleared, there's no app selection. This isn't as important as the above cases, but, IMO, selecting the first app on clearing the search entry is more self-consistent.

Logically, 1 is fixed by selecting the first app on mouse selection (the user shouldn't need the right/left arrow after selecting a category by mouse), and 2 by not launching anything when there is no selected app yet.

EDIT: Please also read my suggestion in the next comment! It may be more self-consistent.

I'll start to review the code when the logic is fixed. Thanks for starting and working on this!

[tsujan]

After giving it more thought, I think another solution for the above-mentioned inconsistency is that, both cases of 1 and 2 could be treated in the same way, i.e., by not launching anything on pressing Enter/Return when no app is selected.

This approach is compatible with the current behavior, before this PR. It also may be cleaner on the coding level: except for the case of searching, no app is selected or launched by Enter/Return when just a category is selected, until the user uses mouse or the left/right arrow to select one.

@stefonarch, @gfgit, what do you think?

[gfgit]

This approach is compatible with the current behavior, before this PR. It also may be cleaner on the coding level: except for the case of searching, no app is selected or launched by Enter/Return when just a category is selected, until the user uses mouse or the left/right arrow to select one.

I've just tried with auto select giving focus to apps, it's just a few lines of code and seems good. I can push the commit so you can test it and in case we revert it

[tsujan]

I think now it has an ideal behavior and no other change is needed, but I'll test it more. Also, let's wait for @stefonarch.

[stefonarch]

Looks al real fine now (and I didn't noticed the enter behavior first), needs still https://github.com/lxqt/lxqt-themes/pull/112 for button focus - which can be merged IMO.

I've just tried with auto select giving focus to apps, it's just a few lines of code and seems good. I can push the commit so you can test it and in case we revert it

I think this would be confusing as it was first.

[tsujan]

OK, so everything is fine as regards functionality.

@gfgit, would you please squash all your 8 commits into one?

[gfgit]

Just curious what's the difference between github "squash merge" and squash/force push/merge?

[tsujan]

I don't know; I use the second one.

But please don't merge the PR itself; I want to take a look at the code later.

[tsujan]

Oh, if you mean GitHub's "Squash and merge" button, as far as I've seen, it merges the PR (which we don't want for now) without really squashing its commits — or at least I don't consider that as squashing.