Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow global keyboard shortcuts to override all other shortcuts by providing a special field #6735

Merged
merged 7 commits into from
Oct 7, 2022

Conversation

EvidentlyCube
Copy link
Contributor

@EvidentlyCube EvidentlyCube commented Jun 23, 2022

Here is my proposed alternate solution to #6705.

In order to make a global shortcut override other shortcuts add a field priority and set it to yes.

Todo:

  • Update the documentation for global keyboard shortcut
  • Ensure it works with simple editor
  • Ensure it works with codemirror editor

Testing:

Below is info about what scenarios I checked and with what tiddlers

Test Tiddlers:

title: Shortcut
tags: $:/tags/KeyboardShortcut
key: ctrl+l

<$action-log $$message="global"/>
<$checkbox field="priority" checked="yes" unchecked=""/> Priority = {{!!priority}}
title: test

<$keyboard actions="""<$action-log $$message="widget"/>""" key="ctrl+l">
<$edit-text field="test" placeholder="Has keyboard action"/>
</$keyboard>
<$edit-text field="test" placeholder="No keyboard action"/>

The shortcut is Ctrl+L. Clicking the checkbox in shortcut tiddler toggles priority between yes and an empty value.

Test cases:

  • With priority = "" pressing Ctrl+L:
    • Nothing focused: logs global in the console
    • EditText widget without KeyboardWidget focused: logs global in the console
    • EditText widget with a KeyboardWidget focused: logs widget in the console
    • Framed editor focused: Opens Create wikitext link dialog
    • Simple editor focused: Opens Create wikitext link dialog logs global in the console (that's actually how it worked before priority was introduced)
    • CodeMirror editor focused: Opens Create wikitext link dialog
  • With priority = "yes" pressing Ctrl+L:
    • Nothing focused: logs global in the console
    • EditText widget without KeyboardWidget focused: logs global in the console
    • EditText widget with a KeyboardWidget focused: logs global in the console
    • Framed editor focused: logs global in the console
    • Simple editor focused: logs global in the console
    • CodeMirror editor focused: logs global in the console

core/modules/keyboard.js Outdated Show resolved Hide resolved
core/modules/keyboard.js Outdated Show resolved Hide resolved
@pmario
Copy link
Contributor

pmario commented Jun 23, 2022

I am not 100% sure about the name "Primary". Perhaps "Overriding" would be better?

What about priority .. So we can use yes and may be a value in the future, if we need to. But IMO value would only make it more complex

@EvidentlyCube
Copy link
Contributor Author

EvidentlyCube commented Jun 24, 2022

Here is a review of the changes I've pushed:

  • I've removed the three handleKeydownEvent methods. Now there is only one and it accepts an optional second argument.
    • From what I know and found, the regular dispatch only ever passes one argument, the event object itself, so the actual handlers will always have the second argument as undefined which is falsey. Old callbacks will work as they did before.
  • The places which previously called handlePrimaryKeydownEvent now just call handleKeydownEvent(event, true)
  • primary renamed to priority and made react to being set to yes
  • Priority is now cached, rather than fetchign it on each keypress

@pmario Thanks for the review so far!

@pmario
Copy link
Contributor

pmario commented Jun 24, 2022

  • The places which previously called handlePrimaryKeydownEvent now just call handleKeydownEvent(event, true)

There is one thing left from my point of view. If we extend the function signature with additional options as you did with handleKeydownEvent(event, true) we typically use an options object. That makes it possible to add more options in the future, without the need to change the signature again.

A bit of inline docs is added with the info about the new options element. The init in the function is done with: options = options || {}; (see no var), to make sure that accessing the object elements doesn't throw an exception, if options is undefined.

Creating the object makes sure that you can directly use if (options.something === true), without a JS exception if the element is undefined.

like so:

TiddlyWiki5/boot/boot.js

Lines 2151 to 2159 in 0391e18

/*
path: path of wiki directory
options:
parentPaths: array of parent paths that we mustn't recurse into
readOnly: true if the tiddler file paths should not be retained
*/
$tw.loadWikiTiddlers = function(wikiPath,options) {
options = options || {};
var parentPaths = options.parentPaths || [],

@EvidentlyCube
Copy link
Contributor Author

EvidentlyCube commented Jun 24, 2022

@pmario Thanks, updated!


Edit: The below section was moved to the body of the PR.

Test Tiddlers:

title: Shortcut
tags: $:/tags/KeyboardShortcut
key: ctrl+l
<$action-log $$message="global"/>
<$checkbox field="priority" checked="yes" unchecked=""/> Priority = {{!!priority}}
title: test
<$keyboard actions="""<$action-log $$message="widget"/>""" key="ctrl+l">
<$edit-text field="test" placeholder="Has keyboard action"/>
</$keyboard>
<$edit-text field="test" placeholder="No keyboard action"/>

The shortcut is Ctrl+L. Clicking the checkbox in shortcut tiddler toggles priority between yes and an empty value.

Test cases:

  • With priority = "" pressing Ctrl+L:
    • Nothing focused: logs global in the console
    • EditText widget without KeyboardWidget focused: logs global in the console
    • EditText widget with a KeyboardWidget focused: logs widget in the console
    • Framed editor focused: Opens Create wikitext link dialog
  • With priority = "yes" pressing Ctrl+L:
    • Nothing focused: logs global in the console
    • EditText widget without KeyboardWidget focused: logs global in the console
    • EditText widget with a KeyboardWidget focused: logs global in the console
    • Framed editor focused: logs global in the console

@EvidentlyCube
Copy link
Contributor Author

I'll tackle updating the documentation for global shortcut later today.

@saqimtiaz
Copy link
Contributor

@EvidentlyCube we should also consider the need for consistency between the different editor engines provided by the core and core plugins, namely framed, simple and codemirror. The simple and codemirror engines would need to be extended with the same features.

@EvidentlyCube
Copy link
Contributor Author

@saqimtiaz I was pretty sure it already worked with simple editor. I'll check both with it and codemirror!

@EvidentlyCube
Copy link
Contributor Author

I'll check the test cases I've added to the PR description after putting the kid to sleep, and if everything works I'll move it from draft to real PR

@EvidentlyCube
Copy link
Contributor Author

All test cases have passed, moving it to "Ready for review"

@EvidentlyCube EvidentlyCube marked this pull request as ready for review July 6, 2022 19:20
@EvidentlyCube
Copy link
Contributor Author

Is there anything I could do to help getting this PR merged/reviewed/abandoned?

Copy link
Owner

@Jermolene Jermolene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience @EvidentlyCube. Looks good to me but I would appreciate @saqimtiaz's feedback too.

@saqimtiaz
Copy link
Contributor

@Jermolene this looks good to go now.

@Jermolene
Copy link
Owner

Thanks you @EvidentlyCube @saqimtiaz

@Jermolene Jermolene merged commit f33c7e2 into Jermolene:master Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants