Presence - Githubissues

curran commented 5 years ago

Closes issue #30

Builds on PR #25 by @houshuang

curran commented 5 years ago

@houshuang I'd like to request your review here, and also I invite you to merge this work back into your fork and continue working on it.

I ended up structuring the presence like this:

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

This is based on a suggestion from @josephg over in https://github.com/ottypes/json1/issues/9 . It feels pretty good so far. Note that this approach pushes the management of the user id (u) and change counting (c) down the the sub-presence, and these don't exist at all on the top-level json0 presence object. It feels good not to duplicate these. I was able to modify your implementation with Quill to make it work with this structure.

Cheers!

curran commented 5 years ago

I'm starting to wonder if maybe this would be a better structure:

{
  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 ]]
  }
}

The reason is, in the implementation there ends up being a bunch of logic for isolating these parts from the array, and building up the array from these parts. The implementation (and consumtion actually) of presence would be more straightforward with this structure. It would eliminate the need for the unpackPresence utility. Also, it would be similar to the existing structure of subtype ops.

curran commented 5 years ago

The end is in sight:

[x] Finalize text0 tests.
[x] Add tests for text0 operations within json0.
[x] Add tests to check that the ops are not mutated (due to subtype conversion).
[x] Update the sample app to work with the new structure.

Possible "Phase II" work (not a priority right now):

[ ] Transform correctly against ops that are not text (si, sd) or subtype ops.
- Includes li, ld, lm, oi, od

houshuang commented 5 years ago

Makes sense to me. Still wondering about this:

Does this mean we've decided to go with a single "cursor" per user, or just that we might have a lot of these presence objects per user? Would that cause some issue when it comes to cancelling presence when someone logs off, etc?

I would prefer a single presence object per user, and I would also like the user information at the top level, because in our case we will be showing a list of users at the left of the page. In fact, the initial presence sent will only be a top-level user object, with no path or subtype, meaning that the user has entered the complex document, but not yet put a cursor anywhere...

If we also need the user information duplicated at the subtype level I'm not sure... I understand that it might simplify a bit having an ottype which can both be used as a top-level type, and a delegated subtype, but I'm not sure this is a strong enough argument.

Anyway excited to check out more of your code - sorry my comments are coming kind of scattered, I'm alone with the kid this weekend :)

houshuang commented 5 years ago

I think there is already code in that json00 that handles subtypes, but anyway it shouldn't be hard... Main case is object or array deletion, if someone was present on that, then cancel their presence, and for array insertion, if inserted before the index of a presence, increment presence...

However, I don't currently have a use-case for this. What I do have a use-case for is just to specify presence at the key level - basically specifying a p, but not an s (according to your latest spec), which I think should be allowed. This would never need to be transformed. (I guess theoretically if someone deleted that entire path, the presence should be invalidated... I'm basically working on the assumption that documents have a fixed structure that will never change, and that it's only sub-paths from that fixed structure that might ever change, for example, if I specify a document as:

{ question1: {
  questionAnswer1: quill stuff},
  question2: { MCQ-answer2: { A: true, B: false, C: false }}
}

then all the stuff within ['question1', 'questionAnswer1'] and ['question2', 'MCQ-answer2'] could change in various ways. Here for question1, I would want a cursor managed by rich-text, for MCQ-answer2 I would just want to specify presence...

Theoretically it would be possible to delete the key MCQ-answer2 and thus invalidate my presence, however that would never happen in my code-base...

Not sure if this is relevant or not, just thinking aloud.

curran commented 5 years ago

Yes, the current approach here is single "cursor" per user.

Re: u and c at the top level, you raise an interesting case I haven't considered before. Namely when a user "enters" a document, but not any particular path. I could imagine one might want to display a user avatar or something, but not a cursor yet. This is the case where the sub-presence s and t could be undefined. Thanks for suggesting this!

Re: user information duplicated at the subtype level, it would be great to avoid this. However, it might make sense to have the json0 createPresence invoke the subtype createPresence, which in the case of ot-rich-text validates that there is a u property there. So, though it doesn't quite feel right, our only option may be to duplicate this u and c information across the top level as well as sub-presence objects. One way out of this would be to modify ot-rich-text such that it allows presence objects that do not have u and c.

curran commented 5 years ago

Published as https://www.npmjs.com/package/@datavis-tech/ot-json0 if anyone wants to try it out.

npm install -S @datavis-tech/ot-json0

curran commented 5 years ago

Here's a demo https://github.com/datavis-tech/json0-presence-demo

presence

selections

curran commented 5 years ago

I encountered a bug and spotted an opportunity to improve developer ergonomics in this implementation.

https://github.com/datavis-tech/json0-presence-demo/issues/2

Presence was not being transformed, and I did not know why. After some debugging, I realized the wrong type was being passed in. Meaning, when the json0 transformPresence looked for the subtype to transform the sub-presence by, it didn't find it and failed silently.

I propose to add a console.warn in the case that the presence type used doesn't have any registered subtypes that are able to transform it. It's not a fatal error, but having this library alert developers of this situation may save them time.

houshuang commented 5 years ago

@curran Hi. It seems to me that the ideal case would be that a subtype did not worry about whether it was receiving the presence directly from ShareDB, or "mediated" through ot-json0. Similarly, I think it would be ideal that users did not have to think too deeply about whether the rich text was a top-level document, or used as part of a json0 document, ie. all you should need to do to change the submitPresence function from a top-level document to a path in a json0 field would be to add a path: ['key'] to the object in submitPresence - do you agree with this goal?

In that case, the shape of the input to the transformPresence function in a subtype should be the same as the shape of the input to the transformPresence function in ot-json0...

However, it's currently not.

Let's say the transformPresence function in ot-json0 receives this input:

transformPresence({
  'text', // Path of the presence.
  '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 ]]
    }
}, 
[{
  p: ['text'],
  o: [
    {'retain': 31}, {'insert': 'hi'}
  ],
  t: 'rich-text'
}],  
  true
)

Then due to this line

(lib/json0.js:677)

s: subtypes[presence.t].transformPresence(presence.s, c.o, isOwnOp)

the subtype (rich-text) ot-type will be called with

transformPresence({
   u: '123',     // An example of an ot-rich-text presence object.
   c: 8,
   s: [ [ 1, 1 ], [ 5, 7 ]]
},
   [{'retain': 31}, {'insert': 'hi'}],
  true
}

In my branch I use

s: subtypes[presence.t].transformPresence(presence, [c], isOwnOp)

So that the op looks identical, and I have my subtype transform always look inside the s key, keeping the structure of my high-level rich text object identical...

What do you think?

curran commented 5 years ago

I'd be curious to compare how json0 and json1 compare in terms of the subtype op structure. I was trying to make the presence structure similar to the subtype op structure of json0.

There's also the idea of multiple presence entries simultaneously, which I think is a use case that would be nice to be able to support. For example, one user has presence in two editors at a time, one being a textarea and the other being a Quill document. I'm not sure if this ability should be at the level of the ShareDB implementation of presence, or the OT type.

dcworldwide commented 5 years ago

I'm following your work with great excitement. This is brilliant stuff.

ottypes / json0

Presence #31