Annotation user guide - #1006
Conversation
|
I think the re-order feature is great though I think we should split it out into a separate PR for better git history one minor ux improvement is to hide the trash icon when dragging is active |
| document.addEventListener("keydown", onKeyDown, true); | ||
| }; | ||
|
|
||
| listElement.addEventListener("mousedown", onMouseDown); |
There was a problem hiding this comment.
I think one improvement here would be to instead listen to an action, like listElement.addEventListener("action:reorder-annotation", onMouseDown);
And then L698 needs to change to match the name. Alternatively you can use registerActionListener and that returns the disposer.
L665 in onMouseDown doesn't need the guard anymore on the mouse button and key modifier, and the name can become less generic because it is not handling all onMouseDown events. And you'd need to add a bind to default_input_event_bindings for the action, something like:
let defaultAnnotationListBindings: EventActionMap | undefined;
export function getDefaultAnnotationListBindings() {
if (defaultAnnotationListBindings === undefined) {
defaultAnnotationListBindings = EventActionMap.fromObject(
{
click0: "pin-annotation",
mousedown2: "move-to-annotation",
"alt+mousedown0": "reorder-annotation", // This is the added bind
},
{ parents: [[getDefaultSelectBindings(), 0]] },
);
}
return defaultAnnotationListBindings;
}
This means that the action mapping should show on hover of the annotation list element, and should also allow to be rebound.
There was a problem hiding this comment.
thanks, i've pushed this adjustment to #1016
d3a74cd to
ff615e0
Compare
|
I have split out the src code changes from the doc changes, i have opened a seperate PR #1016 with the reordering feature and link to the docs from the app. |
seankmartin
left a comment
There was a problem hiding this comment.
I still think the docs are great, I'm just removing the "approve" flag for clarity. There are some TODO links to fill in and if possible I'd like to check with @jbms about storing videos as I get the sense the neuroglancer repository has been setup to be pretty minimal in size
Expanded docs since first look, still think content is great, can come back to it once finalised
|
@seankmartin thanks! i forgot about the TODO items i had put in. I have fixed those now. @jbms i'm happy to put videos somewhere else, I don't think it matters much for git as long as they are under 50 mb, the data isn't packaged into the python package or into the app for deployment, just the docs site. |
|
A few comments about videos:
|
|
Think putting the videos in a bucket is a fine idea, what should be the mechanics of getting the videos into the bucket? In terms of auto-generating them, I don't know if the scripting language we have with neuroglancer python has cursors. honestly, I think having overlays of button press keys on the videos would also be ideal/better. I'm worried that the infrastructure we have to do that is not really sufficient. Perhaps there is some more generic browser emulation that could produce the videos. i do understand that it would be better to make sure that the videos stay up to date with how the interface looks and the overtime there will inevitably be visual regression. That said, I would not let the perfect be the enemy of the good here. We currently have no real documentation associated with the main project that is user oriented, so I don't think we should gate getting something out on developing some more sophisticated infrastructure. |
|
@jbms what about playwright's record feature? https://playwright.dev/docs/videos |
|
I wasn't aware of that, looks like it could be a good option. |
|
i don't think playwright is a good option because it doesn't show a cursor on the screen .. it can't really because the test only captures the events and not the movement of the mouse. |
|
In prior tests I used injected javascript to display an overlay showing the mouse (much larger than normal for visibility and to show the buttons). That works quite well, so it doesn't matter if the real mouse is visible. |
|
I don't know... the more i think about this the more it feels like we are making adding documentation into a coding project. In the long run, it would be great if we had a system which encouraged people to contribute to the documentation without having to be strong coders. Here we are going to have to make each video you want to add to the docs into a mini coding project, where you learn how to use playwright, record actions and generate a script. Writing documentation is challenging enough without having to go through this side quest. If I could write some instructions that let somebody just hit record, then do some actions in a neuroglancer window, and pop out a script that could produce a video that reproduces what they just did that seems reasonable ... but I can't seem to see how that is working with the tooling i've tried so far. Playwright chrome recorder doesn't record mouse events when you have a button pressed down. playwright codegen similarly doesn't let you record clicks on specific locations on the screen. Neither of them record timing information, so the replay isn't a faithful reproduction of what the person was trying to demonstrate. |
|
@jbms does the mechanism of the video generation couple into how the videos get integrated into a cloud bucket. In that you were imagining the script generates the video on a github action and uploads the results to the bucket, so the video isn't in the source code? |
Certainly the question of automated vs manual does have some impact. But in general I'd say that regardless of whether they are automatically generated or manually generated they can't be stored in the source repo, though potentially git lfs could be used.
Agreed that it makes it significantly harder to add a video, at least initially and for first time users, though with coding agents the calculation may change some.
In general you would need an AI model to properly take a recorded sequence of user actions and convert that to a script that robustly performs the intended actions and isn't brittle to small changes in the UI or loading times. I think there is an incremental approach we can take:
|
d992c92 to
5d804df
Compare
|
so fork in the road, should we add this tooling to this PR? if so, I think it will grow in complexity a great deal, which is okay, but it will delay getting the documentation additions, which I think outside of the videos seem uncontroversial (and we need a lot more on top) if not, do we revert this PR to only include pictures? not include any visuals? and then work on a separate with this video infrastructure? and then maybe a 3rd which edits the docs to include the videos after we have the scripts to generate them. |
|
I would vote: in a separate pr: Basically I think we should find a process that works for improving test coverage as well as documentation |
|
I think if @fcollman is ok with it, removing the images and videos from this PR would be best until we have a proper strategy for where to store them with @jbms. I think it's worth opening an issue to discuss further, and can copy some of that over to here. This will be a general pattern, as it also arises for example in the SSAO PR #993 with the images in that PR |
|
yes i'm happy to do that.. will have to do a little rewrite to remove references to pictures for now |
9ef18b6 to
a73f90d
Compare
a73f90d to
05d5ce6
Compare
|
I have removed all the pictures from the PR. I am emailing all the maintainers with a link to a google drive with the images and movies that I did have in there so that the assets are still available when there is agreement/process about how to include such files into the documentation flow. |
seankmartin
left a comment
There was a problem hiding this comment.
Thanks for removing the images and videos, last points from me and then good to go in my eyes!
| Mainline neuroglancer does **not** include built-in hotkeys for | ||
| cycling through annotations (for example, "next annotation" / | ||
| "previous annotation"). To get this behaviour, users must add custom | ||
| bindings. | ||
|
|
||
| This is done by overriding ``inputEventBindings`` in the viewer JSON | ||
| state, which lets you map additional key combinations to existing | ||
| actions. See the JSON API documentation for the binding format and | ||
| the list of available actions. | ||
|
|
||
| Forks or downstream deployments of neuroglancer may bundle additional | ||
| default bindings for annotation navigation; the workflow above is | ||
| required only on mainline. |
There was a problem hiding this comment.
Is inputEventBindings part of the JSON state? I can see it available on the viewer directly via viewer.inputEventBindings and also in the Python API, but not the regular viewer JSON state. I'm not too familiar with this part to be honest
There was a problem hiding this comment.
removed this reference
| different aspects of the annotations | ||
| `Can be found here <https://github.com/google/neuroglancer/blob/master/src/annotation/rendering.md>`_ | ||
|
|
||
| . |
There was a problem hiding this comment.
very minor, this full stop got separated onto a newline by itself
|
i think we are having some broader test issues.. |
This PR add documentation related to annotations to the the docs site, including a section on annotation_shaders