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

cursors with new presence API in share #3

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

cursors with new presence API in share #3

wants to merge 32 commits into from

Conversation

enjalot
Copy link

@enjalot enjalot commented Apr 10, 2014

I've updated this code to use the new presence API in Share. You get some nice stuff for free like a list of all currently connected sessions and you can store arbitrary data in the presence info for each session.

@aslakhellesoy
Copy link
Contributor

Can't wait to try this out! /cc @mattwynne @jbpros

@aslakhellesoy
Copy link
Contributor

I'd like to fix the broken tests on master before merging this in. I think they broke because one of the deps moved.

I can't cut an npm release while package.json has deps that point to a branch, since this will cause the lib to break when that branch moves. I need a release of sharejs+livedb, a tag or as a last resort - a sha.

What do you recommend for pinning share and livedb?

@josephg
Copy link
Member

josephg commented Apr 11, 2014

Yeah I agree. Maybe we should wait on merging until I merge the presence code in sharejs & livedb. There's still a few bugs I need to fix.

PS, for now you can try it out here

@dignifiedquire
Copy link

There is also the issue with CM4 that it supports multiple cursors so we need to look into that.

@josephg
Copy link
Member

josephg commented Apr 14, 2014

As in, sublime text-style cursors? Interesting.... do people use multiple cursors a lot? We could replace the selection range with a list of selection ranges, but if people don't use it much it might not be worth it.

@dignifiedquire
Copy link

I will be needing it as we are just in the phase of migrating to CodeMirror 4 with this being the main driver for the migration. So I'm all in favor.

@wmertens
Copy link

I too love multiple cursors. It's like a pre-emptive "record macro and then
apply it a bunch of times". I use it quite a bit to change variable names
or quoting styles etc.

On Mon, Apr 14, 2014 at 6:23 PM, Friedel Ziegelmayer <
notifications@github.com> wrote:

I will be needing it as we are just in the phase of migrating to
CodeMirror 4 with this being the main driver for the migration. So I'm all
in favor.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3#issuecomment-40386639
.

@aslakhellesoy
Copy link
Contributor

I don't see how CM4's multiple cursor/selection can be used here. Isn't the whole point to display cursors/selections with a distinct colour for each user? -And a marker above the cursor with the user's name?

I don't see how CodeMirror.setSelection and setCursor allows for special styling like this.

Or am I missing something?

@josephg
Copy link
Member

josephg commented Apr 14, 2014

Well, sharejs could let people display a marker over each cursor.

@dignifiedquire
Copy link

The point is not to use multiple cursors for display, but to display all cursors of a single user on the other users screen.

@domagojk
Copy link

I didn't noticed this discussion when I was creating pull request for codemirrors multiple curors support: #6 so if anyone here still needs it, with this fix it worked great for us.

@aslakhellesoy If you are still looking how to display cursors/selection with different colors for every user, yes I dont think how setCursor/setSelection could help but you can use addWidget for cursor and markText for selections.

Hope it helps

@aslakhellesoy
Copy link
Contributor

@domagojk the newcursors branch is already using addWidget and markText

The only reason we haven't merge this to master yet is that the presence API is still not on sharejs master.

As @dignifiedquire pointed out, the functionality of this branch allows me to see your cursor. In light of your PR #6 - what would you expect me to see if you have multiple cursors? All of them? Have you thought about how that would align with the proposed presence API?

@wmertens
Copy link

I think it would be very nice if you could see all the multicursors from a user, although it's not a problem if you only see the main one.
When you do multi-cursor edits, it will change a bunch of things all over the document, but it's only slightly confusing if that happens without a cursors.

@domagojk
Copy link

I agree with @wmertens , all of the cursors would be the best by me (since this is what the owner sees)
Honestly I didn't check proposed presence API, where can I find it?

I inspected message sent from client while using multiple cursors with #6 (using shareJs 0.7, req object in _handleMessage function). So this is example of char "z" written with 2 cursors:
{ a: 'op', v: 11, op: [ 25, 'z', 2, 'z' ] }

Could you tell from this would that be consistent with proposed presence API?

@morokhovets
Copy link

@aslakhellesoy
are there any chances this will be merged?
It looks like @josephg silently abandoned sharejs project and new owner (@nateps, I guess) has resumed activity but he hasnt touch presence branch which is very dangerous to all the work that is already done in this direction.
Should I go DIY route and implement cursors and presence by myself for my project or use this branch (and obsolete sharejs presence branch?)

@aslakhellesoy
Copy link
Contributor

Hi @efdi. Like you I have been waiting for presence support in ShareJS/LiveDB, but I don't have much hope that it will happen.

I did try to fix it myself on a fix-presence branch, but I find the code really difficult to work with and gave up.

I really don't know what to do.

@morokhovets
Copy link

@aslakhellesoy thanks for the reply. I think i'm going DIY.

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.

7 participants