delete tree if delete node is initiated for an empty tree (#2806)

* delete tree if delete node is initiated for an empty tree * fix linting * don't delete tree when deleting last node; move convenience functionality into 'userAction' instead of reducer * adapt tests to new delete node behavior * do some beauty corrections * rename deleteTreeWithConfirm to deleteTreeAsUser
scalableminds · Jun 28, 2018 · ddcfd82 · ddcfd82
1 parent aab64d6
commit ddcfd82
Show file tree

Hide file tree

Showing 7 changed files with 109 additions and 74 deletions.
diff --git a/app/assets/javascripts/oxalis/controller/combinations/skeletontracing_plane_controller.js b/app/assets/javascripts/oxalis/controller/combinations/skeletontracing_plane_controller.js
@@ -14,7 +14,7 @@ import SceneController from "oxalis/controller/scene_controller";
 import { OrthoViews } from "oxalis/constants";
 import {
   setActiveNodeAction,
-  deleteNodeWithConfirmAction,
+  deleteNodeAsUserAction,
   deleteEdgeAction,
   createTreeAction,
   createNodeAction,
@@ -84,7 +84,7 @@ class SkeletonTracingPlaneController extends PlaneControllerClass {
       "2": () => Store.dispatch(toggleInactiveTreesAction()),
 
       // Delete active node
-      delete: () => Store.dispatch(deleteNodeWithConfirmAction()),
+      delete: () => Store.dispatch(deleteNodeAsUserAction()),
       c: () => Store.dispatch(createTreeAction()),
 
       // Branches

diff --git a/app/assets/javascripts/oxalis/controller/viewmodes/arbitrary_controller.js b/app/assets/javascripts/oxalis/controller/viewmodes/arbitrary_controller.js
@@ -20,7 +20,7 @@ import {
 } from "oxalis/model/actions/settings_actions";
 import {
   setActiveNodeAction,
-  deleteNodeWithConfirmAction,
+  deleteNodeAsUserAction,
   createNodeAction,
   createBranchPointAction,
   requestDeleteBranchPointAction,
@@ -221,7 +221,7 @@ class ArbitraryController extends React.PureComponent<Props> {
 
       // Delete active node and recenter last node
       "shift + space": () => {
-        Store.dispatch(deleteNodeWithConfirmAction());
+        Store.dispatch(deleteNodeAsUserAction());
       },
     });
   }

diff --git a/app/assets/javascripts/oxalis/model/actions/skeletontracing_actions.js b/app/assets/javascripts/oxalis/model/actions/skeletontracing_actions.js
@@ -179,31 +179,6 @@ export const deleteNodeAction = (
   timestamp,
 });
 
-export const deleteNodeWithConfirmAction = (
-  nodeId?: number,
-  treeId?: number,
-): DeleteNodeActionType | NoActionType => {
-  const state = Store.getState();
-  return getActiveNode(state.tracing)
-    .map(activeNode => {
-      nodeId = nodeId != null ? nodeId : activeNode.id;
-      if (state.task != null && nodeId === 1) {
-        // Let the user confirm the deletion of the initial node (node with id 1) of a task
-        Modal.confirm({
-          title: messages["tracing.delete_initial_node"],
-          onOk: () => {
-            Store.dispatch(deleteNodeAction(nodeId, treeId));
-          },
-        });
-        // As Modal.confirm is async, return noAction() and the modal will dispatch the real action
-        // if the user confirms
-        return noAction();
-      }
-      return deleteNodeAction(nodeId, treeId);
-    })
-    .getOrElse(noAction());
-};
-
 export const deleteEdgeAction = (
   sourceNodeId: number,
   targetNodeId: number,
@@ -273,31 +248,6 @@ export const deleteTreeAction = (
   timestamp,
 });
 
-export const deleteTreeWithConfirmAction = (treeId?: number): NoActionType => {
-  const state = Store.getState();
-  getTree(state.tracing, treeId).map(tree => {
-    if (state.task != null && tree.nodes.has(1)) {
-      // Let the user confirm the deletion of the initial node (node with id 1) of a task
-      Modal.confirm({
-        title: messages["tracing.delete_tree_with_initial_node"],
-        onOk: () => {
-          Store.dispatch(deleteTreeAction(treeId));
-        },
-      });
-    } else {
-      Modal.confirm({
-        title: messages["tracing.delete_tree"],
-        onOk: () => {
-          Store.dispatch(deleteTreeAction(treeId));
-        },
-      });
-    }
-  });
-  // As Modal.confirm is async, return noAction() and the modal will dispatch the real action
-  // if the user confirms
-  return noAction();
-};
-
 export const toggleTreeAction = (
   treeId?: number,
   timestamp: number = Date.now(),
@@ -393,3 +343,61 @@ export const setTreeGroupAction = (groupId: ?string, treeId: number): SetTreeGro
   groupId,
   treeId,
 });
+
+// The following actions have the prefix "AsUser" which means that they
+// offer some additional logic which is sensible from a user-centered point of view.
+// For example, the deleteNodeAsUserAction also initiates the deletion of a tree,
+// when the current tree is empty.
+
+export const deleteNodeAsUserAction = (
+  nodeId?: number,
+  treeId?: number,
+): DeleteNodeActionType | NoActionType | DeleteTreeActionType => {
+  const state = Store.getState();
+  return (
+    getActiveNode(state.tracing)
+      .map(activeNode => {
+        nodeId = nodeId != null ? nodeId : activeNode.id;
+        if (state.task != null && nodeId === 1) {
+          // Let the user confirm the deletion of the initial node (node with id 1) of a task
+          Modal.confirm({
+            title: messages["tracing.delete_initial_node"],
+            onOk: () => {
+              Store.dispatch(deleteNodeAction(nodeId, treeId));
+            },
+          });
+          // As Modal.confirm is async, return noAction() and the modal will dispatch the real action
+          // if the user confirms
+          return noAction();
+        }
+        return deleteNodeAction(nodeId, treeId);
+      })
+      // If the tree is empty, it will be deleted
+      .getOrElse(deleteTreeAction())
+  );
+};
+
+export const deleteTreeAsUserAction = (treeId?: number): NoActionType => {
+  const state = Store.getState();
+  getTree(state.tracing, treeId).map(tree => {
+    if (state.task != null && tree.nodes.has(1)) {
+      // Let the user confirm the deletion of the initial node (node with id 1) of a task
+      Modal.confirm({
+        title: messages["tracing.delete_tree_with_initial_node"],
+        onOk: () => {
+          Store.dispatch(deleteTreeAction(treeId));
+        },
+      });
+    } else {
+      Modal.confirm({
+        title: messages["tracing.delete_tree"],
+        onOk: () => {
+          Store.dispatch(deleteTreeAction(treeId));
+        },
+      });
+    }
+  });
+  // As Modal.confirm is async, return noAction() and the modal will dispatch the real action
+  // if the user confirms
+  return noAction();
+};
diff --git a/app/assets/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.js b/app/assets/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.js
@@ -157,10 +157,6 @@ export function deleteNode(
         neighborIds.push(edge.target === node.id ? edge.source : edge.target);
       }
 
-      if (neighborIds.length === 0) {
-        return deleteTree(state, activeTree, timestamp);
-      }
-
       const newTrees = splitTreeByNodes(
         state,
         skeletonTracing,
@@ -176,8 +172,10 @@ export function deleteNode(
         newMaxNodeId = getMaximumNodeId(newTrees);
       }
 
-      const newActiveNodeId = neighborIds[0];
-      const newActiveTree = findTreeByNodeId(newTrees, newActiveNodeId).get();
+      const newActiveNodeId = neighborIds.length > 0 ? neighborIds[0] : null;
+      const newActiveTree = newActiveNodeId
+        ? findTreeByNodeId(newTrees, newActiveNodeId).get()
+        : activeTree;
       const newActiveTreeId = newActiveTree.treeId;
 
       return Maybe.Just([newTrees, newActiveTreeId, newActiveNodeId, newMaxNodeId]);
@@ -247,6 +245,12 @@ function splitTreeByNodes(
 
   let newTrees = skeletonTracing.trees;
 
+  if (newTreeRootIds.length === 0) {
+    // As there are no new tree root ids, we are deleting the last node from a tree.
+    // It suffices to simply update that tree within the tree collection
+    return update(newTrees, { [activeTree.treeId]: { $set: activeTree } });
+  }
+
   // Traverse from each possible new root node in all directions (i.e., use each edge) and
   // remember which edges were already visited.
   const visitedEdges = {};

diff --git a/app/assets/javascripts/oxalis/model/sagas/skeletontracing_saga.js b/app/assets/javascripts/oxalis/model/sagas/skeletontracing_saga.js
@@ -106,6 +106,7 @@ export function* watchSkeletonTracingAsync(): Generator<*, *, *> {
       "DELETE_NODE",
       "DELETE_BRANCHPOINT",
       "SELECT_NEXT_TREE",
+      "DELETE_TREE",
       "UNDO",
       "REDO",
     ],

diff --git a/app/assets/javascripts/oxalis/view/right-menu/trees_tab_view.js b/app/assets/javascripts/oxalis/view/right-menu/trees_tab_view.js
@@ -15,7 +15,7 @@ import {
   setTreeNameAction,
   createTreeAction,
   addTreesAndGroupsAction,
-  deleteTreeWithConfirmAction,
+  deleteTreeAsUserAction,
   shuffleTreeColorAction,
   shuffleAllTreeColorsAction,
   selectNextTreeAction,
@@ -285,7 +285,7 @@ const mapDispatchToProps = (dispatch: Dispatch<*>) => ({
     dispatch(createTreeAction());
   },
   onDeleteTree() {
-    dispatch(deleteTreeWithConfirmAction());
+    dispatch(deleteTreeAsUserAction());
   },
   onChangeTreeName(name) {
     dispatch(setTreeNameAction(name));

diff --git a/app/assets/javascripts/test/reducers/skeletontracing_reducer.spec.js b/app/assets/javascripts/test/reducers/skeletontracing_reducer.spec.js
@@ -164,11 +164,24 @@ test("SkeletonTracing should add nodes to a different tree", t => {
   t.deepEqual(newState.tracing.trees[2].edges.asArray(), [{ source: 2, target: 3 }]);
 });
 
-test("SkeletonTracing shouldn't delete a node from an empty tree", t => {
-  const action = SkeletonTracingActions.deleteNodeAction();
-  const newState = SkeletonTracingReducer(initialState, action);
+test("SkeletonTracing shouldn't delete the tree if 'delete node' is initiated for an empty tree", t => {
+  const createTreeAction = SkeletonTracingActions.createTreeAction();
+  const deleteNodeAction = SkeletonTracingActions.deleteNodeAction();
+
+  const newStateA = SkeletonTracingReducer(initialState, createTreeAction);
+  const newStateB = SkeletonTracingReducer(newStateA, deleteNodeAction);
+
+  t.deepEqual(newStateA, newStateB);
+});
+
+test("SkeletonTracing should delete the tree if 'delete node as user' is initiated for an empty tree", t => {
+  const createTreeAction = SkeletonTracingActions.createTreeAction();
+  const deleteNodeAsUserAction = SkeletonTracingActions.deleteNodeAsUserAction();
+  const newState = ChainReducer(initialState)
+    .apply(SkeletonTracingReducer, createTreeAction)
+    .apply(SkeletonTracingReducer, deleteNodeAsUserAction)
+    .unpack();
 
-  t.is(newState, initialState);
   t.deepEqual(newState, initialState);
 });
 
@@ -192,7 +205,7 @@ test("SkeletonTracing should delete a node from a tree", t => {
   t.deepEqual(newStateB, newState);
 });
 
-test("SkeletonTracing should delete several nodes from a tree", t => {
+test("SkeletonTracing should not delete tree when last node is deleted from the tree", t => {
   const createNodeAction = SkeletonTracingActions.createNodeAction(
     position,
     rotation,
@@ -201,14 +214,23 @@ test("SkeletonTracing should delete several nodes from a tree", t => {
   );
   const deleteNodeAction = SkeletonTracingActions.deleteNodeAction();
 
-  // Add one node, then delete two times
-  const newState = SkeletonTracingReducer(initialState, createNodeAction);
-  const newStateA = SkeletonTracingReducer(newState, deleteNodeAction);
-  const newStateB = SkeletonTracingReducer(newStateA, deleteNodeAction);
+  // Create tree, add two nodes, then delete them again so that the tree is removed, as well
+  const emptyTreeState = SkeletonTracingReducer(
+    initialState,
+    SkeletonTracingActions.createTreeAction(),
+  );
 
-  t.not(newStateB, newState);
-  t.not(newStateA, newState);
-  t.is(newStateB, newStateA);
+  const newState = ChainReducer(emptyTreeState)
+    .apply(SkeletonTracingReducer, createNodeAction)
+    .apply(SkeletonTracingReducer, createNodeAction)
+    .apply(SkeletonTracingReducer, deleteNodeAction)
+    .apply(SkeletonTracingReducer, deleteNodeAction)
+    .unpack();
+
+  t.deepEqual(
+    _.map(emptyTreeState.tracing.trees, tree => tree.nodes.size()),
+    _.map(newState.tracing.trees, tree => tree.nodes.size()),
+  );
 });
 
 test("SkeletonTracing should delete nodes and split the tree", t => {