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

Move Presence from Client to Document #536

Closed
wants to merge 39 commits into from
Closed

Move Presence from Client to Document #536

wants to merge 39 commits into from

Conversation

chacha912
Copy link
Contributor

@chacha912 chacha912 commented Jun 7, 2023

What this PR does / why we need it?

Move Presence from Client to Document

Why?

yorkie-team/yorkie#442

  • Topics cannot be separated if a client attaches two different documents.
  • We can use transactions by combining the document and the presence. Let's see an example of using presence in text selection and drawing.

1. Text.SelectionMap

Since Text.SelectionMap is used to represent the user's selection in Text, it is unnecessary to store the selection in the document. Therefore, I changed to handle selection through presence instead of the Text.SelectionMap and select operation. (Related Issue: #366, #379)

When the text selection is changed: model → yorkie

  1. Edit operation:
  • as-is: When editing, the index is updated and the selection change is notified.
  • to-be: When editing, the updated index is returned. Then we convert the updated index to yorkie text position and send it through updatePresence.
// codemirror example
codemirror.on('beforeChange', (cm, change) => {
  // as-is
  doc.update((root) => {
    root.content.edit(from, to, content);
  });

  // to-be
  doc.update((root) => {
    const updatedIndex = root.content.edit(from, to, content);
    const [fromPos, toPos] = root.content.getRangePos(updatedIndex.from, updatedIndex.to,);
    doc.updatePresence({ selection: { from: fromPos, to: toPos }});
  });
});
  1. Select operation:
  • as-is: When selecting, the selection change is notified.
  • to-be: When selecting, we convert updated index to yorkie text position and send it through updatePresence.
codemirror.on('beforeSelectionChange', (cm, change) => {
  // as-is
  doc.update((root) => {
    root.content.select(from, to);
  });

  // to-be
  const [fromPos, toPos] = root.content.getRangePos(from, to);
  doc.updatePresence({ selection: { from: fromPos, to: toPos }});
});

Apply to model: yorkie → model

  • as-is: The text selection change is checked in remote-change.
  • to-be: The text selection change is checked in doc.subscribe('peers').
// as-is
doc.subscribe((event) => {
  if (event.type === 'remote-change') {
    const { actor, operations } = event.value;
    for (const op of operations) {
      if (op.type === 'edit') {
        // handle edit change
      } else if (op.type === 'select') {
        // handle select change
        displayRemoteSelection(codemirror, op, actor);
      }
    }
  }
});

// to-be
doc.subscribe('peers', (event) => {
  if (event.type === 'presence-changed') {
    const { clientID, presence } = event.peer;
    if (clientID === client.getID()) return;
    if (presence && presence.selection) {
      displayRemoteSelection(codemirror, doc, presence.selection.from, presence.selection.to, clientID );
    }
  }
});

2. Drawing

In the drawing example, by sending the shape information as presence during mouse move
and storing it in the document during mouse up, we can reduce the document history.
Previously, it was possible to send shape information as presence,
but due to the different timing of updates between presence and the document, there was a flickering issue.
With this PR, by moving the presence to the document, we can bundle document and presence changes into a single transaction, eliminating the flickering issue.

  • As-is: Flickering occurs during mouseup.
  • To-be:

Additional changes

client.attach('docKey')

Instead of creating and attaching the document separately,
both operations are now handled in the attach method.

  • As-is:
const doc = new yorkie.Document('docKey');
await client.attach(doc);
  • To-be:
const doc = await client.attach('docKey');

initial presence

Instead of providing initial presence when creating the client,
the initial presence is now passed when creating the document.

  • As-is:
client = new yorkie.Client('http://localhost:8080', {
  presence: {
    color: getRandomColor(),
  },
});
  • To-be:
const doc = await client.attach('docKey', {
  initialPresence: {
    color: getRandomColor(),
  },
});

subscribe peersChanged event

The peers-changed event has been moved from the client to the document.

  • As-is:
client.subscribe((event) => {
  if (event.type === 'peers-changed') {
    // event.value = {
    //   type: 'initialized' | 'watched' | 'unwatched' | 'presence-changed';
    //   peers: Record<DocumentKey, Array<{ clientID: ActorID; presence: P }>>;
    // }
  }
});
  • To-be:
doc.subscribe('peers', (event) => {
  // event = {
  //     type: 'watched' | 'unwatched' | 'presence-changed';
  //     peer: { clientID: ActorID; presence: P };
  // }
});

doc.getPeers()

Instead of retrieving peers from the client's perspective,
you can now get peers from the document.

  • As-is:
client.getPeersByDocKey(doc.getKey()) // Array<{ clientID: ActorID; presence: P }>
  • To-be:
doc.getPeers() // Array<{ clientID: ActorID; presence: P }>

What are the relevant tickets?

Fixes #366, Fixes #379

Related yorkie-team/yorkie#542

Checklist

  • Added relevant tests or not required
  • Didn't break anything

@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #536 (a300e36) into main (cf9287a) will decrease coverage by 71.04%.
The diff coverage is 17.68%.

@@             Coverage Diff             @@
##             main     #536       +/-   ##
===========================================
- Coverage   87.93%   16.90%   -71.04%     
===========================================
  Files          79       80        +1     
  Lines        7807     7892       +85     
  Branches      780      805       +25     
===========================================
- Hits         6865     1334     -5531     
- Misses        627     6337     +5710     
+ Partials      315      221       -94     
Impacted Files Coverage Δ
src/client/attachment.ts 47.05% <0.00%> (-30.72%) ⬇️
src/document/crdt/rga_tree_split.ts 1.47% <0.00%> (-90.39%) ⬇️
src/document/crdt/text.ts 4.21% <0.00%> (-83.15%) ⬇️
src/document/json/text.ts 0.00% <0.00%> (-45.46%) ⬇️
src/document/operation/edit_operation.ts 0.00% <0.00%> (-63.34%) ⬇️
src/document/operation/operation.ts 100.00% <ø> (ø)
src/document/time/ticket.ts 72.22% <0.00%> (-13.89%) ⬇️
src/yorkie.ts 100.00% <ø> (ø)
test/helper/helper.ts 0.00% <0.00%> (-83.68%) ⬇️
test/integration/array_test.ts 5.28% <0.00%> (-94.72%) ⬇️
... and 19 more

... and 46 files with indirect coverage changes

@hackerwins hackerwins closed this Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Place Text.selectionMap to presence, not document. Selection of clients should be shared as presence.
2 participants