diff --git a/src/app/modules/gui.cpp b/src/app/modules/gui.cpp index d0b034743..8455a2d61 100644 --- a/src/app/modules/gui.cpp +++ b/src/app/modules/gui.cpp @@ -680,27 +680,13 @@ void CustomizedGuiManager::onNewDisplayConfiguration(Display* display) bool CustomizedGuiManager::processKey(Message* msg) { - App* app = App::instance(); - const KeyContext currentCtx = KeyboardShortcuts::getCurrentKeyContext(); const KeyboardShortcuts* keys = KeyboardShortcuts::instance(); - const KeyContext contexts[] = { currentCtx, KeyContext::Normal }; - int n = (contexts[0] != contexts[1] ? 2 : 1); - - // Find best match (prefer the shortcut that matches the context first) - KeyPtr key = nullptr; - for (int i = 0; i < n; ++i) { - for (const KeyPtr& k : *keys) { - if (k->isPressed(msg, contexts[i]) && - (!key || - (key->keycontext() != currentCtx && match_key_context(k->keycontext(), currentCtx)))) { - key = k; - } - } - } + const KeyPtr key = keys->findBestKeyFromMessage(msg); if (!key) return false; // Cancel menu-bar loops (to close any popup menu) + App* app = App::instance(); app->mainWindow()->getMenuBar()->cancelMenuLoop(); switch (key->type()) { diff --git a/src/app/ui/key.cpp b/src/app/ui/key.cpp index 3e0206d95..4684515a1 100644 --- a/src/app/ui/key.cpp +++ b/src/app/ui/key.cpp @@ -295,6 +295,32 @@ namespace app { using namespace ui; +bool AppShortcut::fitsBetterThan(const KeyContext currentContext, + const KeyContext thisShortcutContext, + const KeyContext otherShortcutContext, + const AppShortcut& otherShortcut) const +{ + // Better context in the same source + if (otherShortcut.source == this->source && otherShortcutContext != currentContext && + thisShortcutContext == currentContext) + return true; + + // Better key source/level: User-defined > Extension-defined > App-defined + if (int(this->source) > int(otherShortcut.source) && (thisShortcutContext == currentContext || + // User-defined "Any" context overwrites all + // app-defined context + thisShortcutContext == KeyContext::Any)) + return true; + + // Normal > SelectionTool > Transformation + if ((currentContext == KeyContext::Transformation && + otherShortcutContext != KeyContext::Transformation && + thisShortcutContext == KeyContext::SelectionTool)) + return true; + + return false; +} + Key::Key(const Key& k) : m_type(k.m_type) , m_adds(k.m_adds) @@ -437,31 +463,58 @@ void Key::add(const ui::Shortcut& shortcut, const KeySource source, KeyboardShor } } +bool Key::fitsContext(const KeyContext keyContext) const +{ + // This key is for any context + if (m_keycontext == KeyContext::Any) + return true; + + // This key is for the same context + if (m_keycontext == keyContext) + return true; + + // Use Normal or SelectionTool keys in Transformation context + if (keyContext == KeyContext::Transformation && + (m_keycontext == KeyContext::SelectionTool || m_keycontext == KeyContext::Normal)) + return true; + + // Use Normal keys in SelectionTool or FramesSelection contexts + if ((keyContext == KeyContext::SelectionTool || keyContext == KeyContext::FramesSelection) && + (m_keycontext == KeyContext::Normal)) + return true; + + return false; +} + const AppShortcut* Key::isPressed(const Message* msg, const KeyContext keyContext) const { + const AppShortcut* best = nullptr; + if (const auto* keyMsg = dynamic_cast(msg)) { - for (const AppShortcut& shortcut : shortcuts()) { - if (shortcut.shortcut.isPressed(keyMsg->modifiers(), - keyMsg->scancode(), - keyMsg->unicodeChar()) && - (m_keycontext == KeyContext::Any || match_key_context(m_keycontext, keyContext))) { - return &shortcut; + if (fitsContext(keyContext)) { + for (const AppShortcut& shortcut : shortcuts()) { + if (shortcut.shortcut.isPressed(keyMsg->modifiers(), + keyMsg->scancode(), + keyMsg->unicodeChar()) && + (!best || shortcut.fitsBetterThan(keyContext, keycontext(), keycontext(), *best))) { + best = &shortcut; + } } } } else if (const auto* mouseMsg = dynamic_cast(msg)) { - for (const AppShortcut& shortcut : shortcuts()) { - if ((shortcut.shortcut.modifiers() == mouseMsg->modifiers()) && - (m_keycontext == KeyContext::Any || - // TODO we could have multiple mouse wheel key-context, - // like "sprite editor" context, or "timeline" context, - // etc. - m_keycontext == KeyContext::MouseWheel)) { - return &shortcut; + if (m_keycontext == KeyContext::Any || + // TODO we could have multiple mouse wheel key-context, + // like "sprite editor" context, or "timeline" context, + // etc. + m_keycontext == KeyContext::MouseWheel) { + for (const AppShortcut& shortcut : shortcuts()) { + if (shortcut.shortcut.modifiers() == mouseMsg->modifiers()) + return &shortcut; } } } - return nullptr; + return best; } const AppShortcut* Key::isPressed(const Message* msg) const diff --git a/src/app/ui/key.h b/src/app/ui/key.h index b2d98e922..aed0eb5d9 100644 --- a/src/app/ui/key.h +++ b/src/app/ui/key.h @@ -103,6 +103,8 @@ inline KeyAction operator&(KeyAction a, KeyAction b) return KeyAction(int(a) & int(b)); } +// This is a ui::Shortcut wrapper (just one key shortcut) defined by +// the app, an extension, or the user (KeySource). struct AppShortcut { KeySource source; ui::Shortcut shortcut; @@ -124,6 +126,13 @@ struct AppShortcut { ui::KeyModifiers modifiers() const { return shortcut.modifiers(); } ui::KeyScancode scancode() const { return shortcut.scancode(); } int unicodeChar() const { return shortcut.unicodeChar(); } + + // Returns true if this AppShortcut is better for the current + // context, compared to another shortcut. + bool fitsBetterThan(KeyContext currentContext, + KeyContext thisShortcutContext, + KeyContext otherShortcutContext, + const AppShortcut& otherShortcut) const; }; using AppShortcuts = ui::ShortcutsT; @@ -133,6 +142,8 @@ using KeyPtr = std::shared_ptr; using Keys = std::vector; using DragVector = base::Vector2d; +// A set of key shortcuts (AppShortcuts) associated to one command, +// tool, or specific action. class Key { public: Key(const Key& key); @@ -148,6 +159,8 @@ public: const AppShortcuts& delsKeys() const { return m_dels; } void add(const ui::Shortcut& shortcut, KeySource source, KeyboardShortcuts& globalKeys); + + bool fitsContext(KeyContext keyContext) const; const AppShortcut* isPressed(const ui::Message* msg, KeyContext keyContext) const; const AppShortcut* isPressed(const ui::Message* msg) const; bool isPressed() const; diff --git a/src/app/ui/key_context.h b/src/app/ui/key_context.h index 205773dbc..6b8f11067 100644 --- a/src/app/ui/key_context.h +++ b/src/app/ui/key_context.h @@ -11,28 +11,61 @@ namespace app { +// Specifies a possible App state/context where the user can press a +// key and associate different actions/commands in that specific +// context. Useful to associate the same key to different actions +// depending on the current context. +// +// This list should be sorted in such a way that more specific +// contexts are below: +// +// Normal > SelectionTool > Transformation +// +// This means that keys defined in SelectionTool are still valid in +// Transformation context. +// +// Other key context are just temporary in the Editor, like a mouse +// drag operation when we are transforming the +// selection. E.g. TranslatingSelection, ScalingSelection, etc. enum class KeyContext { + + // Special context to define keys. This is not used as a "current + // key context", but just to define keys for the whole range of key + // contexts. Any, + + // Regular context in a standby user state, the user is not dragging + // or clicking the mouse. Normal, + + // The user has a selection on the canvas and a selection-like tool + // (selection ink) selected. SelectionTool, + + // Special context to modify a specific action when the mouse is + // above the handles to translate/scale/rotate or is already + // translating/scaling/rotating the selection. TranslatingSelection, ScalingSelection, RotatingSelection, + + // Special context when the user has a specific tool selected to + // modify the tool behavior with a key: MoveTool, FreehandTool, ShapeTool, + + // When the mouse wheel is used above the editor. MouseWheel, + + // The user has a range of frames/cels selected in the timeline. FramesSelection, + + // The user has moved the selection and has yet to drop the pixels + // (the user can drop or undo the transformation). Transformation, }; -inline bool match_key_context(const KeyContext a, const KeyContext b) -{ - return (a == b) || (a == KeyContext::Any || b == KeyContext::Any) || - ((a == KeyContext::SelectionTool && b == KeyContext::Transformation) || - (a == KeyContext::Transformation && b == KeyContext::SelectionTool)); -} - } // namespace app #endif diff --git a/src/app/ui/keyboard_shortcuts.cpp b/src/app/ui/keyboard_shortcuts.cpp index c1cfefb05..de5e9194d 100644 --- a/src/app/ui/keyboard_shortcuts.cpp +++ b/src/app/ui/keyboard_shortcuts.cpp @@ -576,23 +576,46 @@ KeyContext KeyboardShortcuts::getCurrentKeyContext() return KeyContext::Normal; } +KeyPtr KeyboardShortcuts::findBestKeyFromMessage(const ui::Message* msg, + KeyContext currentKeyContext, + std::optional filterByType) const +{ + const KeyContext contexts[] = { currentKeyContext, KeyContext::Normal }; + int n = (contexts[0] != contexts[1] ? 2 : 1); + KeyPtr bestKey = nullptr; + const AppShortcut* bestShortcut = nullptr; + for (int i = 0; i < n; ++i) { + for (const KeyPtr& key : m_keys) { + // Skip keys that are not for the specific KeyType (e.g. only for commands). + if (filterByType.has_value() && key->type() != *filterByType) + continue; + + const AppShortcut* shortcut = key->isPressed(msg, contexts[i]); + if (shortcut && (!bestKey || shortcut->fitsBetterThan(currentKeyContext, + key->keycontext(), + bestKey->keycontext(), + *bestShortcut))) { + bestKey = key; + bestShortcut = shortcut; + } + } + } + return bestKey; +} + bool KeyboardShortcuts::getCommandFromKeyMessage(const ui::Message* msg, Command** command, Params* params, KeyContext currentKeyContext) { - const KeyContext contexts[] = { currentKeyContext, KeyContext::Normal }; - int n = (contexts[0] != contexts[1] ? 2 : 1); - for (int i = 0; i < n; ++i) { - for (KeyPtr& key : m_keys) { - if (key->type() == KeyType::Command && key->isPressed(msg, contexts[i])) { - if (command) - *command = key->command(); - if (params) - *params = key->params(); - return true; - } - } + KeyPtr key = findBestKeyFromMessage(msg, currentKeyContext, std::make_optional(KeyType::Command)); + if (key) { + ASSERT(key->type() == KeyType::Command); + if (command) + *command = key->command(); + if (params) + *params = key->params(); + return true; } return false; } diff --git a/src/app/ui/keyboard_shortcuts.h b/src/app/ui/keyboard_shortcuts.h index 0fbf93593..791bdb06c 100644 --- a/src/app/ui/keyboard_shortcuts.h +++ b/src/app/ui/keyboard_shortcuts.h @@ -12,6 +12,8 @@ #include "app/ui/key.h" #include "obs/signal.h" +#include + namespace tinyxml2 { class XMLElement; } @@ -60,6 +62,11 @@ public: static KeyContext getCurrentKeyContext(); + KeyPtr findBestKeyFromMessage( + const ui::Message* msg, + KeyContext currentKeyContext = KeyboardShortcuts::getCurrentKeyContext(), + std::optional filterByType = std::nullopt) const; + bool getCommandFromKeyMessage( const ui::Message* msg, Command** command, diff --git a/src/app/ui/keyboard_shortcuts_tests.cpp b/src/app/ui/keyboard_shortcuts_tests.cpp index 413fdd07e..1848fdd21 100644 --- a/src/app/ui/keyboard_shortcuts_tests.cpp +++ b/src/app/ui/keyboard_shortcuts_tests.cpp @@ -41,12 +41,18 @@ static KeyboardShortcuts* ks = nullptr; { \ KeyMessage msg(kKeyDownMessage, scancode, kKeyNoneModifier, 0, 0); \ Command* cmd = nullptr; \ - Params params; \ - EXPECT_TRUE(ks->getCommandFromKeyMessage(&msg, &cmd, ¶ms, keycontext)); \ + EXPECT_TRUE(ks->getCommandFromKeyMessage(&msg, &cmd, nullptr, keycontext)); \ ASSERT_TRUE(cmd != nullptr) << "command not found for key"; \ EXPECT_EQ(CommandId::commandId(), cmd->id()) << "other command found: " << cmd->id(); \ } +#define NO_COMMAND_FOR_KEY(scancode, keycontext) \ + { \ + KeyMessage msg(kKeyDownMessage, scancode, kKeyNoneModifier, 0, 0); \ + Command* cmd = nullptr; \ + EXPECT_FALSE(ks->getCommandFromKeyMessage(&msg, &cmd, nullptr, keycontext)); \ + ASSERT_TRUE(cmd == nullptr) << "command found for key: " << cmd->id(); \ + } TEST(KeyboardShortcuts, Basic) { ks->clear(); @@ -73,14 +79,28 @@ TEST(KeyboardShortcuts, KeyContexts) DEFINE_KEY(Cancel, kKeyEsc, KeyContext::Any); DEFINE_KEY(GotoPreviousFrame, kKeyLeft, KeyContext::Normal); + DEFINE_KEY(PlayAnimation, kKeyEnter, KeyContext::Normal); + DEFINE_KEY(Clear, kKeyBackspace, KeyContext::Normal); DEFINE_KEY(MoveMask, kKeyLeft, KeyContext::SelectionTool); + DEFINE_KEY(Apply, kKeyEnter, KeyContext::Transformation); + DEFINE_KEY(Cut, kKeyX, KeyContext::SelectionTool); EXPECT_COMMAND_FOR_KEY(Cancel, kKeyEsc, KeyContext::Normal); EXPECT_COMMAND_FOR_KEY(GotoPreviousFrame, kKeyLeft, KeyContext::Normal); EXPECT_COMMAND_FOR_KEY(MoveMask, kKeyLeft, KeyContext::SelectionTool); + EXPECT_COMMAND_FOR_KEY(PlayAnimation, kKeyEnter, KeyContext::Normal); + EXPECT_COMMAND_FOR_KEY(PlayAnimation, kKeyEnter, KeyContext::SelectionTool); + EXPECT_COMMAND_FOR_KEY(Apply, kKeyEnter, KeyContext::Transformation); + EXPECT_COMMAND_FOR_KEY(Clear, kKeyBackspace, KeyContext::Normal); + EXPECT_COMMAND_FOR_KEY(Clear, kKeyBackspace, KeyContext::SelectionTool); + EXPECT_COMMAND_FOR_KEY(Clear, kKeyBackspace, KeyContext::Transformation); + + NO_COMMAND_FOR_KEY(kKeyX, KeyContext::Normal); + EXPECT_COMMAND_FOR_KEY(Cut, kKeyX, KeyContext::SelectionTool); + EXPECT_COMMAND_FOR_KEY(Cut, kKeyX, KeyContext::Transformation); } -TEST(KeyboardShortcuts, UserDefined) +TEST(KeyboardShortcuts, UserDefinedPriority) { ks->clear(); @@ -89,6 +109,63 @@ TEST(KeyboardShortcuts, UserDefined) DEFINE_USER_KEY(Redo, kKeyZ, KeyContext::Normal); EXPECT_COMMAND_FOR_KEY(Redo, kKeyZ, KeyContext::Normal); + + DEFINE_KEY(MoveMask, kKeyLeft, KeyContext::SelectionTool); + DEFINE_USER_KEY(GotoPreviousFrame, kKeyLeft, KeyContext::Normal); + EXPECT_COMMAND_FOR_KEY(MoveMask, kKeyLeft, KeyContext::SelectionTool); + + DEFINE_USER_KEY(GotoPreviousFrame, kKeyLeft, KeyContext::Any); + EXPECT_COMMAND_FOR_KEY(GotoPreviousFrame, kKeyLeft, KeyContext::SelectionTool); +} + +TEST(KeyboardShortcuts, SpecificContextHasMorePriorityButNotIfItsUserDefined) +{ + ks->clear(); + DEFINE_KEY(Cancel, kKeyEsc, KeyContext::Any); + DEFINE_KEY(Undo, kKeyEsc, KeyContext::Transformation); + + // Pressing "Esc" in "Transformation" context should run "Undo", + // although "Cancel" is defined for "Any" context, a more specific + // context should have more priority. + EXPECT_COMMAND_FOR_KEY(Undo, kKeyEsc, KeyContext::Transformation); + + // But an user-defined key, even for Any context, will overwrite the + // app-defined shortcut in all contexts. + DEFINE_USER_KEY(Zoom, kKeyEsc, KeyContext::Any); + EXPECT_COMMAND_FOR_KEY(Zoom, kKeyEsc, KeyContext::Transformation); +} + +// Test that we can configure the Left key to always Undo when the +// default configuration says that the Left key does other actions in +// different contexts. +// +// Related issue: https://github.com/aseprite/aseprite/issues/5390 +TEST(KeyboardShortcuts, UndoWithLeftAndRight) +{ + ks->clear(); + + DEFINE_KEY(GotoPreviousFrame, kKeyLeft, KeyContext::Normal); + DEFINE_KEY(MoveMask, kKeyLeft, KeyContext::SelectionTool); + EXPECT_COMMAND_FOR_KEY(GotoPreviousFrame, kKeyLeft, KeyContext::Normal); + EXPECT_COMMAND_FOR_KEY(MoveMask, kKeyLeft, KeyContext::SelectionTool); + // "Transformation" is a sub-context of "Selection" context + EXPECT_COMMAND_FOR_KEY(MoveMask, kKeyLeft, KeyContext::Transformation); + + // Now we try defining the "Left" key for "Any" context overwriting it in all contexts. + DEFINE_USER_KEY(Undo, kKeyLeft, KeyContext::Any); + EXPECT_COMMAND_FOR_KEY(Undo, kKeyLeft, KeyContext::Normal); + EXPECT_COMMAND_FOR_KEY(Undo, kKeyLeft, KeyContext::SelectionTool); + EXPECT_COMMAND_FOR_KEY(Undo, kKeyLeft, KeyContext::Transformation); +} + +TEST(KeyboardShortcuts, FramesSelection) +{ + ks->clear(); + + DEFINE_KEY(LayerProperties, kKeyF2, KeyContext::Normal); + DEFINE_KEY(SetLoopSection, kKeyF2, KeyContext::FramesSelection); + EXPECT_COMMAND_FOR_KEY(LayerProperties, kKeyF2, KeyContext::Normal); + EXPECT_COMMAND_FOR_KEY(SetLoopSection, kKeyF2, KeyContext::FramesSelection); } int app_main(int argc, char* argv[])