Added shorthand to mark branch points from context menu

[hotzenklotz]

I added the option to mark a node as branch point directly from the context menu.

I felt like this was missing. I decided against the option to "remove" as branch point because that cause wK to jump to a different location which I felt was weird... or at least unintuitive when interacting with a single node.

Any not sure if there was an active design decision to not include it, that I am not aware of?

URL of deployed dev instance (used for testing):

• https://___.webknossos.xyz

Steps to test:

• Place a node
• Right-click on node to bring up context-menu
• Select "mark as branch point" option

Issues:

• related to More options for (node) context menu #5419

---

(Please delete unneeded items, merge only when none are left open)

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

[philippotto]

Great stuff! Code looks perfect. The only thing I'm hesitant about is:

I decided against the option to "remove" as branch point because that cause wK to jump to a different location which I felt was weird... or at least unintuitive when interacting with a single node.

I agree, that it would be unintuitive to use the normal "pop branchpoint" behavior by default, but it also seems weird that a branchpoint cannot be removed from the context menu even though it was created via that menu. I imagine that users might be having a hard time to remove a branchpoint then.

I think, it should be simple to remove a branchpoint from the context menu which does not change the current position. For this, you would need to make setting activeNodeId here conditional: https://github.com/scalableminds/webknossos/blob/master/frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer.js#L602
The condition itself would be contained in the action deleteNodeAction which could be changed to:

// in skeletontracing_actions.js:
export const deleteNodeAction = (
  nodeId?: number,
  treeId?: number,
  jumpToPrevious: boolean = true,
  timestamp: number = Date.now(),
): DeleteNodeAction => ({
  type: "DELETE_NODE",
  nodeId,
  treeId,
  timestamp,
  jumpToPrevious,
});

The third spot which would need to be adapted is probably in skeletontracing_saga.js in centerActiveNode which should do if (action.type == "DELETE_BRANCHPOINT" && !action.jumpToPrevious) return; so that no re-centering is performed.

What do you think?

[hotzenklotz]

I agree that having "unmark as branch point" would be nice as well and good style. However, I think the implementation is not as easy as outlined. The DELETE_BRANCHPOINT action always(!) delete the last branch point for the queue (aka pop). Let's have another look tomorrow.

[philippotto]

I agree that having "unmark as branch point" would be nice as well and good style. However, I think the implementation is not as easy as outlined. The DELETE_BRANCHPOINT action always(!) delete the last branch point for the queue (aka pop). Let's have another look tomorrow.

Ah, good catch. Then, I assume, we would need a new action with a separate reducer. Let me know if you want to talk about it or whether you want to delegate this.

[hotzenklotz]

@philippotto I added the "unmark as branch point" functionality. Please review / check. Thanks

[philippotto]

The code looks good 👍 I'd only rename the action from deleteSelectedBranchpoint to deleteBranchpointById, since the first name sounds like the selected ("aka" active) node plays a role for the action.

Also, the CI fails (due to prettier?) currently. When it succeeds, I'll do testing round :)

[hotzenklotz]

@philippotto I applied your PR feedback and renamed the action. CI should also be fixed.

[philippotto]

Testing went superbly.