-
Notifications
You must be signed in to change notification settings - Fork 532
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(tree) Fix document corruption due to loss of change info for transient nodes in sequence field #16190
fix(tree) Fix document corruption due to loss of change info for transient nodes in sequence field #16190
Changes from 8 commits
b1b645f
29a8713
342a3a9
ad80585
7c924fe
4a695de
32f1016
ea46eb0
e3120d2
e010ed6
2e01ed5
2e085b8
e7a3c98
20e2b4c
b25749e
95acc80
e7f6734
fc2aa81
39b881d
2672e12
a27251b
a146548
5416d26
02ba46c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,8 @@ import { | |
DetachEvent, | ||
Modify, | ||
MoveId, | ||
Revive, | ||
Insert, | ||
} from "./format"; | ||
import { GapTracker, IndexTracker } from "./tracker"; | ||
import { MarkListFactory } from "./markListFactory"; | ||
|
@@ -57,6 +59,7 @@ import { | |
withRevision, | ||
markEmptiesCells, | ||
splitMark, | ||
markIsTransient, | ||
} from "./utils"; | ||
|
||
/** | ||
|
@@ -118,6 +121,7 @@ function composeMarkLists<TNodeChange>( | |
revisionMetadata, | ||
(a, b) => composeChildChanges(a, b, newRev, composeChild), | ||
); | ||
const inputIndex = new IndexTracker(revisionMetadata.getIndex); | ||
while (!queue.isEmpty()) { | ||
const { baseMark, newMark } = queue.pop(); | ||
if (newMark === undefined) { | ||
|
@@ -127,6 +131,7 @@ function composeMarkLists<TNodeChange>( | |
); | ||
factory.push(baseMark); | ||
} else if (baseMark === undefined) { | ||
inputIndex.advance(newMark); | ||
factory.push(composeMark(newMark, newRev, composeChild)); | ||
} else { | ||
// Past this point, we are guaranteed that `newMark` and `baseMark` have the same length and | ||
|
@@ -136,12 +141,14 @@ function composeMarkLists<TNodeChange>( | |
baseMark, | ||
newRev, | ||
newMark, | ||
inputIndex, | ||
composeChild, | ||
genId, | ||
moveEffects, | ||
revisionMetadata, | ||
); | ||
factory.push(composedMark); | ||
inputIndex.advance(newMark); | ||
} | ||
} | ||
|
||
|
@@ -161,6 +168,7 @@ function composeMarks<TNodeChange>( | |
baseMark: Mark<TNodeChange>, | ||
newRev: RevisionTag | undefined, | ||
newMark: Mark<TNodeChange>, | ||
inputIndex: IndexTracker, | ||
composeChild: NodeChangeComposer<TNodeChange>, | ||
genId: IdAllocator, | ||
moveEffects: MoveEffectTable<TNodeChange>, | ||
|
@@ -173,6 +181,20 @@ function composeMarks<TNodeChange>( | |
composeChild, | ||
); | ||
|
||
const baseMarkIsTransient = markIsTransient(baseMark); | ||
const newMarkIsTransient = markIsTransient(newMark); | ||
if (baseMarkIsTransient || newMarkIsTransient) { | ||
if (newMarkIsTransient) { | ||
return withNodeChange(baseMark, nodeChange); | ||
} | ||
// TODO: Make `withNodeChange` preserve type information so we don't need to cast here | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we just need to merge the |
||
const nonTransient = withNodeChange(baseMark, nodeChange) as | ||
| Insert<TNodeChange> | ||
| Revive<TNodeChange>; | ||
delete nonTransient.detachedBy; | ||
return nonTransient; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to make assumptions about what kinds of marks |
||
} | ||
|
||
if (!markHasCellEffect(baseMark) && !markHasCellEffect(newMark)) { | ||
if (isNoopMark(baseMark)) { | ||
return withNodeChange(newMark, nodeChange); | ||
|
@@ -199,15 +221,7 @@ function composeMarks<TNodeChange>( | |
} | ||
return withNodeChange(baseMark, nodeChange); | ||
} else if (areInputCellsEmpty(baseMark)) { | ||
const moveInId = getMarkMoveId(baseMark); | ||
const moveOutId = getMarkMoveId(newMark); | ||
|
||
if (moveInId !== undefined && moveOutId !== undefined) { | ||
assert( | ||
isMoveMark(baseMark) && isMoveMark(newMark), | ||
0x68f /* Only move marks have move IDs */, | ||
); | ||
|
||
if (isMoveMark(baseMark) && isMoveMark(newMark)) { | ||
// `baseMark` must be a move destination since it is filling cells, and `newMark` must be a move source. | ||
const baseIntention = getIntention(baseMark.revision, revisionMetadata); | ||
const newIntention = getIntention(newMark.revision ?? newRev, revisionMetadata); | ||
|
@@ -245,8 +259,7 @@ function composeMarks<TNodeChange>( | |
return { count: 0 }; | ||
} | ||
|
||
if (moveInId !== undefined) { | ||
assert(isMoveMark(baseMark), 0x690 /* Only move marks have move IDs */); | ||
if (isMoveMark(baseMark)) { | ||
setReplacementMark( | ||
moveEffects, | ||
CrossFieldTarget.Source, | ||
|
@@ -258,9 +271,7 @@ function composeMarks<TNodeChange>( | |
return { count: 0 }; | ||
} | ||
|
||
if (moveOutId !== undefined) { | ||
assert(isMoveMark(newMark), 0x691 /* Only move marks have move IDs */); | ||
|
||
if (isMoveMark(newMark)) { | ||
// The nodes attached by `baseMark` have been moved by `newMark`. | ||
// We can represent net effect of the two marks by moving `baseMark` to the destination of `newMark`. | ||
setReplacementMark( | ||
|
@@ -273,8 +284,20 @@ function composeMarks<TNodeChange>( | |
); | ||
return { count: 0 }; | ||
} | ||
// TODO: Create modify mark for transient node. | ||
return { count: 0 }; | ||
|
||
assert(isDeleteMark(newMark), "Unexpected mark type"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could also assert |
||
const newMarkRevision = newMark.revision ?? newRev; | ||
assert(newMarkRevision !== undefined, "Unable to compose anonymous marks"); | ||
return withNodeChange( | ||
{ | ||
...(baseMark as Insert<TNodeChange> | Revive<TNodeChange>), | ||
detachedBy: { | ||
revision: newMarkRevision, | ||
index: inputIndex.getIndex(newMarkRevision), | ||
}, | ||
}, | ||
nodeChange, | ||
); | ||
} else { | ||
if (isMoveMark(baseMark) && isMoveMark(newMark)) { | ||
// The marks must be inverses, since `newMark` is filling the cells which `baseMark` emptied. | ||
|
@@ -515,48 +538,54 @@ export class ComposeQueue<T> { | |
const length = getOutputLength(baseMark); | ||
return this.dequeueBase(length); | ||
} else if (areOutputCellsEmpty(baseMark) && areInputCellsEmpty(newMark)) { | ||
// TODO: `baseMark` might be a MoveIn, which is not an ExistingCellMark. | ||
// See test "[Move ABC, Return ABC] ↷ Delete B" in sequenceChangeRebaser.spec.ts | ||
assert( | ||
isExistingCellMark(baseMark), | ||
0x693 /* Only existing cell mark can have empty output */, | ||
); | ||
let baseCellId: DetachEvent; | ||
if (markEmptiesCells(baseMark)) { | ||
assert(isDetachMark(baseMark), 0x694 /* Only detach marks can empty cells */); | ||
const baseRevision = baseMark.revision ?? this.baseMarks.revision; | ||
const baseIntention = getIntention(baseRevision, this.revisionMetadata); | ||
if (baseRevision === undefined || baseIntention === undefined) { | ||
// The base revision always be defined except when squashing changes into a transaction. | ||
// In the future, we want to support reattaches in the new change here. | ||
// We will need to be able to order the base mark relative to the new mark by looking at the lineage of the new mark | ||
// (which will be obtained by rebasing the reattach over interim changes | ||
// (which requires the local changes to have a revision tag)) | ||
let cmp: number; | ||
if (markIsTransient(baseMark)) { | ||
// TODO: call on `compareCellPositions` to determine ordering and overlap once it is fixed. | ||
cmp = Infinity; | ||
} else { | ||
// TODO: `baseMark` might be a MoveIn, which is not an ExistingCellMark. | ||
// See test "[Move ABC, Return ABC] ↷ Delete B" in sequenceChangeRebaser.spec.ts | ||
assert( | ||
isExistingCellMark(baseMark), | ||
0x693 /* Only existing cell mark can have empty output */, | ||
); | ||
if (markEmptiesCells(baseMark)) { | ||
assert(isDetachMark(baseMark), 0x694 /* Only detach marks can empty cells */); | ||
const baseRevision = baseMark.revision ?? this.baseMarks.revision; | ||
const baseIntention = getIntention(baseRevision, this.revisionMetadata); | ||
if (baseRevision === undefined || baseIntention === undefined) { | ||
// The base revision always be defined except when squashing changes into a transaction. | ||
// In the future, we want to support reattaches in the new change here. | ||
// We will need to be able to order the base mark relative to the new mark by looking at the lineage of the new mark | ||
// (which will be obtained by rebasing the reattach over interim changes | ||
// (which requires the local changes to have a revision tag)) | ||
assert( | ||
isNewAttach(newMark), | ||
0x695 /* TODO: Assign revision tags to each change in a transaction */, | ||
); | ||
return this.dequeueNew(); | ||
} | ||
baseCellId = { | ||
revision: baseIntention, | ||
index: this.baseIndex.getIndex(baseRevision), | ||
}; | ||
} else { | ||
assert( | ||
isNewAttach(newMark), | ||
0x695 /* TODO: Assign revision tags to each change in a transaction */, | ||
areInputCellsEmpty(baseMark), | ||
0x696 /* Mark with empty output must either be a detach or also have input empty */, | ||
); | ||
return this.dequeueNew(); | ||
baseCellId = baseMark.detachEvent; | ||
} | ||
baseCellId = { | ||
revision: baseIntention, | ||
index: this.baseIndex.getIndex(baseRevision), | ||
}; | ||
} else { | ||
assert( | ||
areInputCellsEmpty(baseMark), | ||
0x696 /* Mark with empty output must either be a detach or also have input empty */, | ||
cmp = compareCellPositions( | ||
baseCellId, | ||
baseMark, | ||
newMark, | ||
this.newRevision, | ||
this.cancelledInserts, | ||
this.baseGap, | ||
); | ||
baseCellId = baseMark.detachEvent; | ||
} | ||
const cmp = compareCellPositions( | ||
baseCellId, | ||
baseMark, | ||
newMark, | ||
this.newRevision, | ||
this.cancelledInserts, | ||
this.baseGap, | ||
); | ||
if (cmp < 0) { | ||
return { baseMark: this.baseMarks.dequeueUpTo(-cmp) }; | ||
} else if (cmp > 0) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -223,9 +223,22 @@ export interface HasRevisionTag { | |
} | ||
export const HasRevisionTag = Type.Object({ revision: Type.Optional(RevisionTagSchema) }); | ||
|
||
export interface Transient { | ||
/** | ||
* When populated, the content is inserted/revived then deleted. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems slightly confusing to say "when populated," as the field is not optional on this interface. |
||
* The contents of the `DetachEvent` describe deletion. | ||
*/ | ||
detachedBy: DetachEvent; | ||
} | ||
export const Transient = Type.Object({ detachedBy: DetachEvent }); | ||
|
||
export type CanBeTransient = Partial<Transient>; | ||
export const CanBeTransient = Type.Partial(Transient); | ||
|
||
export interface Insert<TNodeChange = NodeChangeType> | ||
extends HasTiebreakPolicy, | ||
HasRevisionTag, | ||
CanBeTransient, | ||
HasChanges<TNodeChange> { | ||
type: "Insert"; | ||
content: ProtoNode[]; | ||
|
@@ -240,6 +253,7 @@ export const Insert = <Schema extends TSchema>(tNodeChange: Schema) => | |
Type.Intersect([ | ||
HasTiebreakPolicy, | ||
HasRevisionTag, | ||
CanBeTransient, | ||
HasChanges(tNodeChange), | ||
Type.Object({ | ||
type: Type.Literal("Insert"), | ||
|
@@ -314,6 +328,7 @@ export const MoveOut = <Schema extends TSchema>(tNodeChange: Schema) => | |
export interface Revive<TNodeChange = NodeChangeType> | ||
extends HasReattachFields, | ||
HasRevisionTag, | ||
CanBeTransient, | ||
HasChanges<TNodeChange>, | ||
CellTargetingMark { | ||
type: "Revive"; | ||
|
@@ -324,6 +339,7 @@ export const Revive = <Schema extends TSchema>(tNodeChange: Schema) => | |
Type.Intersect([ | ||
HasReattachFields, | ||
HasRevisionTag, | ||
CanBeTransient, | ||
HasChanges(tNodeChange), | ||
CellTargetingMark, | ||
Type.Object({ | ||
|
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.
It looks like this could be moved before the outer if-block.