Feature/fly-to #230
Feature/fly-to #230
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a "fly-to" feature that allows users to quickly zoom and focus on specific annotations. The feature adds three new API methods and configurable keyboard shortcuts (defaulting to Tab and Shift+Tab) for navigating between annotations.
Key Changes:
- New API methods (
fly_to_next_annotation,fly_to_annotation_id,fly_to_annotation) for programmatic navigation to annotations - Keyboard shortcuts for cycling through annotations with smart handling of deprecated and non-spatial annotations
- Configuration options for customizing keybindings
Reviewed Changes
Copilot reviewed 9 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/version.js | Bumped version to 0.20.0 |
| package.json | Updated package version to match version.js |
| src/subtask.ts | Added fly_to_idx state field to track current annotation focus |
| src/configuration.ts | Added configuration properties for fly-to keybindings |
| src/listeners.ts | Implemented keyboard event handling for fly-to navigation |
| src/index.js | Implemented core fly-to functionality with zoom calculation logic |
| api_spec.md | Documented new API methods and keybindings |
| demo/resume-from.html | Added example usage of custom keybind configuration |
| CHANGELOG.md | Documented changes for 0.20.0 release |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
joshua-dean
left a comment
joshua-dean
left a comment
There was a problem hiding this comment.
Very nice, works well in the default case.
Just one quirk with the configuration, and some ideas.
api_spec.md
Outdated
| - Press `Escape` or `crtl+z` to cancel the start of a new region or hole. | ||
| - Press `Escape` to exit brush/erase mode. | ||
| - Press `Tab` to set the zoom to focus on the next annotation | ||
| - Press `Tab+Shift` to set the zoom to focus on the previous annotation |
There was a problem hiding this comment.
Generally this is would be listed as Shift+Tab. See: https://ux.stackexchange.com/a/58188
src/listeners.ts
Outdated
| case ulabel.config.fly_to_next_annotation_keybind: | ||
| // For 'Tab', prevent default | ||
| if (keydown_event.key === "Tab") { | ||
| keydown_event.preventDefault(); | ||
| } | ||
|
|
||
| if (ulabel.config.fly_to_previous_annotation_keybind === null && shift) { | ||
| ulabel.fly_to_next_annotation(-1, ulabel.config.fly_to_max_zoom); | ||
| } else if (!shift) { | ||
| ulabel.fly_to_next_annotation(1, ulabel.config.fly_to_max_zoom); | ||
| } | ||
| break; | ||
| case ulabel.config.fly_to_previous_annotation_keybind: | ||
| if (ulabel.config.fly_to_previous_annotation_keybind !== null) { | ||
| ulabel.fly_to_next_annotation(-1, ulabel.config.fly_to_max_zoom); | ||
| } | ||
| break; |
There was a problem hiding this comment.
If you set fly_to_next_annotation_keybind to a letter (e.g. "w"), this case won't trigger when holding "Shift" as keydown_event.key will be the capital version of that letter (e.g. "W").
The easy fix is just fold the case on everything for checks like these.
In general, it would be nice to fully support chords (I think that is the right term anyways), e.g. "Ctrl+Shift+Alt+Fn+~", but that is definitely out of scope here. I imagine there's probably a good library that can do that for us.
There was a problem hiding this comment.
good catch, I really should've tested this. And yeah, handling chords is definitely a way better way to do this, and could be generalized for all the configurable keybinds. A full rework of how we configure keybinds is probably a good idea:
- handle chords
- handle duplicate keys
- in-session configurability
- a in-session list of current keybinds
There was a problem hiding this comment.
for now i just check against lowercase: 880ddb5
| ### `fly_to_max_zoom` | ||
| Maximum zoom factor used when flying-to an annotation. Default is `10`. | ||
There was a problem hiding this comment.
My intuition told me that "1" would be no zoom (well, 100% of the original anyways), but upon testing this out, it seems that as you approach "0", that is the "no zoom" setting. However, actually setting it to 0 seems to totally break the view when flying to an annotation, requiring a zoom reset. Actually supporting 0 obviously isn't necessary, but we probably shouldn't break if the user sets it.
We might want to guard against any values <=0 and just do nothing in that case.
There was a problem hiding this comment.
Added a setter for the zoom val and ensure it stays above 0: e84542b
| // Start with the full increment amount | ||
| let next_idx = (start_idx + increment + ordering.length) % ordering.length; | ||
| const first_checked_idx = next_idx; |
There was a problem hiding this comment.
A nice QoL for this that I've seen for other "searching" mechanisms would be a toast or similar pop-up indicating once you have looped around. A display of "current index / total" might also be nice.
Thoughts? If it is out of scope, we can make a separate issue.
There was a problem hiding this comment.
I do like this idea, it fits with a larger idea I have of having an annotation list you can interact with in the toolbox that will also leverage this flyto functionality. I created issue #234 to track
Fly-to API
Description
fly-tofunctions, which sets the zoom and focus to a specific annotationfly_to_next_annotation()fly_to_annotation_id()fly_to_annotation()TabandTab+Shiftdefault keybinds to fly-to the next/previous annotation, respectivelyfly_to_next_annotation_keybindfly_to_previous_annotation_keybindflyToAPI #122PR Checklist
package.jsonhas been bumped since last releasepackage.jsonandsrc/version.jsnpm installandnpm run buildAFTER bumping the version numberapi_spec.md)changelog.mdBreaking API Changes
By default, prevents browser default for
Tab.