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

transformPresence is not a function #423

Open
danijar opened this issue Jan 2, 2021 · 6 comments
Open

transformPresence is not a function #423

danijar opened this issue Jan 2, 2021 · 6 comments

Comments

@danijar
Copy link

danijar commented Jan 2, 2021

We're using ShareDB with the nickasd/sharedb-codemirror connector. The editor content is synchronized well and now we want to show other editor's cursors via ShareDB's presence feature.

Creating the doc on the server:

var doc = conn.get('docs', uuid.v4())
doc.create({content: '', metaData: {...}}, null, {}, err => {})

Connecting to the doc from the client:

var doc = conn.get('collection', doc_id)
doc.fetch(callback)
var presence = conn.getDocPresence('collection', doc.id)
var localPresence = presence.create(new ObjectID().toString())

Error message on document edit:

bundle.js:25816 Uncaught TypeError: Cannot read property 'content' of undefined
    at ShareDBCodeMirror.assertValue (bundle.js:25816)
    at ShareDBCodeMirror.afterLocalChanges (bundle.js:25925)
    at ShareDBCodeMirror.codeMirrorChanges (bundle.js:25728)
    at signal (bundle.js:12762)
    at endOperation_finish (bundle.js:16102)
    at endOperations (bundle.js:16003)
    at bundle.js:15986
    at finishOperation (bundle.js:14244)
    at endOperation (bundle.js:15983)
    at runInOp (bundle.js:16112)
assertValue @ bundle.js:25816
afterLocalChanges @ bundle.js:25925
ShareDBCodeMirror.codeMirrorChanges @ bundle.js:25728
signal @ bundle.js:12762
endOperation_finish @ bundle.js:16102
endOperations @ bundle.js:16003
(anonymous) @ bundle.js:15986
finishOperation @ bundle.js:14244
endOperation @ bundle.js:15983
runInOp @ bundle.js:16112
TextareaInput.poll @ bundle.js:21717
(anonymous) @ bundle.js:21502
bundle.js:25717 TypeError: this._doc.type.transformPresence is not a function
    at LocalDocPresence._transformAgainstOp (bundle.js:27856)
    at emitThree (bundle.js:23452)
    at Doc.emit (bundle.js:23511)
    at Doc._otApply (bundle.js:27357)
    at Doc._submit (bundle.js:27461)
    at Doc.submitOp (bundle.js:27562)
    at ShareDBCodeMirror.afterLocalChanges (bundle.js:25919)
    at ShareDBCodeMirror.codeMirrorChanges (bundle.js:25728)
    at signal (bundle.js:12762)
    at endOperation_finish (bundle.js:16102)
@curran
Copy link
Contributor

curran commented Jan 2, 2021

I've also faced this challenge, and have yet to do a proper implementation.

Here's what I got so far:

import { type } from 'ot-json0';
                               
type.transformPresence = function (presence, op, isOwnOp) {
  if (!presence) {
    return null;               
  }

  //console.log('TODO transform presence.');
  // Draw from https://github.com/datavis-tech/json0/tree/master/lib
  //
  //var start = presence.index;
  //var end = presence.index + presence.length;
  //var delta = new richText.Delta(op);
  //start = delta.transformPosition(start, !isOwnOp);
  //end = delta.transformPosition(end, !isOwnOp);

  //return Object.assign({}, presence, {
  //  index: start,            
  //  length: end - start      
  //});
  return presence;             
};

export { type };

This just makes it so ShareDB does not error, but it does not transform the presence properly.

Also tracked in ottypes/json0#30

@curran
Copy link
Contributor

curran commented Jan 2, 2021

Long story short:

  • Presence was added to ShareDB at the same time that it was implemented for the rich text type. So, presence works "out of the box" with rich text. @alecgibson did most of this work.
  • Presence was never added to the default OT type, json0.
  • A previous API for presence was introduced in the Teamwork ShareDB fork by @gkubisa (here's the unmerged ShareDB PR).
  • On top of that previous implementation, @houshuang and I got it to work, in this json0 PR: Presence.
  • The task at hand is to implement the current ShareDB presence API in json0.
  • The closest thing we have is this transformPresence implementation from the previously proposed presence API. The core of transformation is implemented there, but just needs to be refactored to match with the current API.

Pasting here for reference:

json.transformPresence = function(presence, op, isOwnOp) {
  // Don't transform path-only presence objects.
  if(!presence.t) return presence;

  for (var i = 0; i < op.length; i++) {
    var c = op[i];

    // convert old string ops to use subtype for backwards compatibility
    if (c.si != null || c.sd != null) {
      convertFromText(c);
    }

    // Transform against subtype ops.
    if (c.t && c.t === presence.t && json.pathMatches(c.p, presence.p)) {
      presence = Object.assign({}, presence, {
        s: subtypes[presence.t].transformPresence(presence.s, c.o, isOwnOp)
      });
    }

    // convert back to old string ops
    if (c.t === 'text0') {
      convertToText(c);
    }

    // TODO transform against non-subtype ops.
  };
  return presence;
};

It's a tricky problem, though, because we need consensus on what the shape of presence data should be for json0.

@danijar
Copy link
Author

danijar commented Jan 2, 2021

Thanks for the references, that's definitely helpful. We only need presence information for a text value inside the JSON object. In this case, there would be no problem about deciding what shape the presence data should take if I understand correctly. Do you have an idea whether that would be easy to do?

Besides this, I'm also wondering how important it is to transform the presence information locally at all. Wouldn't it be enough to store a mapping from user ID to arbitrary JSON alongside each doc, and have the ShareDB server delete keys when they lose their websocket connection? Is the problem that this would introduce a lag?

@curran
Copy link
Contributor

curran commented Jan 2, 2021

The transformation is required, because I notice with my implementation now (which also uses CodeMirror), the presence cursor for the remote user stays in the same place, even if I add a character before it. It does not move around along with the text, which it should. It feels buggy without the transformation.

For the shape of the presence, I ended up with something like this:

{
  p: ['some', 'path'], // Path of the presence.
  t: 'ot-rich-text',   // Subtype of the presence (a registered subtype).
  s: {                 // Opaque presence object (subtype-specific structure).
    u: '123',          // An example of an ot-rich-text presence object.
    c: 8,
    s: [ [ 1, 1 ], [ 5, 7 ]]
  }
}

Here's a demo project with this in action.

The task at hand is to upgrade that demo and the transformation logic to use the new ShareDB presence API. I think it's fairly similar, but I have not mapped out the details in full.

@danijar
Copy link
Author

danijar commented Jan 3, 2021

We wrote something without any of the presence features of ShareDB now. We just store a mapping users: {user_id: {timestamp: ..., name: ..., cursor_pos: ...}} as part of the doc. Every client writes its entry in the users dictionary and updates it when moving its cursor. We then show the cursors of other users in CodeMirror via setBookmark so that they flow with the text when making local edits.

We only show cursors whose timestamp is not older than some threshold. Ideally, we could make cursors disappear when the ShareDB server looses the WebSocket connection to them rather than via a timeout, but we haven't found the right middleware to hook into for this.

Thanks for the sharing the demo! It looks really clean and we will probably try switching to that when we run into problems with our solution.

We are experiencing some lag with the solution above, not for your own cursor but for text insertions and cursor movements of other users. The text does not appear character by character but in chunks of about 3-5 characters. We didn't see this lag before. Are you experiencing this with you demo as well?

@alecgibson
Copy link
Collaborator

Yes, sorry, we should definitely have a better error for this case. But yes, json0 doesn't yet have a presence implementation. Always happy to receive and review pull requests!

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

No branches or pull requests

3 participants