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

Replace selection with presence #578

Merged
merged 3 commits into from
Jul 19, 2023
Merged

Conversation

chacha912
Copy link
Contributor

@chacha912 chacha912 commented Jul 19, 2023

What this PR does / why we need it?

This is a follow-up task of #574

Text Selection

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, presence) => {
    const range = root.content.edit(from, to, content); // 👉 return updated index range 
    presence.set({ 
      selection: root.content.indexRangeToPosRange(range), // 👉 update presence
    });
  });
});
  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);  // ⛔️ use select operation 
  });

  // to-be
  doc.update((root, presence) => {
    presence.set({
      selection: root.content.indexRangeToPosRange([from, to]), // 👉 update presence
    });
  });
});

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('others').
// 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') {
        displayRemoteSelection(codemirror, op, actor); // ⛔️ handle select operation
      }
    }
  }
});

// to-be
doc.subscribe('others', (event) => {
  if (event.type === 'presence-changed') { // 👉 handle presence change
    const { clientID, presence } = event.value;
    const range = doc.getRoot().content.posRangeToIndexRange(presence.selection);
  }
});

Tree Selection

// ...

Drawing example

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:

Any background context you want to provide?

What are the relevant tickets?

Fixes #366, Fixes #379

Related yorkie-team/yorkie#583

Checklist

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

@hackerwins hackerwins force-pushed the replace-selection-with-presence branch from 8c63f98 to 47ec95e Compare July 19, 2023 11:09
@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #578 (47ec95e) into main (9e0c431) will decrease coverage by 0.89%.
The diff coverage is 58.26%.

@@            Coverage Diff             @@
##             main     #578      +/-   ##
==========================================
- Coverage   88.30%   87.41%   -0.89%     
==========================================
  Files          81       81              
  Lines        7916     7937      +21     
  Branches      805      817      +12     
==========================================
- Hits         6990     6938      -52     
- Misses        613      684      +71     
- Partials      313      315       +2     
Impacted Files Coverage Δ
src/client/client.ts 75.34% <ø> (-1.82%) ⬇️
src/document/operation/select_operation.ts 0.00% <ø> (-50.00%) ⬇️
src/document/operation/style_operation.ts 54.16% <ø> (ø)
src/document/presence/presence.ts 83.33% <ø> (ø)
src/yorkie.ts 100.00% <ø> (ø)
test/integration/document_test.ts 98.98% <ø> (-0.01%) ⬇️
test/unit/document/crdt/tree_test.ts 69.08% <ø> (-0.47%) ⬇️
test/unit/document/document_test.ts 95.86% <ø> (-0.15%) ⬇️
src/document/json/text.ts 32.89% <27.77%> (-12.56%) ⬇️
src/document/json/tree.ts 57.22% <37.50%> (+2.36%) ⬆️
... and 11 more

... and 1 file with indirect coverage changes

@hackerwins hackerwins self-requested a review July 19, 2023 11:16
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

@hackerwins hackerwins merged commit 77083b8 into main Jul 19, 2023
2 checks passed
@hackerwins hackerwins deleted the replace-selection-with-presence branch July 19, 2023 11:17
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