Skip to content

Batch tree deletion actions when deleting a group #4312

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Nov 19, 2019

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Nov 6, 2019

I batched the group-with-subtree deletion, mulitple tree deletion and multiple tree move user actions. The confirm group/tree/node-with-id-1 deletion code is pretty messy and should be consolidated, but I'll probably not do that as part of this PR. My changes should not make it much worse.
I also found a (minor) bug in the undo functionality where when only a single action was triggered, for example, SET_TREE_GROUP, and that was then undone, the save_saga would take the UNDO action and diff the tracing state, although the "undone" tracing state was not set in the store yet. We didn't notice this before, because usually there's a couple more flycam actions or so that follow, which trigger the differ again. To fix this, I changed the save_saga code to take the SET_TRACING action instead which is what undo/redo dispatch to set the new tracing state.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Create a skeletontracing with nested groups and trees. Delete a group that contains multiple trees. Use undo to restore the previous state. The deletion of trees within one group should be "atomic" and be restored with one undo action.
  • Select multiple trees and delete/move them. Undo the change which should be "atomic".
  • Create a task and check whether the "Are you sure you want to delete the tree with id 1"-warning is still properly shown.

Issues:


Outdated, but for reference:

I gave the redux-batched-actions lib a try and the easy case seems to work pretty well (batching multiple actions of the same type). However, group/tree/node deletion is rather complex in our code, triggering Modals with async responses, so using the batched actions in that scenario is not straight-forward. This is why in the current iteration only the DELETE_TREE actions deleting the trees of one group at a time are batched together.
Another problem would be if we wanted to batch actions of different types as redux-saga's take... methods then no longer work out of the box, because they don't know about batched actions. It would be possible to overwrite those take functions to unpack and match the batched actions but that would be rather involved I think.
I've read quite a bit in the issues sections of the library, but couldn't find a good solution yet.

Let's discuss this tomorrow in person :)

@daniel-wer daniel-wer self-assigned this Nov 6, 2019
@philippotto philippotto changed the title Batch tree deletion actions when deleting a group [WIP] Batch tree deletion actions when deleting a group Nov 12, 2019
@@ -285,22 +286,22 @@ class TreeHierarchyView extends React.PureComponent<Props, State> {
const hasSubgroup = anySatisfyDeep(node.children, child => child.type === TYPE_GROUP);
const menu = (
<Menu onClick={this.handleDropdownClick}>
<Menu.Item key="create" groupId={id}>
<Menu.Item key="create" data-group-id={id}>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix for a React warning that I encountered a lot. React complained about the custom prop groupId that was not removed before adding the element to the DOM.

@daniel-wer daniel-wer changed the title [WIP] Batch tree deletion actions when deleting a group Batch tree deletion actions when deleting a group Nov 13, 2019
@daniel-wer
Copy link
Member Author

@philippotto I implemented this as discussed and updated the PR description. Should be ready for review :)

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! The changes look very good and testing went well, too. One thing which I noticed is that the merger mode uses api.utils.registerOverwrite("DELETE_TREE" ...) and that this should probably be adapted to the new batched action. Shouldn't be much work, I assume? Otherwise, deleting multiple trees will probably break the merger mode.

@daniel-wer
Copy link
Member Author

One thing which I noticed is that the merger mode uses api.utils.registerOverwrite("DELETE_TREE" ...) and that this should probably be adapted to the new batched action.

Very good call, I pushed a fix :)

@philippotto
Copy link
Member

Awesome, looks good 👍

@bulldozer-boy bulldozer-boy bot merged commit 328a27a into master Nov 19, 2019
@bulldozer-boy bulldozer-boy bot deleted the batched-actions branch November 19, 2019 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants