Shortcuts for navigating to next/prev node

[grittaweisheit]

This PR makes it possible to navigate between nodes in a skeleton tracing via "," and "ctr + ." .

URL of deployed dev instance (used for testing):

• https://navigatenodes.webknossos.xyz

Steps to test:

• open a skeleton tracing
• have some nodes
• navigate with "," to the preceding, or with "ctr + ." to the subsequent node
• when on a branch point the node with the highest (subsequent) / lowest (preceding) id is selected
• if navigating back over a branch that was already navigated over, the path that was already taken is chosen again

Issues:

• fixes Add shortcuts for navigating to next/prev node #4147

---

• Updated (unreleased) changelog
• Updated (unreleased) migration guide if applicable
• Updated documentation if applicable
• Adapted wk-connect if datastore API changes
• Needs datastore update after deployment
• Ready for review

[daniel-wer]

Very nice, I like this feature a lot and you did a good job implementing it, it's already a pleasure to use 🎉
There are a couple of edge-cases which I annotated and I suggested a more edge-case-resistant approach to detect invalid navigation lists :)

[daniel-wer]

I think it's very hard to catch all the cases where the navigation list becomes "invalid" and would need to emptied. For example, this is the straight-forward case where a user selects another node, but there are also many other cases, like: Deleting one (or all) nodes of a tree, deleting a group with all its trees, creating a new tree, jumping to a branchpoint (using the J shortcut), ...

Therefore, what do you think about not trying to catch all these cases preemptively, but on-demand. What I mean by that is for every "hit" in the navigation list in the toSubsequentNode and toPrecedingNode functions, so whenever a node that was already in the list would be set active, before doing that you check whether the node from the list is a neighbor of the currently active node:

• If it is not a neighbor, empty the list (as you did here) because something happened in between that made the navigation list invalid, otherwise the nodes would be neighbors. Then you search a neighbor as you would do if the list was empty or exhausted.
• If it is a neighbor, proceed as usual

What do you think about this approach? :)

[grittaweisheit]

I adjusted it a little bit and just check it in advance by comparing the active node with the list's node at the active index.

[daniel-wer]

Works like a charm in my tests, very cool!

I made some suggestions to simplify the code. I'm not too happy about the code duplication, but as the two cases differ in quite a few lines I don't see an easy way to unify the two functions. So I'd say we should aim to simplify the methods as much as possible but keep them separate :)

Please also add the two new shortcuts to the list of keyboard shortcuts (keyboard_shortcuts.md)

[daniel-wer]

You could get rid of the let isValidList by directly assigning the result of the comparison here, like:

const isValidList = navigationList.list.length && activeNodeId === navigationList.list[navigationList.activeIndex];

[daniel-wer]

Same in the other function.

[grittaweisheit]

That's pretty neat! Thanks :)

[daniel-wer]

Maybe simplify this to

const [ tree, node ] = getNodeAndTree(tracing, activeNodeId, activeTreeId)
  .getOrElse([null, null]);

Same in the other method.

[grittaweisheit]

Flow is not happy with that :( . Maybe I could extract it in an extra method so it at least doesn't appear twice?

[daniel-wer]

:/ Yes you could put it into a getNodeAndTreeOrNull method in the skeleton tracing accessor file or you could annotate my proposed solution with a // $FlowFixMe to silence flow (as we know that the code is correct).

[daniel-wer]

Well done, lgtm 👍