Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix volume undo for unfetched data #5608
Fix volume undo for unfetched data #5608
Changes from 3 commits
e902772
e4c92b6
a9b367e
fdc482d
1a0394c
614b44a
95305cf
7fd968c
23134f4
0eaf519
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems a little out of place here. Also, the "Better version:" beginning can be dropped I'd say. Maybe around line 240 would be more suitable to explain this (or above the
compressBucketAndAddToUndoBatch
function)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simply removed the comment in a previous commit, as these were only notes from the call with philipp on how to solve this bug.
And I think you reviewed an old version here, as I can no longer find the comment and I am pretty sure I removed it, before assigning you as an reviewer 👀 🦢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did, but it seems like you didn't push this change until the 16.07. which is why I saw the "old" code during my review. In any case, all good :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm struggling to wrap my head around all of this, but wouldn't it be possible to do the merging which is done in line 348 and line 367, instead here, directly? In my head, this would greatly simplify the
applyAndGetRevertingVolumeBatch
, because no special treatment would need to be implemented, there (except of maybe waiting for the promise to be resolved). But it could very well be that I'm overlooking something. Let me know if it's easier to discuss this in a call.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it is possible to do the merging once the backend data arrived. But we decided against this to defer the more intensive computation. If the backend data is merged directly once it arrives, the following will be done for each bucket, whose backend data is being fetched, when the user drew over it:
Once the backend data is fetched:
And if the user then undos the action:
The way the code works now is more complicated but should be (not measured) more efficient in normal uses cases. Because we assume that only a few brush strokes are undone by the user and therefore only a few buckets actually need to be merged with the backend data:
Once the backend data is fetched:
And only if the undo action is triggered do the following:
As you can see, in the current implementation there are some steps saved in case that the volume action is not undone.
Does this explain the design decision to you and do you think this is justified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the thorough explanation! It does make sense now that I understand the reasoning :) Could you add a comment summarizing this rationale above line 248?