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

Proposal – Add support for presences #25

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Calyhre
Copy link

@Calyhre Calyhre commented Oct 16, 2020

Hello 👋 As stated in #9, ShareDB "recently" introduced presences in share/sharedb#322

The shape of the presence object is up to the OT type to define. So I propose a very simple one:

type Presence = {
  start: Path,
  end: Path,
}

This pull request propose this type and the following transformPresence implementation. This internally calls transformPosition for each end of the presence, and fallbacks on the transformPosition or transformCursor of the leaf subtypes if needed. There is a few examples in the tests for a better understanding.

We could also consider shortening the keys to match the size saving effect used by operations.

Let me know what you think!

@Calyhre
Copy link
Author

Calyhre commented Oct 22, 2020

I just changed a bit the implementation to also allow arbitrary data inside the presence without dropping it.
This allow the client to add implementation specific details to presences, like user id, colors etc.

What do you think of this proposal @josephg?

@willmtl
Copy link

willmtl commented Oct 23, 2020

@Calyhre Have you used it in production? I'm looking to upgrade from json0

@Calyhre
Copy link
Author

Calyhre commented Oct 26, 2020

@Calyhre Have you used it in production? I'm looking to upgrade from json0

This ot-type yes, we are using it at Slite.com. As for the presence patch, not yet.

@ianberdin
Copy link

Sorry, but why it isn't merged, yet?

if (et.transformPosition) path[i] = et.transformPosition(path[i], e)
if (et.transformPosition) {
path[i] = et.transformPosition(path[i], e)
} else if (et.transformCursor) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where would transformCursor come from? Is this for rich text?

Copy link
Author

Choose a reason for hiding this comment

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

Exactly, not ideal but they renamed it there: https://github.com/ottypes/rich-text/blob/master/lib/type.js#L39-L41

Copy link
Contributor

@curran curran Aug 2, 2021

Choose a reason for hiding this comment

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

Ah I see.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, I got it backward. As per the main OTType documentation it's transformCursor that the type should implement. And it's text-unicode (the default subtype in JSON1), that is missing it but implements transformPosition.

@curran
Copy link
Contributor

curran commented Jul 30, 2021

This looks like interesting work! Eager to try it out.

I'm curious, @Calyhre do you happen to have any interactive prototype that uses this that we could try?

Here's an interactive demo that @houshuang and I came up with for the JSON0 presence work we did 2 years ago:

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

It would be interesting to see this JSON1 presence work inside a working demo like that one. That demo code could probably be modified to use JSON1 and this implementation.

Another take - might it be possible to implement presence by extending JSON1 from an external package, rather than modifying JSON1 itself?

@josephg
Copy link
Member

josephg commented Jul 31, 2021

Sorry, but why it isn't merged, yet?

I've dropped the ball on this. Sorry everyone! I'll take a look soon.

@Calyhre
Copy link
Author

Calyhre commented Aug 2, 2021

I'm curious, @Calyhre do you happen to have any interactive prototype that uses this that we could try?

Didn't made any other than implementing it on Slite.com using that very same code.

Another take - might it be possible to implement presence by extending JSON1 from an external package, rather than modifying JSON1 itself?

While I still think the transformPosition logic should stay inside json1, we actually patched it for our own usage:

JSON1.transformPresence = json1TransformPresence

json1TransformPresence being the code of this PR.

@curran
Copy link
Contributor

curran commented Aug 2, 2021

we actually patched it for our own usage:

Very nice! That's a great solution. In that case, it may make sense to publish this work as a separate package, and have this sort of patching be the way that it's used.

transformPosition logic should stay inside json1

Would it work to patch rich text externally with something like richText.transformPosition = richText.transformCursor, and leave this code here unchanged?

@Calyhre
Copy link
Author

Calyhre commented Aug 2, 2021

So here is were things get a bit mixed up:

  • The original OTType documentation is listing transformCursor as one of the optional method a type can implement.
  • ShareDB is using transformPresence, which might represents a point in content, a range, or any arbitrary data
  • Neither ot-json1 nor ot-text-unicode supports transformCursor prior to this PR, neither do they implement transformPresence

I understood it that way: It's up to the type to implement and support the transformPresence the way it makes sense. Depending on how the type is being used, a presence can be represented with completely different meanings, so leaving the implementation to a third party library also make sense.

However, this PR is a proposal for the minimum viable API to support transformPresence in ot-json1 for what I think might be the most common use cases (start + end + arbitrary data). Obviously, it's still over-writable by the implementer if too limited, but I believe this provides a good default to be compatible with presences out of the box.

@curran
Copy link
Contributor

curran commented Nov 19, 2021

I'm interested in re-visiting the challenge of presence with JSON1.

@curran
Copy link
Contributor

curran commented Feb 25, 2023

Again, I'm picking this up, this time in the context of this open source web-based IDE: https://github.com/vizhub-core/vzcode with @Anooj-Pai.

Tracking the presence task here: vizhub-core/vzcode#16

I'm curious to try out the changes in this PR and see if it works for our case. Since it's not merged, I may create a fork of JSON1 based on this and then iterate from there.

From previous conversations here:

I'm curious, @Calyhre do you happen to have any interactive prototype that uses this that we could try?
Didn't made any other than implementing it on Slite.com using that very same code.

This, I suppose, would be the first step in verifying that this works. I'll try implementing it in VZCode and will report here how it goes. If we could get this eventually merged that would be amazing! But I understand the hesitancy to merge this without any fully working open source example based on it.

This was referenced Feb 25, 2023
@curran
Copy link
Contributor

curran commented Mar 4, 2023

I'm considering maintaining a fork with this work. I've been trying to test this in this VZCode project which uses Vite as the build tool, but am having all sorts of problems relating to the legacy build requirements (CommonJS + Node polyfills due to the process reference). Also, I realized that the OT type name and URI should be updated, so as not to conflict with the published version of JSON1.

My game plan at the moment for implementing presence in a project that uses Vite as its build system:

@curran
Copy link
Contributor

curran commented Mar 4, 2023

Suggest to update the "test" script in package.json to actually run the presence tests:

    "test": "mocha test/cursor.js test/test.js test/immutable.js test/presence.js",

@curran
Copy link
Contributor

curran commented Mar 4, 2023

@curran
Copy link
Contributor

curran commented Mar 8, 2023

The main substantive feedback on this that I have is that it does not support multiple selections.

I'm working on an integration with CodeMirror 6, which does support multiple selections.

So one of the limiting factors with the integration is that in case a user makes multiple selections, we need to just pick one to broadcast as presence.

Perhaps the shape of the presence could be changed from

type Presence = {
  start: Path,
  end: Path,
}

to

type PresenceSelection = {
  start: Path,
  end: Path,
}
type Presence = Array<PresenceSelection>;

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.

5 participants