Skip to content
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

WIP: Undo-redo for intervals #11550

Closed
wants to merge 3 commits into from

Conversation

Abe27342
Copy link
Contributor

@Abe27342 Abe27342 commented Aug 15, 2022

Description

This adds an API to @fluidframework/sequence which enables recovering an interval from one of its endpoints. The specific API retrieves the label for the interval collection that endpoint is a part of, as well as the id of the interval. This required storing an additional prop on each endpoint reference. I also fixed a bug in treating the label field differently on reloaded intervals.

The motivation for this change is to enable reasonable undo-redo behaviors for SlideOnRemove intervals to be implementable. There is some discussion of the issue on #11269. To that end, I also augmented the example undo-redo handler we have implemented for sequence to demonstrate how one might go about reverting refs. A real implementation would likely filter which refs to restore based on their properties.

One alternative approach I could think of would be to push elements onto the undo-redo stack when the changeInterval event is emitted from an interval collection. This is only remotely plausible once the changes from #10966 are released (currently in next). However, that approach suffers from a few timing-related issues: the interval change due to slide won't be emitted until the remote is acked, which is awkward for app code to handle (e.g. one might want to undo an operation before it's acked), and also means we'd need to expose additional APIs on sequence to resolve the correct "previous position": the previousInterval argument on the change event only provides a ReferencePosition that lives on a removed segment, but the interval collection APIs operate in terms of positions. So one would need to find the "equivalent position on the newly inserted segments," which would need something like a rebase op. A sample of how that might look (without getting the rebase logic correct) can be found here.

Going with this approach closes #11269.

@Abe27342 Abe27342 requested review from a team as code owners August 15, 2022 20:52
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: sharedstring area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct public api change Changes to a public API base: main PRs targeted against main branch labels Aug 15, 2022
@msfluid-bot
Copy link
Collaborator

@fluid-example/bundle-size-tests: +356 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 392.36 KB 392.53 KB +178 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 191.92 KB 191.92 KB No change
loader.js 151.12 KB 151.12 KB No change
map.js 42.63 KB 42.63 KB No change
matrix.js 131.7 KB 131.7 KB No change
odspDriver.js 150.23 KB 150.23 KB No change
odspPrefetchSnapshot.js 38.39 KB 38.39 KB No change
sharedString.js 152.34 KB 152.51 KB +178 Bytes
Total Size 1.25 MB 1.25 MB +356 Bytes

Baseline commit: e471dda

Generated by 🚫 dangerJS against 923a1cd

const segRefs = Array.from(segment.localRefs ?? [], (ref) => ({ ref, offset: ref.getOffset() }));
if (segRefs.length > 0) {
current.refsToRestore ??= new Map();
current.refsToRestore.set(segment, segRefs);
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think this works, as the segment could still split after being removed, as it is still above the min seq, that could then mess with the mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, yeah--that makes sense. It should be possible to do something similar but using tracking groups, right? (haven't looked too much into the details, but I see that they split how one would want on segment split)

Then--is that logic something worth exporting from MergeTree? At first glance it looks like it should be possible to write in a tree-shakeable way, so hopefully no impact for non-users.

Copy link
Contributor Author

@Abe27342 Abe27342 Aug 16, 2022

Choose a reason for hiding this comment

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

alternatively, seems like mergeTreeMaintenanceCallback has enough information to remedy this. It's a lot of burden to put on the implementer though.

Using tracking groups seems tractable but might need some API changes--doesn't look like there's quite enough information there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed a change with a test for this kind of thing (and haven't tweaked implementation, so it fails). Going to table the undo/redo part of this PR for now and split out the interval collection changes into a separate thing.

@Abe27342 Abe27342 changed the title Allow querying for interval from its local reference endpoint WIP: Undo-redo for intervals Aug 17, 2022
@Abe27342
Copy link
Contributor Author

Going to close this without merging for now, may re-open later with improved undo-redo or just start fresh. I split out the necessary changes to interval collections elsewhere.

@Abe27342 Abe27342 closed this Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: sharedstring area: dds Issues related to distributed data structures area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intervals don't seem to go back to their previous positions after an undo operation
3 participants