Keyboard: fix shortcut activation and input focus issues#1256
Merged
scrubbbbs merged 14 commits intonomacs:masterfrom May 15, 2025
Merged
Keyboard: fix shortcut activation and input focus issues#1256scrubbbbs merged 14 commits intonomacs:masterfrom
scrubbbbs merged 14 commits intonomacs:masterfrom
Conversation
scrubbbbs
force-pushed
the
shortcut-context
branch
4 times, most recently
from
f6e2f2f to
f4eda83
Compare
Closed
scrubbbbs
force-pushed
the
shortcut-context
branch
from
f4eda83 to
3d84e5e
Compare
scrubbbbs
force-pushed
the
shortcut-context
branch
4 times, most recently
from
1192ec4 to
6dc0f13
Compare
scrubbbbs
force-pushed
the
shortcut-context
branch
2 times, most recently
from
b4e7f0d to
760ad04
Compare
scrubbbbs
force-pushed
the
shortcut-context
branch
from
760ad04 to
0681d26
Compare
leejuyuu
reviewed
Apr 3, 2025
Collaborator
|
I don't really use shortcuts so can't test very thoroughly, but the mentioned issues seems to have been fixed. |
leejuyuu
reviewed
Apr 4, 2025
scrubbbbs
force-pushed
the
shortcut-context
branch
from
0681d26 to
e03b071
Compare
Collaborator
Author
A good test is to bind the "About Nomacs" dialog shortcut to a reserved key (like up/down or left/right) and see if we can break anything. |
scrubbbbs
force-pushed
the
shortcut-context
branch
from
e03b071 to
68d4540
Compare
Shortcuts now get Window context, which makes them available all of the time unless their actions are disabled. Fixes: nomacs#599 Fixes: nomacs#653 Fixes: nomacs#994 Fixes: nomacs#1190 Fixes: nomacs#1198 Fixes: nomacs#1234
Not configurable and conflicts with the default Quick Look key.
When hiding/showing docks they tend to steal keyboard focus. Now require user to click in the widget in order to have input focus. This breaks tab navigation into dockwidgets, however tab navigation in general is not working properly right now and would be much more difficult to fix.
Resolves shortcut conflict with pageup/pagedown and also disallows viewport actions such as rename (F2) etc. which is fine as the viewport is obscured (thumbs view) or not applicable (settings,batch). Removes adding of actions as it has no effect.
Plugins require a dummy/stub menu item at startup because they are lazy-loaded. It seems there was an effort to do this but it was never used/finished. Seems to work fine now. Also removes widget context from plugin actions, no longer required.
Add menubar actions to main window so they work when the menu bar is hidden, regardless of the viewport state. Remove menubar actions from viewport, which was the hack to do this before and is no longer needed.
- disable undo/redo actions so plugins can have their own - disable copy/paste actions since they work on original image - workaround open new tab unloading the plugin w/o saving - workaround open settings/batch changes action enablement - prevent loading two plugins at the same time Fixes: nomacs#571
The change to use default shortcut context (Window context) solves our shortcut activation problems, however comes with some drawbacks: 1) widgets that have non-action keybinds like keyPressEvent() can have their keys overriden by a shortcut 2) actions that bind the same key that worked before, now get the "ambiguous shortcut" error from Qt To solve (1), DkShortcutEventFilter is added as an application event filter. Widgets which have known keybinds (reserved keys) are defined in a table and we can reserve additional keys or release the default keybinds wit DkShortcutEventFilter::reserveKeys() To solve (2), DkShortcutEventFilter also traps ambiguous shortcut events and resolves them by walking the widget heirarchy for a bound action. This is similar to Qt::WidgetWithChildrenShortcut, but will also walk *up* from the widget to its parent if nothing is found in itself or children. As an alternate solution to (2), DkActionEventFilter is added, which allows widgets to binding actions without going through Qt shortcut system, which will override any global actions/shortcuts. With these solutions users and plugins should be able to bind more keys than previously possible, and without having conflicts that break the application.
Notes buttons used to activate on esc/ctrl+enter whether focused or not. Now can only work when typing in notes/focused.
Reserve keys used by keyPressEvent() so shortcuts binding them will be overriden
Actions can have multiple keys assigned, check them all. Also do not consider disabled actions.
If explorer is visible, now-ambiguous "return" key resolves to explorer action instead of crop toolbar action. Fix this by copying crop toolbar actions so they have a higher priority when resolving conflicts.
- move a static_cast<> near its type assertion - remove semantic inversion of "enabled" argument to enableViewPortPluginActions() - remove some indentation - improve comments - fix shortcut conflicts in floating docks
scrubbbbs
force-pushed
the
shortcut-context
branch
from
68d4540 to
328ab88
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See commit messages.
Summary:
Shortcut activation problems can be fixed by removing
setShortcutContext(Qt::WidgetAndChidrenShortcut). This flag requires widget focus for the shortcut to activate. Which is why we have to click in the main view for most shortcuts to work. I have been able to remove most instances of this and it fixes most of the activation issues.However it leaves us with new issues to resolve:
There are Qt controls that use built-in keys that might be defined in a shortcut; these shortcuts need to be "overridden" explicitly, for example pressing the "e" key when explorer has focus should not hide the explorer, it should jump to the next file/folder starting with "e". Mostly this is QTreeView, and the fix is to add a handler for QEvent::ShortCutOverride.
These come from swidgets that define their own QAction outside of menus and/or toolbars. These are resolved by:
DkActionEventHandlerto bypass Qt shortcuts system for those actionsDkShortcutEventHandlerwhich has a method to resolve the ambiguity. In some case actions will have to be copied around to give them higher priority.For example control-Z in paint-on-image. This is address in two ways