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

### Changed
Expand Down
23 changes: 20 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,31 @@ 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

To be able to switch to a specific tool first press CTRL + K and then the key that was assigned to that tool.
philippotto marked this conversation as resolved.
Show resolved Hide resolved

| 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 lErase Too |
MichaelBuessemeyer marked this conversation as resolved.
Show resolved Hide resolved
| 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
59 changes: 56 additions & 3 deletions frontend/javascripts/libs/input.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,28 +67,67 @@ 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;
extendedBindings: Array<KeyboardBindingPress> = [];
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.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);
};

cancelExtendedModeTimeout() {
this.cancelExtendedModeTimeoutId && clearTimeout(this.cancelExtendedModeTimeoutId);
MichaelBuessemeyer marked this conversation as resolved.
Show resolved Hide resolved
}

attach(key: KeyboardKey, callback: KeyboardHandler, isExtendedCommand: boolean = false) {
const binding = [
key,
(event: KeyboardEvent) => {
Expand All @@ -103,6 +142,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 +157,14 @@ export class InputKeyboardNoLoop {
},
_.noop,
];
KeyboardJS.bind(...binding);
if (isExtendedCommand) {
KeyboardJS.withContext("extended", () => {
KeyboardJS.bind(...binding);
});
}
KeyboardJS.withContext("default", () => {
KeyboardJS.bind(...binding);
});
Copy link
Contributor Author

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 👍

philippotto marked this conversation as resolved.
Show resolved Hide resolved
// @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 Down Expand Up @@ -221,7 +272,9 @@ export class InputKeyboard {
}
},
];
KeyboardJS.bind(...binding);
KeyboardJS.withContext("default", () => {
KeyboardJS.bind(...binding);
});
this.bindings.push(binding);
}

Expand Down
1 change: 1 addition & 0 deletions frontend/javascripts/libs/keyboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@
this._paused = false;
this.setContext("global");
this.watch(targetWindow, targetElement, platform, userAgent);
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