Moving and deleting multiple trees in skeletontracing

[MichaelBuessemeyer]

This PR enables selecting multiple trees. Moving and deleting effect all selected trees.

URL of deployed dev instance (used for testing):

• https://movinganddeletingmultipletrees.webknossos.xyz

Steps to test:

• open a skeletontracing
• create some trees and groups
• select some of them with ctrl + left mouse
• try deleting and moving the trees
• there should be no possibility to have a parent and a child selected simultaneously
• when deleting and groups are selected a modal should show up that asks whether to remove the groups and the subtrees or only the groups

Issues:

• fixes Delete/move trees of group #3143

---

• Updated changelog
• [ ] Updated migration guide if applicable
• Updated documentation if applicable -> maybe
• [ ] Needs datastore update after deployment
• Ready for review

[MichaelBuessemeyer]

TODO:

• discuss keys that are used to select a tree (there might be a better short than alt + left mouse) -> use ctrl + left mouse
• discuss how to better deselect everything -> click here to clear the selection -> no icon
• no tooltip at mouse position possible while dragging trees/groups -> other possibilities? (but it is maybe possible when using only js and not react to render the tooltip and then just adjust the position via event handling) => if not used -> remove sticky-mouse-tooltip + adjust state of tree-hierarchy-view
  -> nope, text is fine.
• moving a not selected tree unintentionally selects the tree (groups are working fine) -> fix this
• selected -> maybe other color -> selected in light green
• only one scrolling bar -> issue Only one scrollbar in trees tab view #3492
• too much recursion when dragging trees in some cases -> bug
• add to keyboard shortcuts
• add to discuss

[MichaelBuessemeyer]

@philippotto @daniel-wer This PR is ready for another review :) (200 line of code less 🎉 )

[daniel-wer]

Great PR @MichaelBuessemeyer :)
The simplifications were really worth it imo and the code reads very well! I made some suggestions.

After testing, I have some other UX improvement recommendations and ideas which could be best explained in a short call I think. Please let me know in slack, when you would be available for a 10min call in the next days :)

[daniel-wer]

Just for the record, the action items from our call:

• Only show the "x trees are selected" message if selectedTrees.length > 1
• Selecting another tree (without ctrl) or group will clear selection (as in file picker)
• Active Tree is always selected, if multiple trees are selected all should be green
• If a group is active, no other trees can be selected with ctrl + click (maybe show message)

[MichaelBuessemeyer]

@daniel-wer I applied your feedback: You can now only have either selected trees, an active group or an active tree.

[daniel-wer]

@MichaelBuessemeyer This looks very good!

There seems to be a small bug when having an active tree and selecting the very same active tree using Ctrl + Click. It then says, "Two Tree(s) selected", although probably nothing should have happened at all.

The "Delete Group and Subtrees" option of the group deletion modal can lead to inconsistent state, because subgroups and sub-subtrees are not properly deleted. This needs to be fixed.

There are a couple of cases where the user can end up with neither an activeTree nor an activeGroup, for example when clicking on "Clear Selection", and I can understand why you designed it that way. We'll have to see whether this confuses users and how heavily this feature will be used, I'd say it's fine for now :)

[MichaelBuessemeyer]

@daniel-wer I applied your feedback: I fixed the error with the inconsistent state after deleting a group with all subtrees. And I added better handling for the corner cases of the tree selection.

[daniel-wer]

Looking good, I've added two minor comments.

The only remaining issue I see is related to the group removal, see this example case:

• group1
  • group2
    • tree2
  • tree1

In the current version, when removing group1 and selecting "Remove group and subtrees", group1 and tree1 will be removed, whereas group2 and tree2 will be moved up one level in the tree hierarchy. While the caption of this action is technically correct, I think this is going to confuse users. We should either:

• remove the option to delete subtrees completely (as it was before)
• delete all subtrees and subgroups recursively

I would prefer the second option, because I can imagine this functionality is useful, what's your opinion on this @philippotto?

[philippotto]

I would prefer the second option, because I can imagine this functionality is useful, what's your opinion on this @philippotto?

Yes, that makes perfect sense. The two options should be either deleting a minimum or a maximum of reasonable trees. Deleting all children recursively appears to be the more frequent scenario. The caption of the confirm dialog needs to be adapted then, as well ("Do want to delete this group and keep its children or do you want to remove all children recursively?" or something like that).

[daniel-wer]

@MichaelBuessemeyer Awesome, works as expected for me :)

@philippotto Maybe you could do a test run of the functionality as well, this PR has been open for a while and I want to avoid "Betriebsblindheit" :)

If that is successful, ship it 🚢

[philippotto]

I just tested this branch and it works pretty well 👍 The UI is very intuitive in my opinion and easy to grasp.

However, I noticed one edge case: If there is only a group at the root level (which contains all other groups and trees), deleting that group recursively will result in a completely empty tracing. By itself, this sounds fine, however, the rest of WK assumes that there will always be at least one (empty) tree. In the other places we ensure that by adding an empty tree if the last tree was added (see the skeleton reducer for details). This would need to be fixed before merging, I think :)

[daniel-wer]

Without explicitly debugging this, but from reading the code, I think the bug is a little more subtle than this. The deleteMultipleTreesAsUserAction does use the usual deleteTreeAction, so when deleting the last tree, a new tree is automatically created. However, the new tree is assigned to the group of the active tree (the last tree), which is probably one of the deleted groups, which is then deleted afterwards.
I'll think about a possible solution.

[daniel-wer]

I pushed a fix for this issue and opted to fix it in the reducer itself as a safety measure against possible future bugs. When updating the tree groups, all trees with non-existing groupIds are moved to the root tree group (groupId = null).