-
Notifications
You must be signed in to change notification settings - Fork 340
feat(tools): custom keybinds and layer independent tool keybinds #464
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
base: master
Are you sure you want to change the base?
Conversation
| declare var NEUROGLANCER_DEFAULT_STATE_FRAGMENT: string|undefined; | ||
|
|
||
| type CustomBinding = { | ||
| layer: string, tool: string, protocol?: string, |
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.
tool should be a json object, not just a string, as some tools support options
| }); | ||
| } | ||
|
|
||
| const nameToLayer: {[key: string]: UserLayerConstructor|undefined} = {}; |
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.
This already exists as layerTypes in neuroglancer/layer.ts
|
@jbms, is it possible to get this merged? We need this feature in ng for proofreading H01 with CAVE. |
9428faa to
bc4f166
Compare
|
Updated this pull request with the ability to unbind existing keybinds with a false value and also with the latest code formatting. Side note: I am curious if you have vs code (if that is what you use) config set up to automatically do correct imports. My current issue is that I have to manually add the # before src, fix import order, and split out type only imports. Example config {
"keym": {
"action": {
"layerType": "segmentation",
"tool": "grapheneMergeSegments",
"provider": "graphene"
}
},
"keyx": {
"action": false
},
"control+shift+keyx": {
"action": "clear-segments",
"context": "sliceView"
}
} |
src/ui/default_viewer_setup.ts
Outdated
| [key: string]: CustomToolBinding | string | boolean; | ||
| }; | ||
|
|
||
| declare const CUSTOM_BINDINGS: CustomBindings | undefined; |
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.
Please rename to NEUROGLANCER_CUSTOM_INPUT_BINDINGS
src/ui/default_viewer_setup.ts
Outdated
| } else { | ||
| viewer.inputEventBindings.global.set(key, `tool-${val.tool}`); | ||
| const layerConstructor = layerTypes.get(val.layer); | ||
| if (layerConstructor) { |
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.
Probably should throw or log an error if layerConstructor not found to avoid silently ignoring invalid input. Since this is a build time option better to ensure it is correct.
src/ui/default_viewer_setup.ts
Outdated
| if (hasCustomBindings) { | ||
| for (const [key, val] of Object.entries(CUSTOM_BINDINGS!)) { | ||
| if (typeof val === "string") { | ||
| viewer.inputEventBindings.global.set(key, val); |
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.
Note that in addition to the global map there are several others that are relevant in default_input_event_bindings.ts --- in particular the default slice view and perspective view bindings. Perhaps this should account for that?
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 added a "context" property, one of "global" | "perspectiveView" | "sliceView". I could also call it map or actionMap. Restructured the json object to support it.
{
"keym": {
"action": {
"layerType": "segmentation",
"tool": "grapheneMergeSegments",
"provider": "graphene"
}
},
"keyx": {
"action": false
},
"control+shift+keyx": {
"action": "clear-segments",
"context": "sliceView"
}
}
src/ui/default_viewer_setup.ts
Outdated
| const protocol = viewer.dataSourceProvider.getProvider( | ||
| dataSource.spec.url, | ||
| )[2]; | ||
| if (protocol === desiredProvider) { |
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.
With the kvstore changes getProvider is not exactly the right method anymore. Perhaps just have a regexp that matches on the URL?
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 switched it to use the schemePattern from "#src/kvstore/url.js"; does that seem fine?
src/ui/tool.ts
Outdated
| activate(key: string): Borrowed<Tool> | undefined { | ||
| const tool = this.get(key); | ||
| activate(key: string, tool?: Tool<object>): Borrowed<Tool> | undefined { | ||
| tool = tool || this.get(key); |
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.
Please use tool ?? this.get(key) instead --- not necessary here but avoids thinking about javascript truthyness complications
src/ui/tool.ts
Outdated
| this.debounceDeactivate.cancel(); | ||
| const activeTool = this.activeTool_; | ||
| if (tool === activeTool?.tool) { | ||
| if (tool.toJSON() === activeTool?.tool.toJSON()) { |
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.
We can't compare the json like this because it can be an object rather than a plain string. Instead we need to compare the serialized json. However, it would be better if we can avoid having to insert the json comparison here, and instead just ensure that the same tool object is passed. Is your logic involving previousTool not sufficient?
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 think originally my previousTool was for deactivation but then I realized if you have a custom binding and an user specified binding for the same tool+layer. It is a bit awkward. It is not a big deal because very few users would swap between both so this code can be removed but I made it so they can deactivate each other. For example if c is bound to multicut tool and the user binds shift+a to multicut, they can activate it with either bind and deactivate it with the other.
activeTool.tool.constructor === tool.constructor &&
activeTool.tool.context === tool.context
I think this logic is cleaner, let me know. If you prefer, I'm fine with removing it. Also because of this logic, previousTool has less value, really just a cache for restoreTool. I could remove it and call restoreTool with every keypress.
|
Regarding your question about the imports, in case you haven't already figured it out --- npm run lint:fix fixes some things. I don't use vscode --- but the typescript language server does have some options for configuring how it spells imports that it generates. Unfortunately as far as I am aware there is no standard way to specify those configuration options --- instead they need to be specified in some editor-specific way. If there is a way to configure it for vscode with a config file I'm happy to include such a configuration file in this repo. |
src/ui/default_viewer_setup.ts
Outdated
| declare let NEUROGLANCER_DEFAULT_STATE_FRAGMENT: string | undefined; | ||
|
|
||
| type CustomToolBinding = { | ||
| layer: string; |
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.
rename to layerType for consistency with the tool palette queries.
bc4f166 to
7c55342
Compare
|
Thanks for going over this, I didn't notice you had reviewed it until recently. |
|
@jbms if you have time, could you take a look at this PR again? I pushed changes after your review and responded here: #464 (comment) |
|
I forgot to add this addition to the spec, being able to pass an array of bindings per identifier. I used this for our custom annotation iteration keybinds to re-bind the bracket keys. "bracketleft": [
{ "action": false, "context": "sliceView" },
{ "action": false, "context": "perspectiveView" },
{ "action": "select-previous" }
] |
…perspectiveView, sliceView) cleaned up code, addressed PR comments
…ier, allowing multiple contexts to be modified
de33dc9 to
6a84179
Compare
🔥 Preview 🔥: https://neuroglancer--pr464-jqtohdii.web.app
Updated at Thu Feb 22 2024 17:27:49 GMT+0000 (Coordinated Universal Time) for commit bc4f166
Expires: Invalid Date