Clean up and refactor global shortcuts
What does this MR do and why?
This is the first MR towards #392845 (closed).
chore: Remove dead event listener
The .js-more-help-button
element was removed nearly three years ago in
!34660 (merged), so this
event listener will never get called.
chore: Remove dead code due to unused arguments
- The skipResetBindings constructor parameter was removed in 4ee29e89, so all related code can be removed.
- The ShortcutsEpic.openSidebarDropdown static method's only parameter was removed in !75302 (merged), so there's no point passing anything to it.
- The ShortcutsTestCase.openSidebarDropdown static method never expected any parameters since creation in !109527 (merged), so there's no point passing anything to it.
test: Improve Shortcuts specs
- Only instantiate Shortcuts once, since constructing it has side effects, and there's no teardown method.
- Spy on Mousetrap methods instead of mocking it entirely. It turns out that the stopCallback tests weren't passing for the right reason as a result. (There's a difference between the global Mousetrap instance and the one set up for Shortcuts#initMarkdownEditorShortcuts`.)
- Rename
shortcuts
variable to more explicitshortcutElements
.
refactor: Add and use Shortcut#bindCommand(s) methods
These encapsulate the logic to call keysFor and Mousetrap.bind. This will allow for simpler calls in Shortcuts and its subclasses.
They are instance methods rather than static methods to encourage access via the (in theory) singleton Shortcuts instance.
The call refactoring was partially implemented using this command:
comby "Mousetrap.bind(keysFor(:[[command]]), :[callback]);" \
"[:[command], :[callback]]," \
{ee/,}app/assets/javascripts/behaviors/shortcuts/*.js
See https://comby.dev/ for details about comby.
The actual calls to this.bindCommand(s)
were then done by hand, along
with some minor code rearrangement in the Shortcuts constructor.
refactor: Extract Mousetrap util file
This new ~/lib/utils/mousetrap file is similar to ~/lib/utils/axios_utils, in that it augments the raw package.
In particular, it makes it slightly easier to update Mousetrap's
stopCallback
method.
They Shortcut#addStopCallback
is an instance method rather than static
methods to encourage access via the (in theory) singleton Shortcuts
instance.
A problem this does not solve is the race between our overwriting of
Mousetrap#stopCallback
and the pause
plugin's overwriting of it:
https://github.com/ccampbell/mousetrap/blob/1.6.5/plugins/pause/mousetrap-pause.js#L10-L18.
If the pause
plugin is loaded first, then our subsequent overwrites
will no longer be subject to the paused status of Mousetrap.
The bulk import path rename was done using this command:
comby "import :[[name]] from 'mousetrap';" \
"import :[name] from '~/lib/utils/mousetrap';" \
{ee/,}{spec/frontend,app/assets/javascripts}/**/*.{js,vue} -i
See https://comby.dev/ for details about comby.
Finally, a redundant Mousetrap Jest mock has been removed, as the package has provided a CommonJS module since v1.5.0: https://github.com/ccampbell/mousetrap/releases/tag/1.5.0
Use named export
We do technically lint for default exports, but currently opt out in GitLab due of the sheer number of violations. Anyway, we might as well use a named export since we're wrapping Mousetrap.
Document Mousetrap pause plugin compatibility
Remove unnecessary method
The indirection serves no purpose, so just use the imported function directly.
Fix incorrect JSDoc type
Improve JSDoc types
- Document callback signature.
- There is no return value, so use void instead of undefined.
Improve description of addStopCallback
Screenshots or screen recordings
N/A, no changes.
How to set up and validate locally
- Press ? to open shortcuts help modal.
- Choose some shortcuts to test.
- Visit the relevant pages for them.
- Press the shortcuts.
- Ensure they do as advertised.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #392845 (closed)