From cd98a88c4d0730d8558e615e2baca00b4f1a9f19 Mon Sep 17 00:00:00 2001 From: Eva Marco Date: Mon, 7 Sep 2026 10:14:09 +0200 Subject: [PATCH] :bug: Fix errors on shortcuts (#11081) * :bug: Fix shift + character recording * :bug: Fix importing conflicts * :bug: Fix paste as non customizable shortcut * :bug: Fix duplicate manage of custom shortcuts * :bug: Fix json file name * :tada: Add search by commands * :bug: Fix CI --- frontend/packages/mousetrap/index.js | 335 ++++++++++-------- frontend/playwright/ui/pages/ShortcutsPage.js | 4 +- .../app/main/data/workspace/shortcuts.cljs | 1 + frontend/src/app/main/ui/hooks.cljs | 11 +- .../src/app/main/ui/settings/shortcuts.cljs | 38 +- frontend/src/app/main/ui/shortcuts.cljs | 64 +++- frontend/src/app/main/ui/shortcuts.scss | 4 + .../main/ui/workspace/sidebar/shortcuts.cljs | 10 +- .../ui/settings_shortcuts_test.cljs | 70 ++++ 9 files changed, 361 insertions(+), 176 deletions(-) diff --git a/frontend/packages/mousetrap/index.js b/frontend/packages/mousetrap/index.js index 12bcbab1b9..4d0576e417 100644 --- a/frontend/packages/mousetrap/index.js +++ b/frontend/packages/mousetrap/index.js @@ -30,29 +30,29 @@ const globalDocument = globalThis?.document; * @type {Object} */ var _MAP = { - 8: 'backspace', - 9: 'tab', - 13: 'enter', - 16: 'shift', - 17: 'ctrl', - 18: 'alt', - 20: 'capslock', - 27: 'esc', - 32: 'space', - 33: 'pageup', - 34: 'pagedown', - 35: 'end', - 36: 'home', - 37: 'left', - 38: 'up', - 39: 'right', - 40: 'down', - 45: 'ins', - 46: 'del', - 91: 'meta', - 93: 'meta', - 224: 'meta', - 219: '219' + 8: "backspace", + 9: "tab", + 13: "enter", + 16: "shift", + 17: "ctrl", + 18: "alt", + 20: "capslock", + 27: "esc", + 32: "space", + 33: "pageup", + 34: "pagedown", + 35: "end", + 36: "home", + 37: "left", + 38: "up", + 39: "right", + 40: "down", + 45: "ins", + 46: "del", + 91: "meta", + 93: "meta", + 224: "meta", + 219: "219", }; /** @@ -64,22 +64,22 @@ var _MAP = { * @type {Object} */ var _KEYCODE_MAP = { - 106: '*', - 107: '+', - 109: '-', - 110: '.', - 111 : '/', - 186: ';', - 187: '=', - 188: ',', - 189: '-', - 190: '.', - 191: '/', - 192: '`', - 219: '[', - 220: '\\', - 221: ']', - 222: '\'' + 106: "*", + 107: "+", + 109: "-", + 110: ".", + 111: "/", + 186: ";", + 187: "=", + 188: ",", + 189: "-", + 190: ".", + 191: "/", + 192: "`", + 219: "[", + 220: "\\", + 221: "]", + 222: "'", }; /** @@ -93,25 +93,25 @@ var _KEYCODE_MAP = { * @type {Object} */ var _SHIFT_MAP = { - '~': '`', - '!': '1', - '@': '2', - '#': '3', - '$': '4', - '%': '5', - '^': '6', - '&': '7', - '*': '8', - '(': '9', - ')': '0', - '_': '-', - '+': '=', - ':': ';', - '\"': '\'', - '<': ',', - '>': '.', - '?': '/', - '|': '\\' + "~": "`", + "!": "1", + "@": "2", + "#": "3", + $: "4", + "%": "5", + "^": "6", + "&": "7", + "*": "8", + "(": "9", + ")": "0", + _: "-", + "+": "=", + ":": ";", + '\"': "'", + "<": ",", + ">": ".", + "?": "/", + "|": "\\", }; /** @@ -124,12 +124,12 @@ var _SHIFT_MAP = { var globalNavigator = globalThis.navigator; var _SPECIAL_ALIASES = { - 'option': 'alt', - 'command': 'meta', - 'return': 'enter', - 'escape': 'esc', - 'plus': '+', - 'mod': /Mac|iPod|iPhone|iPad/.test(globalNavigator?.platform) ? 'meta' : 'ctrl' + option: "alt", + command: "meta", + return: "enter", + escape: "esc", + plus: "+", + mod: /Mac|iPod|iPhone|iPad/.test(globalNavigator?.platform) ? "meta" : "ctrl", }; /** @@ -146,14 +146,13 @@ var _REVERSE_MAP; * programatically */ for (var i = 1; i < 20; ++i) { - _MAP[111 + i] = 'f' + i; + _MAP[111 + i] = "f" + i; } /** * loop through to map numbers on the numeric keypad */ for (i = 0; i <= 9; ++i) { - // This needs to use a string cause otherwise since 0 is falsey // mousetrap will never fire for numpad 0 pressed as part of a keydown // event. @@ -176,7 +175,7 @@ function _addEvent(object, type, callback) { return; } - object.attachEvent('on' + type, callback); + object.attachEvent("on" + type, callback); } /** @@ -186,17 +185,16 @@ function _addEvent(object, type, callback) { * @return {string} */ function _characterFromEvent(e) { - // Numpad digits as "num0".."num9" — keeps them separate from main-row bindings across NumLock states and event types. - if (e.code && e.code.indexOf('Numpad') === 0) { + if (e.code && e.code.indexOf("Numpad") === 0) { var suffix = e.code.substring(6); - if (suffix.length === 1 && suffix >= '0' && suffix <= '9') { - return 'num' + suffix; + if (suffix.length === 1 && suffix >= "0" && suffix <= "9") { + return "num" + suffix; } } // for keypress events we should return the character as is - if (e.type == 'keypress') { + if (e.type == "keypress") { var character = String.fromCharCode(e.which); // if the shift key is not pressed then it is safe to assume @@ -225,6 +223,9 @@ function _characterFromEvent(e) { } // if it is not in the special map + if (typeof e.key === "string") { + return e.key.toLowerCase(); + } // with keydown and keyup events the character seems to always // come in as an uppercase character whether you are pressing shift @@ -240,7 +241,7 @@ function _characterFromEvent(e) { * @returns {boolean} */ function _modifiersMatch(modifiers1, modifiers2) { - return modifiers1.sort().join(',') === modifiers2.sort().join(','); + return modifiers1.sort().join(",") === modifiers2.sort().join(","); } /** @@ -253,19 +254,19 @@ function _eventModifiers(e) { var modifiers = []; if (e.shiftKey) { - modifiers.push('shift'); + modifiers.push("shift"); } if (e.altKey) { - modifiers.push('alt'); + modifiers.push("alt"); } if (e.ctrlKey) { - modifiers.push('ctrl'); + modifiers.push("ctrl"); } if (e.metaKey) { - modifiers.push('meta'); + modifiers.push("meta"); } return modifiers; @@ -308,7 +309,7 @@ function _stopPropagation(e) { * @returns {boolean} */ function _isModifier(key) { - return key == 'shift' || key == 'ctrl' || key == 'alt' || key == 'meta'; + return key == "shift" || key == "ctrl" || key == "alt" || key == "meta"; } /** @@ -321,7 +322,6 @@ function _getReverseMap() { if (!_REVERSE_MAP) { _REVERSE_MAP = {}; for (var key in _MAP) { - // pull out the numeric keypad from here cause keypress should // be able to detect the keys from the character if (key > 95 && key < 112) { @@ -344,17 +344,16 @@ function _getReverseMap() { * @param {string=} action passed in */ function _pickBestAction(key, modifiers, action) { - // if no action was picked in we should try to pick the one // that we think would work best for this key if (!action) { - action = _getReverseMap()[key] ? 'keydown' : 'keypress'; + action = _getReverseMap()[key] ? "keydown" : "keypress"; } // modifier keys don't work as expected with keypress, // switch to keydown - if (action == 'keypress' && modifiers.length) { - action = 'keydown'; + if (action == "keypress" && modifiers.length) { + action = "keydown"; } return action; @@ -367,12 +366,12 @@ function _pickBestAction(key, modifiers, action) { * @return {Array} */ function _keysFromString(combination) { - if (combination === '+') { - return ['+']; + if (combination === "+") { + return ["+"]; } - combination = combination.replace(/\+{2}/g, '+plus'); - return combination.split('+'); + combination = combination.replace(/\+{2}/g, "+plus"); + return combination.split("+"); } /** @@ -403,9 +402,9 @@ function _getKeyInfo(combination, action) { // if this is not a keypress event then we should // be smart about using shift keys // this will only work for US keyboards however - if (action && action != 'keypress' && _SHIFT_MAP[key]) { + if (action && action != "keypress" && _SHIFT_MAP[key]) { key = _SHIFT_MAP[key]; - modifiers.push('shift'); + modifiers.push("shift"); } // if this key is a modifier then add it to the list of modifiers @@ -421,7 +420,7 @@ function _getKeyInfo(combination, action) { return { key: key, modifiers: modifiers, - action: action + action: action, }; } @@ -510,7 +509,7 @@ function Mousetrap(targetElement) { doNotReset = doNotReset || {}; var activeSequences = false, - key; + key; for (key in _sequenceLevels) { if (doNotReset[key]) { @@ -537,7 +536,14 @@ function Mousetrap(targetElement) { * @param {number=} level * @returns {Array} */ - function _getMatches(character, modifiers, e, sequenceName, combination, level) { + function _getMatches( + character, + modifiers, + e, + sequenceName, + combination, + level, + ) { var i; var callback; var matches = []; @@ -549,7 +555,7 @@ function Mousetrap(targetElement) { } // if a modifier key is coming up on its own we should allow it - if (action == 'keyup' && _isModifier(character)) { + if (action == "keyup" && _isModifier(character)) { modifiers = [character]; } @@ -560,7 +566,11 @@ function Mousetrap(targetElement) { // if a sequence name is not specified, but this is a sequence at // the wrong level then move onto the next match - if (!sequenceName && callback.seq && _sequenceLevels[callback.seq] != callback.level) { + if ( + !sequenceName && + callback.seq && + _sequenceLevels[callback.seq] != callback.level + ) { continue; } @@ -577,15 +587,20 @@ function Mousetrap(targetElement) { // chrome will not fire a keypress if meta or control is down // safari will fire a keypress if meta or meta+shift is down // firefox will fire a keypress if meta or control is down - if ((action == 'keypress' && !e.metaKey && !e.ctrlKey) || _modifiersMatch(modifiers, callback.modifiers)) { - + if ( + (action == "keypress" && !e.metaKey && !e.ctrlKey) || + _modifiersMatch(modifiers, callback.modifiers) + ) { // when you bind a combination or sequence a second time it // should overwrite the first one. if a sequenceName or // combination is specified in this call it does just that // // @todo make deleting its own method? var deleteCombo = !sequenceName && callback.combo == combination; - var deleteSequence = sequenceName && callback.seq == sequenceName && callback.level == level; + var deleteSequence = + sequenceName && + callback.seq == sequenceName && + callback.level == level; if (deleteCombo || deleteSequence) { self._callbacks[character].splice(i, 1); } @@ -608,7 +623,6 @@ function Mousetrap(targetElement) { * @returns void */ function _fireCallback(callback, e, combo, sequence) { - // if this event should not happen stop here if (self.stopCallback(e, e.target || e.srcElement, combo, sequence)) { return; @@ -628,7 +642,7 @@ function Mousetrap(targetElement) { * @param {Event} e * @returns void */ - self._handleKey = function(character, modifiers, e) { + self._handleKey = function (character, modifiers, e) { var callbacks = _getMatches(character, modifiers, e); var i; var doNotReset = {}; @@ -644,14 +658,12 @@ function Mousetrap(targetElement) { // loop through matching callbacks for this key event for (i = 0; i < callbacks.length; ++i) { - // fire for all sequence callbacks // this is because if for example you have multiple sequences // bound such as "g i" and "g t" they both need to fire the // callback for matching g cause otherwise you can only ever // match the first one if (callbacks[i].seq) { - // only fire callbacks for the maxLevel to prevent // subsequences from also firing // @@ -668,7 +680,12 @@ function Mousetrap(targetElement) { // keep a list of which sequences were matches for later doNotReset[callbacks[i].seq] = 1; - _fireCallback(callbacks[i].callback, e, callbacks[i].combo, callbacks[i].seq); + _fireCallback( + callbacks[i].callback, + e, + callbacks[i].combo, + callbacks[i].seq, + ); continue; } @@ -700,12 +717,16 @@ function Mousetrap(targetElement) { // // we ignore keypresses in a sequence that directly follow a keydown // for the same character - var ignoreThisKeypress = e.type == 'keypress' && _ignoreNextKeypress; - if (e.type == _nextExpectedAction && !_isModifier(character) && !ignoreThisKeypress) { + var ignoreThisKeypress = e.type == "keypress" && _ignoreNextKeypress; + if ( + e.type == _nextExpectedAction && + !_isModifier(character) && + !ignoreThisKeypress + ) { _resetSequences(doNotReset); } - _ignoreNextKeypress = processedSequenceCallback && e.type == 'keydown'; + _ignoreNextKeypress = processedSequenceCallback && e.type == "keydown"; }; /** @@ -715,10 +736,9 @@ function Mousetrap(targetElement) { * @returns void */ function _handleKeyEvent(e) { - // normalize e.which for key events // @see http://stackoverflow.com/questions/4285627/javascript-keycode-vs-charcode-utter-confusion - if (typeof e.which !== 'number') { + if (typeof e.which !== "number") { e.which = e.keyCode; } @@ -730,7 +750,7 @@ function Mousetrap(targetElement) { } // need to use === for the character check because the character can be 0 - if (e.type == 'keyup' && _ignoreNextKeyup === character) { + if (e.type == "keyup" && _ignoreNextKeyup === character) { _ignoreNextKeyup = false; return; } @@ -761,7 +781,6 @@ function Mousetrap(targetElement) { * @returns void */ function _bindSequence(combo, keys, callback, action) { - // start off by adding a sequence level record for this combination // and setting the level to 0 _sequenceLevels[combo] = 0; @@ -774,7 +793,7 @@ function Mousetrap(targetElement) { * @returns {Function} */ function _increaseSequence(nextAction) { - return function() { + return function () { _nextExpectedAction = nextAction; ++_sequenceLevels[combo]; _resetSequenceTimer(); @@ -794,7 +813,7 @@ function Mousetrap(targetElement) { // we should ignore the next key up if the action is key down // or keypress. this is so if you finish a sequence and // release the key the final key will not trigger a keyup - if (action !== 'keyup') { + if (action !== "keyup") { _ignoreNextKeyup = _characterFromEvent(e); } @@ -814,7 +833,9 @@ function Mousetrap(targetElement) { // ones are better suited to the key provided for (var i = 0; i < keys.length; ++i) { var isFinal = i + 1 === keys.length; - var wrappedCallback = isFinal ? _callbackAndReset : _increaseSequence(action || _getKeyInfo(keys[i + 1]).action); + var wrappedCallback = isFinal + ? _callbackAndReset + : _increaseSequence(action || _getKeyInfo(keys[i + 1]).action); _bindSingle(keys[i], wrappedCallback, action, combo, i); } } @@ -829,15 +850,21 @@ function Mousetrap(targetElement) { * @param {number=} level - what part of the sequence the command is * @returns void */ - function _bindSingle(combination, callback, action, sequenceName, level, overwrite) { - + function _bindSingle( + combination, + callback, + action, + sequenceName, + level, + overwrite, + ) { // store a direct mapped reference for use with Mousetrap.trigger - self._directMap[combination + ':' + action] = callback; + self._directMap[combination + ":" + action] = callback; // make sure multiple spaces in a row become a single space - combination = combination.replace(/\s+/g, ' '); + combination = combination.replace(/\s+/g, " "); - var sequence = combination.split(' '); + var sequence = combination.split(" "); var info; // if this pattern is a sequence of keys then run through this method @@ -855,7 +882,14 @@ function Mousetrap(targetElement) { // remove an existing match if there is one if (overwrite) { - _getMatches(info.key, info.modifiers, {type: info.action}, sequenceName, combination, level); + _getMatches( + info.key, + info.modifiers, + { type: info.action }, + sequenceName, + combination, + level, + ); } // add this call back to the array @@ -864,13 +898,13 @@ function Mousetrap(targetElement) { // // this is important because the way these are processed expects // the sequence ones to come first - self._callbacks[info.key][sequenceName ? 'unshift' : 'push']({ + self._callbacks[info.key][sequenceName ? "unshift" : "push"]({ callback: callback, modifiers: info.modifiers, action: info.action, seq: sequenceName, level: level, - combo: combination + combo: combination, }); } @@ -882,16 +916,23 @@ function Mousetrap(targetElement) { * @param {string|undefined} action * @returns void */ - self._bindMultiple = function(combinations, callback, action, overwrite) { + self._bindMultiple = function (combinations, callback, action, overwrite) { for (var i = 0; i < combinations.length; ++i) { - _bindSingle(combinations[i], callback, action, undefined, undefined, overwrite); + _bindSingle( + combinations[i], + callback, + action, + undefined, + undefined, + overwrite, + ); } }; if (targetElement) { - _addEvent(targetElement, 'keypress', _handleKeyEvent); - _addEvent(targetElement, 'keydown', _handleKeyEvent); - _addEvent(targetElement, 'keyup', _handleKeyEvent); + _addEvent(targetElement, "keypress", _handleKeyEvent); + _addEvent(targetElement, "keydown", _handleKeyEvent); + _addEvent(targetElement, "keyup", _handleKeyEvent); } } @@ -909,7 +950,7 @@ function Mousetrap(targetElement) { * @param {string=} action - 'keypress', 'keydown', or 'keyup' * @returns void */ -Mousetrap.prototype.bind = function(keys, callback, action, overwrite) { +Mousetrap.prototype.bind = function (keys, callback, action, overwrite) { var self = this; keys = keys instanceof Array ? keys : [keys]; self._bindMultiple.call(self, keys, callback, action, overwrite); @@ -933,9 +974,9 @@ Mousetrap.prototype.bind = function(keys, callback, action, overwrite) { * @param {string} action * @returns void */ -Mousetrap.prototype.unbind = function(keys, action) { +Mousetrap.prototype.unbind = function (keys, action) { var self = this; - return self.bind.call(self, keys, function() {}, action); + return self.bind.call(self, keys, function () {}, action); }; /** @@ -945,10 +986,10 @@ Mousetrap.prototype.unbind = function(keys, action) { * @param {string=} action * @returns void */ -Mousetrap.prototype.trigger = function(keys, action) { +Mousetrap.prototype.trigger = function (keys, action) { var self = this; - if (self._directMap[keys + ':' + action]) { - self._directMap[keys + ':' + action]({}, keys); + if (self._directMap[keys + ":" + action]) { + self._directMap[keys + ":" + action]({}, keys); } return self; }; @@ -960,7 +1001,7 @@ Mousetrap.prototype.trigger = function(keys, action) { * * @returns void */ -Mousetrap.prototype.reset = function() { +Mousetrap.prototype.reset = function () { var self = this; self._callbacks = {}; self._directMap = {}; @@ -978,20 +1019,20 @@ Mousetrap.prototype.stopCallback = function (e, element, combo) { // if the element has the data attribute "mousetrap-dont-stop" then no need // to stop. It should be used like
...
// or :div {:data-mousetrap-dont-stop true} - if ('mousetrapDontStop' in element.dataset) { - return false + if ("mousetrapDontStop" in element.dataset) { + return false; } - if ((' ' + element.className + ' ').indexOf(' mousetrap ') > -1) { + if ((" " + element.className + " ").indexOf(" mousetrap ") > -1) { return false; } // Keyup events need to be dispatched always. Otherwise some events can be stuck - if (e.type == 'keyup') { + if (e.type == "keyup") { return false; } - if ('composedPath' in e && typeof e.composedPath === 'function') { + if ("composedPath" in e && typeof e.composedPath === "function") { // For open shadow trees, update `element` so that the following check works. const initialEventTarget = e.composedPath()[0]; if (initialEventTarget !== e.target) { @@ -1000,20 +1041,22 @@ Mousetrap.prototype.stopCallback = function (e, element, combo) { } // stop for input, select, textarea and button - const shouldStop = element.tagName == "INPUT" || - element.tagName == "SELECT" || - element.tagName == "TEXTAREA" || - (element.tagName == "BUTTON" && combo.includes("tab")) || - (element.contentEditable && (element.contentEditable == "true" || element.contentEditable === "plaintext-only")); + const shouldStop = + element.tagName == "INPUT" || + element.tagName == "SELECT" || + element.tagName == "TEXTAREA" || + (element.tagName == "BUTTON" && combo.includes("tab")) || + (element.contentEditable && + (element.contentEditable == "true" || + element.contentEditable === "plaintext-only")); return shouldStop; -} - +}; /** * exposes _handleKey publicly so it can be overwritten by extensions */ -Mousetrap.prototype.handleKey = function() { +Mousetrap.prototype.handleKey = function () { var self = this; return self._handleKey.apply(self, arguments); }; @@ -1028,7 +1071,7 @@ export function addKeycodes(object) { } } _REVERSE_MAP = null; -}; +} /** * Init the global mousetrap functions diff --git a/frontend/playwright/ui/pages/ShortcutsPage.js b/frontend/playwright/ui/pages/ShortcutsPage.js index 578311570e..d45bb1b970 100644 --- a/frontend/playwright/ui/pages/ShortcutsPage.js +++ b/frontend/playwright/ui/pages/ShortcutsPage.js @@ -318,7 +318,9 @@ export class ShortcutsPage extends BaseWebSocketPage { this.exportShortcuts(), ]); - expect(download.suggestedFilename()).toBe("penpot-shortcuts.json"); + expect(download.suggestedFilename()).toMatch( + /^penpot-shortcuts-Princesa_Leia-\d{4}-\d{2}-\d{2}\.json$/, + ); const path = await download.path(); const content = await readFile(path, "utf-8"); diff --git a/frontend/src/app/main/data/workspace/shortcuts.cljs b/frontend/src/app/main/data/workspace/shortcuts.cljs index 0923a24f80..5c042575a4 100644 --- a/frontend/src/app/main/data/workspace/shortcuts.cljs +++ b/frontend/src/app/main/data/workspace/shortcuts.cljs @@ -110,6 +110,7 @@ :command (ds/c-mod "v") :subsections [:edit] :section [:workspace] + :customizable false :fn (constantly nil)} :paste-replace {:tooltip (ds/meta (ds/shift "V")) diff --git a/frontend/src/app/main/ui/hooks.cljs b/frontend/src/app/main/ui/hooks.cljs index 83b86d932d..3a4e76edeb 100644 --- a/frontend/src/app/main/ui/hooks.cljs +++ b/frontend/src/app/main/ui/hooks.cljs @@ -43,13 +43,12 @@ (defn use-shortcuts [key shortcuts group-key] - (let [custom-shortcuts (mf/deref refs/custom-shortcuts)] - (mf/use-effect - #js [(str key) shortcuts custom-shortcuts] + (mf/use-effect + #js [(str key) shortcuts] + (fn [] + (st/emit! (dsc/push-shortcuts key shortcuts group-key)) (fn [] - (st/emit! (dsc/push-shortcuts key shortcuts group-key)) - (fn [] - (st/emit! (dsc/pop-shortcuts key))))))) + (st/emit! (dsc/pop-shortcuts key)))))) (defn- set-timer [state ms func] diff --git a/frontend/src/app/main/ui/settings/shortcuts.cljs b/frontend/src/app/main/ui/settings/shortcuts.cljs index 7800403adc..bb1ba9d383 100644 --- a/frontend/src/app/main/ui/settings/shortcuts.cljs +++ b/frontend/src/app/main/ui/settings/shortcuts.cljs @@ -93,10 +93,18 @@ ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; (def ^:private known-shortcut-keys - "Known shortcut keys per context, derived from the default shortcuts maps." - {:workspace (set (keys wsc/shortcuts)) - :dashboard (set (keys dsc/shortcuts)) - :viewer (set (keys vsc/shortcuts))}) + "Known shortcut keys per context, derived from the default shortcuts maps. + Shortcuts marked :customizable false are excluded." + (letfn [(collect-keys [shortcuts] + (reduce-kv (fn [s k v] + (if (false? (:customizable v)) + s + (conj s k))) + #{} + shortcuts))] + {:workspace (into (collect-keys psc/shortcuts) (collect-keys wsc/shortcuts)) + :dashboard (collect-keys dsc/shortcuts) + :viewer (collect-keys vsc/shortcuts)})) (def ^:private schema:imported-shortcuts "Malli schema for an imported custom-shortcuts payload. @@ -373,7 +381,8 @@ (mf/with-memo [] (fn [_ shortcut search-term] (or (str/blank? search-term) - (matches-search (:translation shortcut) search-term)))) + (matches-search (:translation shortcut) search-term) + (matches-search (ss/shortcut->command-string shortcut) search-term)))) filter-personalized (mf/use-fn @@ -385,8 +394,10 @@ customized? (and (contains? group-map shortcut-key) (not (str/blank? (get group-map shortcut-key))))] (and customized? + (not (false? (:customizable shortcut))) (or (str/blank? search-term) - (matches-search (:translation shortcut) search-term)))))) + (matches-search (:translation shortcut) search-term) + (matches-search (ss/shortcut->command-string shortcut) search-term)))))) filter-disabled (mf/use-fn @@ -398,8 +409,10 @@ in-group? (contains? group-map shortcut-key) blank? (str/blank? (get group-map shortcut-key))] (and in-group? blank? + (not (false? (:customizable shortcut))) (or (str/blank? search-term) - (matches-search (:translation shortcut) search-term)))))) + (matches-search (:translation shortcut) search-term) + (matches-search (ss/shortcut->command-string shortcut) search-term)))))) on-import-file (mf/use-fn @@ -414,11 +427,16 @@ on-export (mf/use-fn - (mf/deps shortcuts-json has-custom-shortcuts) + (mf/deps shortcuts-json has-custom-shortcuts (:fullname profile)) (fn [] (when has-custom-shortcuts - (->> (wapi/create-blob shortcuts-json "application/json") - (dom/trigger-download "penpot-shortcuts.json"))))) + (let [fullname (-> (or (:fullname profile) "user") + (str/replace #"[^a-zA-Z0-9\-_ ]" "") + (str/replace #"\s+" "_")) + date (.slice (.toISOString (js/Date.)) 0 10) + filename (str "penpot-shortcuts-" fullname "-" date ".json")] + (->> (wapi/create-blob shortcuts-json "application/json") + (dom/trigger-download filename)))))) on-file-selected (mf/use-fn diff --git a/frontend/src/app/main/ui/shortcuts.cljs b/frontend/src/app/main/ui/shortcuts.cljs index 212f296c95..47454fa2c8 100644 --- a/frontend/src/app/main/ui/shortcuts.cljs +++ b/frontend/src/app/main/ui/shortcuts.cljs @@ -10,9 +10,13 @@ [app.common.data :as d] [app.common.data.macros :as dm] [app.config :as cf] + [app.main.data.dashboard.shortcuts :as dsc] [app.main.data.dashboard.shortcuts.customize :as customize] [app.main.data.profile :as du] [app.main.data.shortcuts :as ds] + [app.main.data.viewer.shortcuts :as vsc] + [app.main.data.workspace.path.shortcuts :as psc] + [app.main.data.workspace.shortcuts :as wsc] [app.main.store :as st] [app.main.ui.context :as ctx] [app.main.ui.ds.buttons.icon-button :refer [icon-button*]] @@ -107,14 +111,24 @@ (def ^:private import-contexts [:workspace :dashboard :viewer]) +(def ^:private context->known-keys + {:workspace (into #{} (concat (keys psc/shortcuts) (keys wsc/shortcuts))) + :dashboard (into #{} (keys dsc/shortcuts)) + :viewer (into #{} (keys vsc/shortcuts))}) + +(defn- build-context-shortcuts + [all-shortcuts ctx] + (let [known-keys (get context->known-keys ctx)] + (into {} (filter (fn [[k _]] (contains? known-keys k))) all-shortcuts))) + (defn- import-context-group "Imports a single context group from the payload, disabling any default shortcut whose command collides with a newly imported one, and any previously-imported entry in the same batch with a duplicate command." - [group all-shortcuts] + [group context-shortcuts] (reduce (fn [acc [command recorded-command]] - (let [default-conflict (find-conflict recorded-command all-shortcuts command) + (let [default-conflict (find-conflict recorded-command context-shortcuts command) acc-conflict (some (fn [[k v]] (when (and (not= k command) (= v recorded-command)) k)) @@ -136,7 +150,8 @@ new-customs (reduce (fn [acc ctx] (if (contains? shortcuts ctx) - (assoc acc ctx (import-context-group (get shortcuts ctx) all-shortcuts)) + (let [ctx-sc (build-context-shortcuts all-shortcuts ctx)] + (assoc acc ctx (import-context-group (get shortcuts ctx) ctx-sc))) acc)) current-customs import-contexts)] @@ -147,6 +162,20 @@ [type item] (map (fn [[k v]] [k (assoc v :translation (translation-keyname type k))]) item)) +(defn shortcut->command-string + "Extract a lowercase searchable string from a shortcut entry's key combo(s). + Prefers `:show-command` (display override) over `:command` (Mousetrap format), + matching what the keycap UI renders. Joins vector commands (key sequences) + with a space so every token is searchable. Returns \"\" when there is no + command (e.g. a section/subsection node)." + [shortcut] + (let [cmd (or (:show-command shortcut) (:command shortcut))] + (-> (cond + (nil? cmd) "" + (vector? cmd) (str/join " " cmd) + :else (str cmd)) + (str/lower)))) + (defn shortcuts->subsections [shortcuts] (let [subsections (into #{} (mapcat :subsections) (vals shortcuts)) @@ -576,11 +605,19 @@ [{:keys [elements filter-term is-match-section is-match-subsection editable? custom-shortcuts section-key conflicts hidden subsection-name]}] (let [shortcut-translations (->> elements vals (map :translation) sort) - match-shortcut? (some #(matches-search % filter-term) shortcut-translations) + match-shortcut? (some (fn [info] + (or (matches-search (:translation info) filter-term) + (matches-search (shortcut->command-string info) filter-term))) + (vals elements)) filtered (if (and (or is-match-section is-match-subsection) (not match-shortcut?)) shortcut-translations - (filter #(matches-search % filter-term) shortcut-translations)) - sorted-filtered (sort filtered) + (->> (vals elements) + (filter (fn [info] + (or (matches-search (:translation info) filter-term) + (matches-search (shortcut->command-string info) filter-term)))) + (map :translation) + sort)) + sorted-filtered filtered trigger-ref (mf/use-ref nil)] [:ul {:class (stl/css :sub-menu) @@ -597,8 +634,9 @@ (get custom-shortcuts section-key)) group-map (if (map? group-map) group-map {}) customized? (contains? group-map command) - has-conflict? (contains? conflicts command)] - (if editable? + has-conflict? (contains? conflicts command) + customizable? (not (false? (:customizable command-info)))] + (if (and editable? customizable?) [:> shortcut-row-editable* {:elements elements :custom-shortcuts custom-shortcuts :section-key section-key @@ -611,9 +649,13 @@ :data-conflict (str has-conflict?) :aria-label command-translate :key command-translate} - [:span {:class (stl/css :command-name) - :id (dm/str command-translate "-label")} - command-translate] + [:span + [:span {:class (stl/css-case :command-name true + :not-customizable-label (not customizable?)) + :id (dm/str command-translate "-label")} + command-translate] + (when (not customizable?) + [:span {:class (stl/css :not-customizable-label)} "(not customizable)"])] [:div {:class (stl/css :shortcut-actions) :aria-labelledby (dm/str command-translate "-label")} (if (and customized? (str/blank? content)) diff --git a/frontend/src/app/main/ui/shortcuts.scss b/frontend/src/app/main/ui/shortcuts.scss index 4576fe8d50..11eef8a482 100644 --- a/frontend/src/app/main/ui/shortcuts.scss +++ b/frontend/src/app/main/ui/shortcuts.scss @@ -78,6 +78,10 @@ text-align: start; } +.not-customizable-label { + padding-inline-start: px2rem(6); +} + // Editable rows .shortcuts-name-editable { diff --git a/frontend/src/app/main/ui/workspace/sidebar/shortcuts.cljs b/frontend/src/app/main/ui/workspace/sidebar/shortcuts.cljs index 542d01aa83..3b36a79318 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/shortcuts.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/shortcuts.cljs @@ -66,7 +66,12 @@ (ss/build-all-shortcuts workspace-shortcuts dashboard-shortcuts viewer-shortcuts) all-item-names (concat all-sc-names all-sub-names all-section-names) - match-any? (some #(matches-search % filter-term) all-item-names) + all-command-strings (->> (concat (vals workspace-shortcuts) + (vals dashboard-shortcuts) + (vals viewer-shortcuts)) + (map ss/shortcut->command-string)) + all-searchable-names (concat all-item-names all-command-strings) + match-any? (some #(matches-search % filter-term) all-searchable-names) manage-sections (fn [item] @@ -89,7 +94,8 @@ (fn [section term] (let [node-seq (tree-seq :children #(vals (:children %)) (get all-shortcuts section))] (reduce (fn [acc node] - (if (matches-search (:translation node) term) + (if (or (matches-search (:translation node) term) + (matches-search (ss/shortcut->command-string node) term)) (add-ids acc node) acc)) [] diff --git a/frontend/test/frontend_tests/ui/settings_shortcuts_test.cljs b/frontend/test/frontend_tests/ui/settings_shortcuts_test.cljs index 8bbf5af6b2..ba668650d7 100644 --- a/frontend/test/frontend_tests/ui/settings_shortcuts_test.cljs +++ b/frontend/test/frontend_tests/ui/settings_shortcuts_test.cljs @@ -5,6 +5,7 @@ [app.main.ui.settings.restore-shortcuts-modal :as restore-modal] [app.main.ui.settings.shortcuts :as sut] [app.main.ui.shortcuts :as ui-shortcuts] + [app.util.strings :refer [matches-search]] [cljs.test :as t :include-macros true] [clojure.string :as str])) @@ -221,3 +222,72 @@ (let [result (restore-modal/extract-shortcut-keys :next-frame {} :viewer)] (t/is (nth result 3) "Should return a default command for :next-frame in :viewer context"))) + +;; --- shortcut->command-string + command-based search -------------------- +;; The search in both the settings shortcuts page and the workspace sidebar +;; matches shortcut entries by their translated name AND by their key-combo +;; string. `shortcut->command-string` (in `app.main.ui.shortcuts`) extracts the +;; searchable form from `:command`/`:show-command`; `matches-search` does the +;; case-insensitive substring match. These tests pin that contract so searching +;; e.g. "ctrl" surfaces every shortcut whose combo includes ctrl. + +(t/deftest shortcut->command-string-extracts-string-command + (t/testing "a plain string command is returned lowercased" + (t/is (= "ctrl+z" (ui-shortcuts/shortcut->command-string + {:command "ctrl+z"}))))) + +(t/deftest shortcut->command-string-joins-vector-command + (t/testing "a vector command (key sequence) is joined with spaces so every + token is individually searchable" + (t/is (= "g v" (ui-shortcuts/shortcut->command-string + {:command ["g" "v"]}))))) + +(t/deftest shortcut->command-string-prefers-show-command + (t/testing ":show-command (display override) wins over :command" + (t/is (= "shift+x" (ui-shortcuts/shortcut->command-string + {:command "ctrl+z" :show-command "shift+x"}))))) + +(t/deftest shortcut->command-string-empty-for-section-node + (t/testing "a node without :command/:show-command (e.g. a section or + subsection heading) yields an empty string so it never matches a + non-blank command search" + (t/is (= "" (ui-shortcuts/shortcut->command-string + {:translation "workspace"}))))) + +(t/deftest shortcut->command-string-lowercases + (t/testing "the result is lowercased so search is case-insensitive" + (t/is (= "ctrl+shift+z" (ui-shortcuts/shortcut->command-string + {:command "Ctrl+Shift+Z"}))))) + +(t/deftest command-search-matches-ctrl-prefix + (t/testing "searching 'ctrl' matches a shortcut whose command contains ctrl" + (let [shortcut {:command "ctrl+shift+s" + :translation "Save all"}] + (t/is (matches-search (ui-shortcuts/shortcut->command-string shortcut) + "ctrl"))))) + +(t/deftest command-search-does-not-match-when-command-lacks-term + (t/testing "searching 'alt' does not match a shortcut with no alt in its combo" + (let [shortcut {:command "ctrl+z" + :translation "Undo"}] + (t/is (not (matches-search (ui-shortcuts/shortcut->command-string shortcut) + "alt")))))) + +(t/deftest command-search-matches-key-sequence-vector + (t/testing "searching a single key in a key-sequence vector command matches" + (let [shortcut {:command ["g" "v"] + :translation "Group"}] + (t/is (matches-search (ui-shortcuts/shortcut->command-string shortcut) + "g"))))) + +(t/deftest search-matches-by-translation-or-command + (t/testing "a search term matches if it appears in either the translation or + the command string — the OR that the filter predicates use" + (let [shortcut {:command "ctrl+s" + :translation "Save"}] + ;; by translation + (t/is (or (matches-search (:translation shortcut) "save") + (matches-search (ui-shortcuts/shortcut->command-string shortcut) "save"))) + ;; by command + (t/is (or (matches-search (:translation shortcut) "ctrl") + (matches-search (ui-shortcuts/shortcut->command-string shortcut) "ctrl"))))))