-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add extended keyboard shortcut mode via ctrl + k for tool shortcuts #7112
Conversation
ensureNonConflictingHandlers(extendedSkeletonControls, extendedVolumeControls); | ||
const extendedAnnotationControls = { ...extendedSkeletonControls, ...extendedVolumeControls }; | ||
ensureNonConflictingHandlers(extendedAnnotationControls, extendedControls); | ||
extendedControls = { ...extendedControls, ...extendedAnnotationControls }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not so happy about these lines as they seem not easy to read, but I thought that still ensuring no conflicts makes sense and overweights the hard to read code lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not ideal, but okay in my opinion :)
b: () => setTool(AnnotationToolEnum.BRUSH), | ||
e: () => setTool(AnnotationToolEnum.ERASE_BRUSH), | ||
l: () => setTool(AnnotationToolEnum.TRACE), | ||
r: () => setTool(AnnotationToolEnum.ERASE_TRACE), | ||
f: () => setTool(AnnotationToolEnum.FILL_CELL), | ||
p: () => setTool(AnnotationToolEnum.PICK_CELL), | ||
q: () => setTool(AnnotationToolEnum.QUICK_SELECT), | ||
o: () => setTool(AnnotationToolEnum.PROOFREAD), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dieknolle3333 In order to define new "extended" shortcuts, just define them as regular keyboard shortcuts leaving out the ctrl + k
. But make sure these are included in their own object that is additionally passed to the InputKeyboardNoLoop
in the constructor. E.g. ctrl + k, 1
becomes simply 1
.
Then you need to create the InputKeyboardNoLoop
like this:
const extendedKeyboardBindings = { 1: () -> setBrushToPreset(PRESETS.1)};
new InputKeyboardNoLoop(normalKeyboardBindings, {options}, extendedKeyboardBindings);
This way pressing ctrl + k, 1
should call () -> setBrushToPreset(PRESETS.1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, I'll try that once it's merged :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@philippotto I assigned you to this review as Daniel already knows the "hacky"-way we implemented the "extended" keyboard mode using KeyboardJS. This pr should not take long to review (IMO #tm).
frontend/javascripts/libs/input.ts
Outdated
if (isExtendedCommand) { | ||
KeyboardJS.withContext("extended", () => { | ||
KeyboardJS.bind(...binding); | ||
}); | ||
} | ||
KeyboardJS.withContext("default", () => { | ||
KeyboardJS.bind(...binding); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to realize / implement the "extended keyboard mode" we use the context feature of KeyboardJS. This might not be the way this feature was intended but it works 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@philippotto I assigned you to this review as Daniel already knows the "hacky"-way we implemented the "extended" keyboard mode using KeyboardJS. This pr should not take long to review (IMO #tm).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great :) looks mostly good to me, but left some comments. will test after the next iteration.
ensureNonConflictingHandlers(extendedSkeletonControls, extendedVolumeControls); | ||
const extendedAnnotationControls = { ...extendedSkeletonControls, ...extendedVolumeControls }; | ||
ensureNonConflictingHandlers(extendedAnnotationControls, extendedControls); | ||
extendedControls = { ...extendedControls, ...extendedAnnotationControls }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not ideal, but okay in my opinion :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great! Only see my one comment. However, I think, that this is not really critical (and I'm not sure whether it's fixable in the first place)..
if (extendedCommands) { | ||
this.attach(EXTENDED_COMMAND_KEYS, this.toggleExtendedMode); | ||
// Add empty callback in extended mode to deactivate the extended mode via the same EXTENDED_COMMAND_KEYS. | ||
this.attach(EXTENDED_COMMAND_KEYS, _.noop, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work for you? If I hit CTRL + K twice, Chrome focuses my search bar on the second hit. I'm releasing both keys fully inbetween..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, unfortunatley, this does not work for me :/
I'll try a bit whether I can fix this as github seemed to have managed it. Maybe I can find a workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed a fix that works on Firefox. I also checked for chrome, but my chrome installation is somewhat broken. Thus it would be best, if you could check this again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works 🥇
Great 👍 :) |
…esign-right-sidebar * 'master' of github.com:scalableminds/webknossos: added Youtube videos to docs Log dataset uploads (with no conversion) to slack (#7157) Added "Automation Tutorial" to docs (#7160) fix logo image in README.md Second try for “Async IO for HttpsDataVault, Fox Error Handling” (#7155) Revert "Async IO for HttpsDataVault, Fox Error Handling (#7137)" (#7154) Async IO for HttpsDataVault, Fox Error Handling (#7137) Fix vault path for precomputed datasets (#7151) Add extended keyboard shortcut mode via ctrl + k for tool shortcuts (#7112) Shared Chunk Cache for all DatasetArrays, CacheWeight for AlfuCache (#7067)
This PR adds a new "extended" mode to the keyboard shortcuts. This mode is activated using
ctrl + k
and gives access to more commands. Only shortcuts for switching between annotation tools are implemented, but more can be added.To leave the "extended" mode, the user needs to use one of the supported extended shortcuts or use
ctrl + k
again or wait 3 seconds.Here is a list of all shortcuts for all tools. These shortcuts only work, once
ctrl + k
was pressed.URL of deployed dev instance (used for testing):
Steps to test:
ctrl + k
and try to quick-access the tools above by using their respective shortcuts. This should work.Issues:
Open Questions:
Known Problems:
ctrl + k
again, but Firefox does not trigger the second press as a key event and instead opens the Google search bar.TODOs:
(Please delete unneeded items, merge only when none are left open)