Skip to content

Commit

Permalink
Fix incorrect calculation in indexTree.treePosToPath operation (#751)
Browse files Browse the repository at this point in the history
`TreeChange.Path` is calculated as one offset based on contents even
if the elements have several text nodes. This commit ensures that
elements with text nodes return the position of the content rather
than the offset of children.

---------

Co-authored-by: Youngteac Hong <[email protected]>
  • Loading branch information
raararaara and hackerwins authored Feb 21, 2024
1 parent 16bbe87 commit 5c8e56f
Show file tree
Hide file tree
Showing 2 changed files with 161 additions and 3 deletions.
16 changes: 13 additions & 3 deletions src/util/index_tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,10 +252,12 @@ export abstract class IndexTreeNode<T extends IndexTreeNode<T>> {
}

/**
* `hasTextChild` returns true if the node has an text child.
* `hasTextChild` returns true if the node's children consist of only text children.
*/
hasTextChild(): boolean {
return this.children.some((child) => child.isText);
return (
this.children.length > 0 && this.children.every((child) => child.isText)
);
}

/**
Expand Down Expand Up @@ -804,8 +806,16 @@ export class IndexTree<T extends IndexTreeNode<T>> {
node.parent! as T,
offset,
);
node = node.parent!;
path.push(sizeOfLeftSiblings + treePos.offset);
node = node.parent!;
} else if (node.hasTextChild()) {
// TODO(hackerwins): The function does not consider the situation
// where Element and Text nodes are mixed in the Element's Children.
const sizeOfLeftSiblings = addSizeOfLeftSiblings(
node! as T,
treePos.offset,
);
path.push(sizeOfLeftSiblings);
} else {
path.push(treePos.offset);
}
Expand Down
148 changes: 148 additions & 0 deletions test/integration/tree_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1333,6 +1333,154 @@ describe('Tree.style', function () {
);
});
});

it('Should return correct range path within doc.subscribe', async function ({
task,
}) {
await withTwoClientsAndDocuments<{ t: Tree }>(async (c1, d1, c2, d2) => {
d1.update((root) => {
root.t = new Tree({
type: 'r',
children: [
{
type: 'c',
children: [
{
type: 'u',
children: [
{
type: 'p',
children: [
{
type: 'n',
children: [],
},
],
},
],
},
],
},
{
type: 'c',
children: [
{
type: 'p',
children: [
{
type: 'n',
children: [],
},
],
},
],
},
],
});
});
await c1.sync();
await c2.sync();
assert.equal(
d1.getRoot().t.toXML(),
/*html*/ `<r><c><u><p><n></n></p></u></c><c><p><n></n></p></c></r>`,
);
assert.equal(
d2.getRoot().t.toXML(),
/*html*/ `<r><c><u><p><n></n></p></u></c><c><p><n></n></p></c></r>`,
);

d2.update((r) =>
r.t.editByPath([1, 0, 0, 0], [1, 0, 0, 0], {
type: 'text',
value: '1',
}),
);
d2.update((r) =>
r.t.editByPath([1, 0, 0, 1], [1, 0, 0, 1], {
type: 'text',
value: '2',
}),
);
d2.update((r) =>
r.t.editByPath([1, 0, 0, 2], [1, 0, 0, 2], {
type: 'text',
value: '3',
}),
);
await c2.sync();
await c1.sync();
assert.equal(
d1.getRoot().t.toXML(),
/*html*/ `<r><c><u><p><n></n></p></u></c><c><p><n>123</n></p></c></r>`,
);
assert.equal(
d2.getRoot().t.toXML(),
/*html*/ `<r><c><u><p><n></n></p></u></c><c><p><n>123</n></p></c></r>`,
);

d1.update((r) =>
r.t.editByPath([1, 0, 0, 1], [1, 0, 0, 1], {
type: 'text',
value: 'abcdefgh',
}),
);
await c1.sync();
await c2.sync();
assert.equal(
d1.getRoot().t.toXML(),
/*html*/ `<r><c><u><p><n></n></p></u></c><c><p><n>1abcdefgh23</n></p></c></r>`,
);
assert.equal(
d2.getRoot().t.toXML(),
/*html*/ `<r><c><u><p><n></n></p></u></c><c><p><n>1abcdefgh23</n></p></c></r>`,
);

d2.update((r) =>
r.t.editByPath([1, 0, 0, 5], [1, 0, 0, 5], {
type: 'text',
value: '4',
}),
);
d2.update((r) => r.t.editByPath([1, 0, 0, 6], [1, 0, 0, 7]));
d2.update((r) =>
r.t.editByPath([1, 0, 0, 6], [1, 0, 0, 6], {
type: 'text',
value: '5',
}),
);
await c2.sync();
await c1.sync();

const eventCollector = new EventCollector<{ type: DocEventType }>();
const unsub = d2.subscribe((event) => {
if (event.type === 'local-change' || event.type === 'remote-change') {
const operation = event.value.operations[0] as TreeEditOpInfo;
const { fromPath, toPath } = operation;
assert.deepEqual(fromPath, [1, 0, 0, 7]);
assert.deepEqual(toPath, [1, 0, 0, 8]);
eventCollector.add({ type: event.type });
}
});

d2.update((r) => r.t.editByPath([1, 0, 0, 7], [1, 0, 0, 8]));

await c2.sync();
await c1.sync();
assert.equal(
d1.getRoot().t.toXML(),
/*html*/ `<r><c><u><p><n></n></p></u></c><c><p><n>1abcd45gh23</n></p></c></r>`,
);
assert.equal(
d2.getRoot().t.toXML(),
/*html*/ `<r><c><u><p><n></n></p></u></c><c><p><n>1abcd45gh23</n></p></c></r>`,
);

await eventCollector.waitAndVerifyNthEvent(1, {
type: DocEventType.LocalChange,
});
unsub();
}, task.name);
});
});

describe('Tree.edit(concurrent overlapping range)', () => {
Expand Down

0 comments on commit 5c8e56f

Please sign in to comment.