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

CodeMirror 4 support? #112

Closed
benbro opened this issue Apr 1, 2014 · 11 comments
Closed

CodeMirror 4 support? #112

benbro opened this issue Apr 1, 2014 · 11 comments

Comments

@benbro
Copy link

benbro commented Apr 1, 2014

Hi

Any plans to support CodeMirror 4?
ot.js has few commits:
Operational-Transformation/ot.js@dfa9b5d

@mikelehen
Copy link
Collaborator

Yeah, we should definitely support CodeMirror 4. Not sure when I'll get
around to it, but if you or somebody else beats me to it, awesome. :-)

On Tue, Apr 1, 2014 at 11:08 AM, benbro notifications@github.com wrote:

Hi

Any plans to support CodeMirror 4?
ot.js has few commits:
Operational-Transformation/ot.js@dfa9b5dhttps://github.com/Operational-Transformation/ot.js/commit/dfa9b5dab43dfce89856a2e69369e74e64d1e53f

Reply to this email directly or view it on GitHubhttps://github.com//issues/112
.

@vincentwoo
Copy link
Collaborator

👍 i will pay someone TWENTY US DOLLARS to do this

@vincentwoo
Copy link
Collaborator

@benbro I'm testing FirePad on CodeMirror 4.1 and it seems to be running fine now (I think they removed some of the breaking 3.x -> 4.x stuff). Can you confirm?

Edit: Ah, there appear to be some problems in list indentation in rich text mode.

@benbro
Copy link
Author

benbro commented May 16, 2014

ot.js has some CodeMirror 4.0 commits so there are probably few changes needed
Operational-Transformation/ot.js@dfa9b5d

@vincentwoo
Copy link
Collaborator

@benbro I've made some commits to Firepad to enable CodeMirror 4 support. Can you test out the latest master?

@benbro
Copy link
Author

benbro commented May 17, 2014

listening to 'changes' instead of 'change' is not needed?
Maybe it will be faster or prevent race condition?
https://github.com/Operational-Transformation/ot.js/blob/master/lib/codemirror-adapter.js#L19

http://codemirror.net/doc/upgrade_v4.html

Rather than forcing client code to follow next pointers from one change object to the next, the library will now simply fire multiple "change" events. Existing code will probably continue to work unmodified.

Thank you for the update, I'll test it and report back.

@mikelehen
Copy link
Collaborator

Sounds like for the change/changes thing, you might need to trigger a "compound change" (made up of multiple individual changes) to be affected.

I think indenting a bunch of lines in the code editor would do that (it'll add a tab to the beginning of each line as a single compound change)

@vincentwoo
Copy link
Collaborator

@mikelehen I don't think we actually need to modify the change handler. It sounds like compound changes are gone entirely? Don't we just get one change event per actual change now?

@cben
Copy link
Contributor

cben commented May 20, 2014

We get a 1 or more 'change' calls.
We only need to use 'changes' if we care about processing several related
changes (~ one user action) together.

BTW in CM 3 it was also possible to get changes one-by-one without manually
walking the .next linked list - 'change' events on documents were never
chained.

I don't understand OT/firepad enough, but I assume since there was existing
code to walk .next, and since OT.js was changed to keep the ability to
process them together, there is a benefit in doing it.
Not sure whether it's just an optimization (compress communication & stored
history?) or improves user experience (probably wrt. undo?)

@vincentwoo
Copy link
Collaborator

Yeah, looks like change batching in CodeMirror 4 is actually messing with #109

@mikelehen
Copy link
Collaborator

Ahh, @vincentwoo, I see what you're saying (everything should still work
with the new 'change' events, it just won't be batched), which in general
seems like it should indeed be fine (it'll result in extra history items,
but that's not a big deal).

I'll let you figure out if there are actual issues (related or not) with
CM4 that we still need to deal with. :-)

On Mon, May 19, 2014 at 6:19 PM, Vincent Woo notifications@github.comwrote:

Yeah, looks like change batching in CodeMirror 4 is actually messing with
#109 #109


Reply to this email directly or view it on GitHubhttps://github.com//issues/112#issuecomment-43577101
.

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

4 participants