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.

Sign up for GitHub

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 regressions in collaboration #1005

Merged
merged 2 commits into from
Dec 13, 2021
Merged

Fix regressions in collaboration #1005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf43868
Fix regressions in collaboration
trueadm Dec 13, 2021
e1a92fd
Revise logic
trueadm Dec 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 96 additions & 4 deletions packages/outline-react/src/__tests__/unit/useOutlineRichTextWithCollab.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ describe('useOutlineRichTextWithCollab', () => {
'<div contenteditable="true" data-outline-editor="true"><p dir="ltr"><span data-outline-text="true">Hello metaverse</span></p></div>',
);
expect(client1.getHTML()).toEqual(client2.getHTML());
expect(client1.getDocJSON()).toEqual({
root: '[object Object]Hello metaverse',
});
expect(client1.getDocJSON()).toEqual(client2.getDocJSON());

client1.stop();
Expand Down Expand Up @@ -138,9 +141,10 @@ describe('useOutlineRichTextWithCollab', () => {
expect(client1.getHTML()).toEqual(
'<div contenteditable="true" data-outline-editor="true"><p dir="ltr"><span data-outline-text="true">Hello worldHello world</span></p></div>',
);
expect(client2.getHTML()).toEqual(
'<div contenteditable="true" data-outline-editor="true"><p dir="ltr"><span data-outline-text="true">Hello worldHello world</span></p></div>',
);
expect(client1.getHTML()).toEqual(client2.getHTML());
expect(client1.getDocJSON()).toEqual({
root: '[object Object]Hello worldHello world',
});
expect(client1.getDocJSON()).toEqual(client2.getDocJSON());

client2.disconnect();
Expand Down Expand Up @@ -177,9 +181,97 @@ describe('useOutlineRichTextWithCollab', () => {
expect(client1.getHTML()).toEqual(
'<div contenteditable="true" data-outline-editor="true"><p dir="ltr"><span data-outline-text="true">Hello world!</span></p></div>',
);
expect(client1.getHTML()).toEqual(client2.getHTML());
expect(client1.getDocJSON()).toEqual({
root: '[object Object]Hello world!',
});
expect(client1.getDocJSON()).toEqual(client2.getDocJSON());

client1.stop();
client2.stop();
});

it('Should collaborate basic text deletion conflicts between two clients', async () => {
const connector = createTestConnection();
const client1 = connector.createClient('1');
const client2 = connector.createClient('2');

client1.start(container);
client2.start(container);

await exepctCorrectInitialContent(client1, client2);

// Insert some a text node on client 1
await waitForReact(() => {
client1.update(() => {
const root = $getRoot();
const paragraph = root.getFirstChild();
const text = $createTextNode('Hello world');
paragraph.append(text);
});
});

expect(client1.getHTML()).toEqual(
'<div contenteditable="true" data-outline-editor="true"><p dir="ltr"><span data-outline-text="true">Hello world</span></p></div>',
);
expect(client1.getHTML()).toEqual(client2.getHTML());
expect(client1.getDocJSON()).toEqual({
root: '[object Object]Hello world',
});
expect(client1.getDocJSON()).toEqual(client2.getDocJSON());

client1.disconnect();

// Delete the text on client 1
await waitForReact(() => {
client1.update(() => {
const root = $getRoot();
const paragraph = root.getFirstChild();
paragraph.getFirstChild().remove();
});
});

expect(client1.getHTML()).toEqual(
'<div contenteditable="true" data-outline-editor="true"><p><br></p></div>',
);
expect(client2.getHTML()).toEqual(
'<div contenteditable="true" data-outline-editor="true"><p dir="ltr"><span data-outline-text="true">Hello world!</span></p></div>',
'<div contenteditable="true" data-outline-editor="true"><p dir="ltr"><span data-outline-text="true">Hello world</span></p></div>',
);

// Insert some text on client 2
await waitForReact(() => {
client2.update(() => {
const root = $getRoot();
const paragraph = root.getFirstChild();
paragraph.getFirstChild().spliceText(11, 0, 'Hello world');
});
});

expect(client1.getHTML()).toEqual(
'<div contenteditable="true" data-outline-editor="true"><p><br></p></div>',
);
expect(client2.getHTML()).toEqual(
'<div contenteditable="true" data-outline-editor="true"><p dir="ltr"><span data-outline-text="true">Hello worldHello world</span></p></div>',
);

await waitForReact(() => {
client1.connect();
});

// TODO we can probably handle these conflicts better. We could keep around
// a "fallback" {Map} when we remove text without any adjacent text nodes. This
// would require big changes in `CollabElementNode.splice` and also need adjustments
// in `CollabElementNode.applyChildrenYjsDelta` to handle the existance of these
// fallback maps. For now though, if a user clears all text nodes from an element
// and another user inserts some text into the same element at the same time, the
// deletion will take precedence on conflicts.
expect(client1.getHTML()).toEqual(
'<div contenteditable="true" data-outline-editor="true"><p><br></p></div>',
);
expect(client1.getHTML()).toEqual(client2.getHTML());
expect(client1.getDocJSON()).toEqual({
root: '',
});
expect(client1.getDocJSON()).toEqual(client2.getDocJSON());

client1.stop();
Expand Down
39 changes: 34 additions & 5 deletions packages/outline-yjs/src/CollabElementNode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* @flow strict
*/

import type {TextOperation, Map as YMap, XmlText, XmlElement} from 'yjs';
import type {TextOperation, XmlText, XmlElement} from 'yjs';
import type {
ElementNode,
NodeKey,
Expand Down Expand Up @@ -35,6 +35,7 @@ import {
$isDecoratorNode,
} from 'outline';
import {CollabDecoratorNode} from './CollabDecoratorNode';
import {Map as YMap} from 'yjs';

export class CollabElementNode {
_key: NodeKey;
Expand Down Expand Up @@ -139,15 +140,30 @@ export class CollabElementNode {
deletionSize -= 1;
} else if (node instanceof CollabTextNode) {
const delCount = Math.min(deletionSize, length);
if (offset === 0 && delCount === node.getSize()) {
const prevCollabNode =
nodeIndex !== 0 ? children[nodeIndex - 1] : null;
const nodeSize = node.getSize();

if (
offset === 0 &&
delCount === 1 &&
nodeIndex > 0 &&
prevCollabNode instanceof CollabTextNode &&
length === nodeSize
) {
// Merge the text node with previous.
prevCollabNode._text += node._text;
children.splice(nodeIndex, 1);
} else if (offset === 0 && delCount === nodeSize) {
// The entire thing needs removing
children.splice(nodeIndex, 1);
} else {
node._text = spliceString(node._text, offset, delCount, '');
}
deletionSize -= delCount;
} else {
throw new Error('Should never happen for ' + String(node));
// Can occur due to the deletion from the dangling text heuristic below.
break;
}
}
} else if (insertDelta != null) {
Expand All @@ -161,7 +177,15 @@ export class CollabElementNode {
if (node instanceof CollabTextNode) {
node._text = spliceString(node._text, offset, 0, insertDelta);
} else {
throw new Error('Should never happen');
// TODO: maybe we can improve this by keeping around a redundant
// text node map, rather than removing all the text nodes, so there
// never can be dangling text.

// We have a conflict where there was likely a CollabTextNode and
// an Outline TextNode too, but they were removed in a merge. So
// let's just ignore the text and trigger a removal for it from our
// shared type.
this._xmlText.delete(offset, insertDelta.length);
}
currIndex += insertDelta.length;
} else {
Expand Down Expand Up @@ -365,6 +389,7 @@ export class CollabElementNode {
const nextChildren = nextOutlineNode.__children;
const prevEndIndex = prevChildren.length - 1;
const nextEndIndex = nextChildren.length - 1;
const collabNodeMap = binding.collabNodeMap;
let prevChildrenSet: void | Set<NodeKey>;
let nextChildrenSet: void | Set<NodeKey>;
let prevIndex = 0;
Expand Down Expand Up @@ -408,6 +433,7 @@ export class CollabElementNode {
nextChildNode,
this,
);
collabNodeMap.set(nextKey, collabNode);
if (prevHasNextKey) {
this.splice(binding, nextIndex, 1, collabNode);
prevIndex++;
Expand All @@ -432,9 +458,10 @@ export class CollabElementNode {
this,
);
this.append(collabNode);
collabNodeMap.set(key, collabNode);
}
} else if (removeOldChildren && !appendNewChildren) {
for (let i = prevEndIndex; i >= prevIndex; i--) {
for (let i = this._children.length - 1; i >= nextIndex; i--) {
this.splice(binding, i, 1);
}
}
Expand Down Expand Up @@ -494,6 +521,8 @@ export class CollabElementNode {
}
const xmlText = this._xmlText;
if (delCount !== 0) {
// What if we delete many nodes, don't we need to get all their
// sizes?
xmlText.delete(offset, child.getSize());
}
if (collabNode instanceof CollabElementNode) {
Expand Down
7 changes: 2 additions & 5 deletions packages/outline-yjs/src/SyncEditorStates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,11 +178,7 @@ function handleNormalizationMergeConflicts(
}
const parent = collabNode._parent;
collabNode._normalized = true;
// Only try and delete the collab node if its backing
// map is not empty;
if (collabNode._map._length !== 0) {
parent._xmlText.delete(offset, 1);
}
parent._xmlText.delete(offset, 1);
collabNodeMap.delete(nodeKey);
const parentChildren = parent._children;
const index = parentChildren.indexOf(collabNode);
Expand All @@ -206,6 +202,7 @@ export function syncOutlineUpdateToYjs(
normalizedNodes: Set<NodeKey>,
tags: Set<string>,
): void {
window.foo = binding.doc;
binding.doc.transact(() => {
currEditorState.read(() => {
// We check if the update has come from a origin where the origin
Expand Down