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

Add extended keyboard shortcut mode via ctrl + k for tool shortcuts #7112

Merged
merged 15 commits into from
Jun 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
[Commits](https://github.com/scalableminds/webknossos/compare/23.06.0...HEAD)

### Added
- Added new shortcuts for fast tool switching. Look at the updated [Keyboard Shortcuts documentation](https://docs.webknossos.org/webknossos/keyboard_shortcuts.html#tool-switching-shortcuts) to see the new shortcuts. [#7112](https://github.com/scalableminds/webknossos/pull/7112)
- Subfolders of the currently active folder are now also rendered in the dataset table in the dashboard. [#6996](https://github.com/scalableminds/webknossos/pull/6996)
- Added ability to view [zarr v3](https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html) datasets. [#7079](https://github.com/scalableminds/webknossos/pull/7079)
- Added an index structure for volume annotation segments, in preparation for per-segment statistics. [#7063](https://github.com/scalableminds/webknossos/pull/7063)
Expand Down
24 changes: 21 additions & 3 deletions docs/keyboard_shortcuts.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,6 @@ Note that you can enable *Classic Controls* which will behave slightly different
| CTRL + SHIFT + Left Mouse Drag | Remove Voxels From Segment |
| Alt + Mouse Move | Move |
| C | Create New Segment |
| W | Cycle Through Tools (Move / Skeleton / Trace / Brush / ...) |
| SHIFT + W | Cycle Backwards Through Tools (Move / Proofread / Bounding Box / Pick Cell / ...) |
philippotto marked this conversation as resolved.
Show resolved Hide resolved
| SHIFT + Mousewheel or SHIFT + I, O | Change Brush Size (Brush Mode) |
| V | Interpolate current segment between last labeled and current slice |

Expand All @@ -112,12 +110,32 @@ Note that you can enable *Classic Controls* which won't open a context menu on r
| Right Mouse Drag | Remove Voxels |
| CTRL + Right Mouse Drag | Remove Voxels while inverting the overwrite-mode (see toolbar for overwrite-mode) |

## Tool Switching Shortcuts

Note that you need to first press CTRL + K, release these keys and then the letter that was assigned to a specific tool in order to switch to it.
CTRL + K is not needed for cyclic tool switching via W / SHIFT + W.

| Key Binding | Operation |
| --------------------------------- | --------------------------------------------------------------------------------- |
| W | Cycle Through Tools (Move / Skeleton / Trace / Brush / ...) |
| SHIFT + W | Cycle Backwards Through Tools (Move / Proofread / Bounding Box / Pick Cell / ...) |
| CTRL + K, **M** | Move Tool |
| CTRL + K, **S** | Skeleton Tool |
| CTRL + K, **B** | Brush Tool |
| CTRL + K, **E** | Brush Erase Tool |
| CTRL + K, **L** | Lasso Tool |
| CTRL + K, **R** | Lasso Erase Too |
| CTRL + K, **P** | Segment Picker Tool |
| CTRL + K, **Q** | Quick Select Tool |
| CTRL + K, **X** | Bounding Box Tool |
| CTRL + K, **O** | Proofreading Tool |

## Mesh Related Shortcuts

| Key Binding | Operation |
| ------------------------------------------------------ | ----------------------------------------------------------- |
| Shift + Click on a mesh in the 3D viewport | Move the camera to the clicked position |
| Ctrl + Click on a mesh in the 3D viewport | Unload the mesh from WEBKNOSSOS
| Ctrl + Click on a mesh in the 3D viewport | Unload the mesh from WEBKNOSSOS |

## Agglomerate File Mapping Skeleton

Expand Down
75 changes: 72 additions & 3 deletions frontend/javascripts/libs/input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,28 +67,79 @@ function shouldIgnore(event: KeyboardEvent, key: KeyboardKey) {
// This keyboard hook directly passes a keycombo and callback
// to the underlying KeyboadJS library to do its dirty work.
// Pressing a button will only fire an event once.
const EXTENDED_COMMAND_KEYS = "ctrl + k";
const EXTENDED_COMMAND_DURATION = 3000;
export class InputKeyboardNoLoop {
bindings: Array<KeyboardBindingPress> = [];
isStarted: boolean = true;
supportInputElements: boolean = false;
hasExtendedBindings: boolean = false;
cancelExtendedModeTimeoutId: ReturnType<typeof setTimeout> | null = null;

constructor(
initialBindings: BindingMap<KeyboardHandler>,
options?: {
supportInputElements?: boolean;
},
extendedCommands?: BindingMap<KeyboardHandler>,
) {
if (options) {
this.supportInputElements = options.supportInputElements || this.supportInputElements;
}

if (extendedCommands != null && initialBindings[EXTENDED_COMMAND_KEYS] != null) {
console.warn(
`Extended commands are enabled, but the keybinding for it is already in use. Please change the keybinding for '${EXTENDED_COMMAND_KEYS}'.`,
);
}

if (extendedCommands) {
this.hasExtendedBindings = true;
document.addEventListener("keydown", this.preventBrowserSearchbarShortcut);
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);
Copy link
Member

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..

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

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

works 🥇

for (const key of Object.keys(extendedCommands)) {
const callback = extendedCommands[key];
this.attach(key, callback, true);
}
}

for (const key of Object.keys(initialBindings)) {
const callback = initialBindings[key];
this.attach(key, callback);
}
}

attach(key: KeyboardKey, callback: KeyboardHandler) {
toggleExtendedMode = (evt: KeyboardEvent) => {
evt.preventDefault();
const isInExtendedMode = KeyboardJS.getContext() === "extended";
if (isInExtendedMode) {
this.cancelExtendedModeTimeout();
KeyboardJS.setContext("default");
return;
}
KeyboardJS.setContext("extended");
this.cancelExtendedModeTimeoutId = setTimeout(() => {
KeyboardJS.setContext("default");
}, EXTENDED_COMMAND_DURATION);
};

preventBrowserSearchbarShortcut = (evt: KeyboardEvent) => {
if (evt.ctrlKey && evt.key === "k") {
evt.preventDefault();
evt.stopPropagation();
}
};

cancelExtendedModeTimeout() {
if (this.cancelExtendedModeTimeoutId != null) {
clearTimeout(this.cancelExtendedModeTimeoutId);
this.cancelExtendedModeTimeoutId = null;
}
}

attach(key: KeyboardKey, callback: KeyboardHandler, isExtendedCommand: boolean = false) {
const binding = [
key,
(event: KeyboardEvent) => {
Expand All @@ -103,6 +154,11 @@ export class InputKeyboardNoLoop {
if (shouldIgnore(event, key)) {
return;
}
const isInExtendedMode = KeyboardJS.getContext() === "extended";
if (isInExtendedMode) {
this.cancelExtendedModeTimeout();
KeyboardJS.setContext("default");
}

if (!event.repeat) {
callback(event);
Expand All @@ -113,7 +169,15 @@ export class InputKeyboardNoLoop {
},
_.noop,
];
KeyboardJS.bind(...binding);
if (isExtendedCommand) {
KeyboardJS.withContext("extended", () => {
KeyboardJS.bind(...binding);
});
} else {
KeyboardJS.withContext("default", () => {
KeyboardJS.bind(...binding);
});
}
// @ts-expect-error ts-migrate(2345) FIXME: Argument of type '(string | ((...args: any[]) => v... Remove this comment to see the full error message
return this.bindings.push(binding);
}
Expand All @@ -124,6 +188,9 @@ export class InputKeyboardNoLoop {
for (const binding of this.bindings) {
KeyboardJS.unbind(...binding);
}
if (this.hasExtendedBindings) {
document.removeEventListener("keydown", this.preventBrowserSearchbarShortcut);
}
}
}
// This module is "main" keyboard handler.
Expand Down Expand Up @@ -221,7 +288,9 @@ export class InputKeyboard {
}
},
];
KeyboardJS.bind(...binding);
KeyboardJS.withContext("default", () => {
KeyboardJS.bind(...binding);
});
this.bindings.push(binding);
}

Expand Down
3 changes: 3 additions & 0 deletions frontend/javascripts/libs/keyboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,9 @@
this._paused = false;
this.setContext("global");
this.watch(targetWindow, targetElement, platform, userAgent);
// Switch to default context whose shortcuts will not be active in other contexts.
// Having the global context as default would leave the shortcuts in all contexts active.
this.setContext("default");
MichaelBuessemeyer marked this conversation as resolved.
Show resolved Hide resolved
}

Keyboard.prototype.setLocale = function (localeName, localeBuilder) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,12 @@ import {
createCellAction,
interpolateSegmentationLayerAction,
} from "oxalis/model/actions/volumetracing_actions";
import { cycleToolAction, enterAction, escapeAction } from "oxalis/model/actions/ui_actions";
import {
cycleToolAction,
enterAction,
escapeAction,
setToolAction,
} from "oxalis/model/actions/ui_actions";
import {
MoveTool,
SkeletonTool,
Expand Down Expand Up @@ -93,6 +98,10 @@ const cycleToolsBackwards = () => {
Store.dispatch(cycleToolAction(true));
};

const setTool = (tool: AnnotationTool) => {
Store.dispatch(setToolAction(tool));
};

type StateProps = {
tracing: Tracing;
activeTool: AnnotationTool;
Expand Down Expand Up @@ -131,6 +140,10 @@ class SkeletonKeybindings {
"ctrl + down": () => SkeletonHandlers.moveNode(0, 1),
};
}

static getExtendedKeyboardControls() {
return { s: () => setTool(AnnotationToolEnum.SKELETON) };
}
}

class VolumeKeybindings {
Expand All @@ -154,6 +167,19 @@ class VolumeKeybindings {
},
};
}

static getExtendedKeyboardControls() {
return {
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),
Comment on lines +173 to +180
Copy link
Contributor Author

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)

Copy link
Contributor

@dieknolle3333 dieknolle3333 Jun 2, 2023

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 :)

};
}
}

class BoundingBoxKeybindings {
Expand All @@ -162,6 +188,10 @@ class BoundingBoxKeybindings {
c: () => Store.dispatch(addUserBoundingBoxAction()),
};
}

static getExtendedKeyboardControls() {
return { x: () => setTool(AnnotationToolEnum.BOUNDING_BOX) };
}
}

const getMoveValue = (timeFactor: number) => {
Expand Down Expand Up @@ -376,7 +406,10 @@ class PlaneController extends React.PureComponent<Props> {
up: (timeFactor) => MoveHandlers.moveV(-getMoveValue(timeFactor)),
down: (timeFactor) => MoveHandlers.moveV(getMoveValue(timeFactor)),
});
const notLoopedKeyboardControls = this.getNotLoopedKeyboardControls();
const {
baseControls: notLoopedKeyboardControls,
extendedControls: extendedNotLoopedKeyboardControls,
} = this.getNotLoopedKeyboardControls();
const loopedKeyboardControls = this.getLoopedKeyboardControls();
ensureNonConflictingHandlers(notLoopedKeyboardControls, loopedKeyboardControls);
this.input.keyboardLoopDelayed = new InputKeyboard(
Expand Down Expand Up @@ -404,7 +437,11 @@ class PlaneController extends React.PureComponent<Props> {
delay: Store.getState().userConfiguration.keyboardDelay,
},
);
this.input.keyboardNoLoop = new InputKeyboardNoLoop(notLoopedKeyboardControls);
this.input.keyboardNoLoop = new InputKeyboardNoLoop(
notLoopedKeyboardControls,
{},
extendedNotLoopedKeyboardControls,
);
this.storePropertyUnsubscribers.push(
listenToStoreProperty(
(state) => state.userConfiguration.keyboardDelay,
Expand Down Expand Up @@ -454,6 +491,11 @@ class PlaneController extends React.PureComponent<Props> {
w: cycleTools,
"shift + w": cycleToolsBackwards,
};
let extendedControls = {
m: () => setTool(AnnotationToolEnum.MOVE),
...BoundingBoxKeybindings.getExtendedKeyboardControls(),
};

// TODO: Find a nicer way to express this, while satisfying flow
const emptyDefaultHandler = {
c: null,
Expand All @@ -468,16 +510,29 @@ class PlaneController extends React.PureComponent<Props> {
: emptyDefaultHandler;
const { c: boundingBoxCHandler } = BoundingBoxKeybindings.getKeyboardControls();
ensureNonConflictingHandlers(skeletonControls, volumeControls);
const extendedSkeletonControls =
this.props.tracing.skeleton != null ? SkeletonKeybindings.getExtendedKeyboardControls() : {};
const extendedVolumeControls =
this.props.tracing.volumes.length > 0 != null
? VolumeKeybindings.getExtendedKeyboardControls()
: {};
ensureNonConflictingHandlers(extendedSkeletonControls, extendedVolumeControls);
const extendedAnnotationControls = { ...extendedSkeletonControls, ...extendedVolumeControls };
ensureNonConflictingHandlers(extendedAnnotationControls, extendedControls);
extendedControls = { ...extendedControls, ...extendedAnnotationControls };
Comment on lines +519 to +522
Copy link
Contributor Author

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

Copy link
Member

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 :)


return {
...baseControls,
...skeletonControls,
...volumeControls,
c: this.createToolDependentKeyboardHandler(
skeletonCHandler,
volumeCHandler,
boundingBoxCHandler,
),
baseControls: {
...baseControls,
...skeletonControls,
...volumeControls,
c: this.createToolDependentKeyboardHandler(
skeletonCHandler,
volumeCHandler,
boundingBoxCHandler,
),
},
extendedControls,
};
}

Expand Down
1 change: 1 addition & 0 deletions frontend/javascripts/test/helpers/apiHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ mockRequire("libs/date", DateMock);
export const KeyboardJS = {
bind: _.noop,
unbind: _.noop,
withContext: (_arg0: string, arg1: () => void) => arg1(),
};
mockRequire("libs/keyboard", KeyboardJS);
mockRequire("libs/toast", {
Expand Down