Clean up methods related to presence

public/quill.html

@@ -110,8 +110,8 @@
           });
           doc.subscribe('others', (event) => {
             if (event.type === 'unwatched') {
-              const { clientID } = event.value;
-              cursors.removeCursor(doc.getPresence(clientID).username);
+              const { clientID, presence } = event.value;
+              cursors.removeCursor(presence.username);
             } else if (event.type === 'presence-changed') {
               const { clientID, presence } = event.value;
               const range = doc
@@ -222,8 +222,13 @@
                   to = from + op.delete;
                   console.log(`%c local: ${from}-${to}: ''`, 'color: green');
 
-                  doc.update((root) => {
-                    root.content.edit(from, to, '');
+                  doc.update((root, presence) => {
+                    const range = root.content.edit(from, to, '');
+                    if (range) {
+                      presence.set({
+                        selection: root.content.indexRangeToPosRange(range),
+                      });
+                    }
                   }, `update content by ${client.getID()}`);
                 } else if (op.retain !== undefined) {
                   from = to + op.retain;

src/client/client.ts

@@ -892,6 +892,8 @@ export class Client implements Observable<ClientEvent> {
         break;
       case PbDocEventType.DOC_EVENT_TYPE_DOCUMENTS_WATCHED:
         attachment.doc.addOnlineClient(publisher);
+        // NOTE(chacha912): We added to onlineClients, but we won't trigger watched event
+        // unless we also know their initial presence data at this point.
         if (attachment.doc.hasPresence(publisher)) {
           attachment.doc.publish({
             type: DocEventType.Watched,
@@ -903,11 +905,16 @@ export class Client implements Observable<ClientEvent> {
         }
         break;
       case PbDocEventType.DOC_EVENT_TYPE_DOCUMENTS_UNWATCHED: {
+        const presence = attachment.doc.getPresence(publisher);
         attachment.doc.removeOnlineClient(publisher);
-        attachment.doc.publish({
-          type: DocEventType.Unwatched,
-          value: { clientID: publisher },
-        });
+        // NOTE(chacha912): There is no presence, when PresenceChange(clear) is applied before unwatching.
+        // In that case, the 'unwatched' event is triggered while handling the PresenceChange.
+        if (presence) {
+          attachment.doc.publish({
+            type: DocEventType.Unwatched,
+            value: { clientID: publisher, presence },
+          });
+        }
         break;
       }
     }

src/document/document.ts

@@ -142,7 +142,7 @@ export type DocEvent<P extends Indexable = Indexable, T = OperationInfo> =
   | RemoteChangeEvent<T>
   | InitializedEvent<P>
   | WatchedEvent<P>
-  | UnwatchedEvent
+  | UnwatchedEvent<P>
   | PresenceChangedEvent<P>;
 
 /**
@@ -223,9 +223,9 @@ export interface WatchedEvent<P extends Indexable> extends BaseDocEvent {
   value: { clientID: ActorID; presence: P };
 }
 
-export interface UnwatchedEvent extends BaseDocEvent {
+export interface UnwatchedEvent<P extends Indexable> extends BaseDocEvent {
   type: DocEventType.Unwatched;
-  value: { clientID: ActorID };
+  value: { clientID: ActorID; presence: P };
 }
 
 export interface PresenceChangedEvent<P extends Indexable>
@@ -446,23 +446,21 @@ export class Document<T, P extends Indexable = Indexable> {
       this.clone!.root,
       message,
     );
+    const actorID = this.changeID.getActorID()!;
 
     try {
       const proxy = createJSON<JSONObject<T>>(
         context,
         this.clone!.root.getObject(),
       );
 
-      if (!this.presences.has(this.changeID.getActorID()!)) {
-        this.clone!.presences.set(this.changeID.getActorID()!, {} as P);
+      if (!this.presences.has(actorID)) {
+        this.clone!.presences.set(actorID, {} as P);
       }
 
       updater(
         proxy,
-        new Presence(
-          context,
-          this.clone!.presences.get(this.changeID.getActorID()!)!,
-        ),
+        new Presence(context, this.clone!.presences.get(actorID)!),
       );
     } catch (err) {
       // drop clone because it is contaminated.
@@ -487,7 +485,7 @@ export class Document<T, P extends Indexable = Indexable> {
           value: {
             message: change.getMessage() || '',
             operations: opInfos,
-            actor: change.getID().getActorID(),
+            actor: actorID,
           },
         });
       }
@@ -496,8 +494,8 @@ export class Document<T, P extends Indexable = Indexable> {
         this.publish({
           type: DocEventType.PresenceChanged,
           value: {
-            clientID: change.getID().getActorID()!,
-            presence: this.getPresence(change.getID().getActorID()!)!,
+            clientID: actorID,
+            presence: this.getPresence(actorID)!,
           },
         });
       }
@@ -975,21 +973,40 @@ export class Document<T, P extends Indexable = Indexable> {
       let changeInfo: ChangeInfo | undefined;
       let docEvent: DocEvent<P> | undefined;
       const actorID = change.getID().getActorID()!;
-      if (change.hasPresenceChange()) {
+      if (change.hasPresenceChange() && this.onlineClients.has(actorID)) {
         const presenceChange = change.getPresenceChange()!;
-        if (
-          presenceChange.type === PresenceChangeType.Put &&
-          this.onlineClients.has(actorID)
-        ) {
-          docEvent = {
-            type: this.presences.has(actorID)
-              ? DocEventType.PresenceChanged
-              : DocEventType.Watched,
-            value: {
-              clientID: actorID,
-              presence: presenceChange.presence,
-            },
-          };
+        switch (presenceChange.type) {
+          case PresenceChangeType.Put:
+            // NOTE(chacha912): When the user exists in onlineClients, but
+            // their presence was initially absent, we can consider that we have
+            // received their initial presence, so trigger the 'watched' event.
+            docEvent = {
+              type: this.presences.has(actorID)
+                ? DocEventType.PresenceChanged
+                : DocEventType.Watched,
+              value: {
+                clientID: actorID,
+                presence: presenceChange.presence,
+              },
+            };
+            break;
+          case PresenceChangeType.Clear:
+            // NOTE(chacha912): When the user exists in onlineClients, but
+            // PresenceChange(clear) is received, we can consider it as detachment
+            // occurring before unwatching.
+            // Detached user is no longer participating in the document, we remove
+            // them from the online clients and trigger the 'unwatched' event.
+            docEvent = {
+              type: DocEventType.Unwatched,
+              value: {
+                clientID: actorID,
+                presence: this.getPresence(actorID)!,
+              },
+            };
+            this.removeOnlineClient(actorID);
+            break;
+          default:
+            break;
         }
       }
 
@@ -1047,27 +1064,35 @@ export class Document<T, P extends Indexable = Indexable> {
 
   /**
    * `setOnlineClients` sets the given online client set.
+   *
+   * @internal
    */
   public setOnlineClients(onlineClients: Set<ActorID>) {
     this.onlineClients = onlineClients;
   }
 
   /**
    * `addOnlineClient` adds the given clientID into the online client set.
+   *
+   * @internal
    */
   public addOnlineClient(clientID: ActorID) {
     this.onlineClients.add(clientID);
   }
 
   /**
    * `removeOnlineClient` removes the clientID from the online client set.
+   *
+   * @internal
    */
   public removeOnlineClient(clientID: ActorID) {
     this.onlineClients.delete(clientID);
   }
 
   /**
    * `hasPresence` returns whether the given clientID has a presence or not.
+   *
+   * @internal
    */
   public hasPresence(clientID: ActorID): boolean {
     return this.presences.has(clientID);
@@ -1081,14 +1106,28 @@ export class Document<T, P extends Indexable = Indexable> {
       return {} as P;
     }
 
-    return this.presences.get(this.changeID.getActorID()!)!;
+    const p = this.presences.get(this.changeID.getActorID()!)!;
+    return deepcopy(p);
   }
 
   /**
    * `getPresence` returns the presence of the given clientID.
    */
   public getPresence(clientID: ActorID): P | undefined {
-    return this.presences.get(clientID);
+    if (!this.onlineClients.has(clientID)) return;
+    const p = this.presences.get(clientID);
+    return p ? deepcopy(p) : undefined;
+  }
+
+  /**
+   * `getPresenceForTest` returns the presence of the given clientID
+   * regardless of whether the client is online or not.
+   *
+   * @internal
+   */
+  public getPresenceForTest(clientID: ActorID): P | undefined {
+    const p = this.presences.get(clientID);
+    return p ? deepcopy(p) : undefined;
   }
 
   /**
@@ -1100,7 +1139,7 @@ export class Document<T, P extends Indexable = Indexable> {
       if (this.presences.has(clientID)) {
         presences.push({
           clientID,
-          presence: this.presences.get(clientID)!,
+          presence: deepcopy(this.presences.get(clientID)!),
         });
       }
     }

test/helper/helper.ts

@@ -23,6 +23,14 @@ import { OperationInfo } from '@yorkie-js-sdk/src/document/operation/operation';
 
 export type Indexable = Record<string, any>;
 
+export async function sleep(interval = 1000): Promise<void> {
+  return new Promise((resolve) => {
+    setTimeout(() => {
+      resolve();
+    }, interval);
+  });
+}
+
 export async function waitStubCallCount(
   stub: sinon.SinonStub,
   callCount: number,
@@ -31,12 +39,13 @@ export async function waitStubCallCount(
     const doLoop = () => {
       if (stub.callCount >= callCount) {
         resolve();
+        return;
       }
-      return false;
+
+      setTimeout(doLoop, 0);
     };
-    if (!doLoop()) {
-      setTimeout(doLoop, 1000);
-    }
+
+    doLoop();
   });
 }
 

test/integration/client_test.ts

@@ -171,9 +171,10 @@ describe('Client', function () {
     });
 
     await waitStubCallCount(stubD2, 1); // d2 should be able to update
-    assert.equal(d2Events.pop(), DocEventType.LocalChange);
+
+    assert.equal(d2Events.at(-1), DocEventType.LocalChange);
     await waitStubCallCount(stubD1, 1); // d1 should be able to receive d2's update
-    assert.equal(d1Events.pop(), DocEventType.RemoteChange);
+    assert.equal(d1Events.at(-1), DocEventType.RemoteChange);
     assert.equal(d1.toSortedJSON(), d2.toSortedJSON());
 
     // Simulate network error
@@ -193,10 +194,11 @@ describe('Client', function () {
     });
 
     await waitStubCallCount(stubD2, 2); // d2 should be able to update
-    assert.equal(d2Events.pop(), DocEventType.LocalChange);
+    assert.equal(d2Events.at(-1), DocEventType.LocalChange);
     await waitStubCallCount(stubC2, 2); // c2 should fail to sync
+
     assert.equal(
-      c2Events.pop(),
+      c2Events.at(-1),
       DocumentSyncResultType.SyncFailed,
       'c2 sync fail',
     );
@@ -205,7 +207,7 @@ describe('Client', function () {
       assert.equal(err.message, 'INVALID_STATE_ERR - 0'); // c1 should also fail to sync
     });
     assert.equal(
-      c1Events.pop(),
+      c1Events.at(-1),
       DocumentSyncResultType.SyncFailed,
       'c1 sync fail',
     );
@@ -216,11 +218,16 @@ describe('Client', function () {
     xhr.restore();
 
     await waitStubCallCount(stubC2, 3); // wait for c2 to sync
-    assert.equal(c2Events.pop(), DocumentSyncResultType.Synced, 'c2 sync');
-    await waitStubCallCount(stubC1, 5);
-    assert.equal(c1Events.pop(), DocumentSyncResultType.Synced, 'c1 sync');
+    assert.equal(c2Events.at(-1), DocumentSyncResultType.Synced, 'c2 sync');
+    await waitStubCallCount(stubC1, 6);
+    assert.isTrue(
+      [c1Events.at(-1), c1Events.at(-2)].includes(
+        DocumentSyncResultType.Synced,
+      ),
+      'c1 sync',
+    );
     await waitStubCallCount(stubD1, 2);
-    assert.equal(d1Events.pop(), DocEventType.RemoteChange); // d1 should be able to receive d2's update
+    assert.equal(d1Events.at(-1), DocEventType.RemoteChange); // d1 should be able to receive d2's update
     assert.equal(d1.toSortedJSON(), d2.toSortedJSON());
 
     unsub1.client();
@@ -268,8 +275,12 @@ describe('Client', function () {
       root.version = 'v2';
     });
     await c1.sync();
-    await waitStubCallCount(stubC2, 2);
-    assert.equal(c2Events.pop(), ClientEventType.DocumentSynced);
+    await waitStubCallCount(stubC2, 3);
+    assert.isTrue(
+      [c2Events.at(-1), c2Events.at(-1)].includes(
+        ClientEventType.DocumentSynced,
+      ),
+    );
     assert.equal(d1.toSortedJSON(), d2.toSortedJSON());
     unsub1();
 

test/integration/document_test.ts

@@ -589,26 +589,26 @@ describe('Document', function () {
     d1.update((root) => {
       root['k1'] = [1, 2];
     }, 'set array');
-    await c1.attach(d1);
+    await c1.attach(d1, { isRealtimeSync: false });
     assert.equal(d1.toSortedJSON(), '{"k1":[1,2]}');
 
     const c2 = new yorkie.Client(testRPCAddr);
     await c2.activate();
     const d2 = new yorkie.Document<TestDoc>(docKey);
-    await c2.attach(d2);
+    await c2.attach(d2, { isRealtimeSync: false });
     assert.equal(d2.toSortedJSON(), '{"k1":[1,2]}');
 
     // 02. c1 updates d1 and removes it.
     d1.update((root) => {
       root['k1'].push(3);
     });
     await c1.remove(d1);
-    assert.equal(d1.toSortedJSON(), '{"k1":[1,2,3]}');
+    assert.equal(d1.toSortedJSON(), '{"k1":[1,2,3]}', 'd1');
     assert.equal(d1.getStatus(), DocumentStatus.Removed);
 
     // 03. c2 syncs and checks that d2 is removed.
     await c2.sync();
-    assert.equal(d2.toSortedJSON(), '{"k1":[1,2,3]}');
+    assert.equal(d2.toSortedJSON(), '{"k1":[1,2,3]}', 'd2');
     assert.equal(d2.getStatus(), DocumentStatus.Removed);
 
     await c1.deactivate();