Etherpad type2 #172

Merged
merged 8 commits into from Feb 27, 2013

Conversation

Projects
None yet
5 participants
@jucovschi
Contributor

jucovschi commented Feb 26, 2013

Here a remake of the Etherpad type integration.

@luto

This comment has been minimized.

Show comment Hide comment
@luto

luto Feb 26, 2013

Contributor

Hmm.. should the exampleserver work like this: http://localhost:8000/hello-etherpad.html? It does not sync in FF or Chrome, nor does the bold-button work.

Error shown in the console in Chrome:

Uncaught TypeError: Cannot call method 'n' of null bcsocket.js:77
t.Ob bcsocket.js:77
(anonymous function)

Edit: the example on the mainpage works just fine.

Contributor

luto commented Feb 26, 2013

Hmm.. should the exampleserver work like this: http://localhost:8000/hello-etherpad.html? It does not sync in FF or Chrome, nor does the bold-button work.

Error shown in the console in Chrome:

Uncaught TypeError: Cannot call method 'n' of null bcsocket.js:77
t.Ob bcsocket.js:77
(anonymous function)

Edit: the example on the mainpage works just fine.

@jucovschi

This comment has been minimized.

Show comment Hide comment
@jucovschi

jucovschi Feb 26, 2013

Contributor

hmm, yes, it should work like this: http://localhost:8000/hello-etherpad.html. The error seems to come from bcsocket though and I didn't change anything there.

Do you see "Hello" being bold? Are there any errors on the server side? Did you do a "cake build", "cake webclient"?

Contributor

jucovschi commented Feb 26, 2013

hmm, yes, it should work like this: http://localhost:8000/hello-etherpad.html. The error seems to come from bcsocket though and I didn't change anything there.

Do you see "Hello" being bold? Are there any errors on the server side? Did you do a "cake build", "cake webclient"?

@luto

This comment has been minimized.

Show comment Hide comment
@luto

luto Feb 26, 2013

Contributor

Yes. cake webclient did the job, figured that out in the exact moment your reply arrived.

It works great now! But there are some problems with shift/not-shift in Chrome and FF.
Type asd\n, type press shift and then type asd\n and then again asd without shift..

Contributor

luto commented Feb 26, 2013

Yes. cake webclient did the job, figured that out in the exact moment your reply arrived.

It works great now! But there are some problems with shift/not-shift in Chrome and FF.
Type asd\n, type press shift and then type asd\n and then again asd without shift..

@luto

This comment has been minimized.

Show comment Hide comment
@luto

luto Feb 26, 2013

Contributor

Uhm.. there seem to be some serious syncing problems.. typing this:

hellohellohello
asdasdasdasd

Results in two lines of hellohellohello first, but then it switches to asdasdasdasd..
Tried it in FF and Chrome.

Contributor

luto commented Feb 26, 2013

Uhm.. there seem to be some serious syncing problems.. typing this:

hellohellohello
asdasdasdasd

Results in two lines of hellohellohello first, but then it switches to asdasdasdasd..
Tried it in FF and Chrome.

@jucovschi

This comment has been minimized.

Show comment Hide comment
@jucovschi

jucovschi Feb 26, 2013

Contributor

it's not a ShareJS syncing problem. It's a problem with the integration with ACE. More specifically in src/client/ace.coffee:87, I rewrite the ACE tokenizer so that ACE produces "BOLD" at the right place. The function gives two parameters: (line, state) which gives me the current line the tokenizer should process and the current state of the tokenizer at that line. The problem is that ACE supposes that the tokenizer can just restart at any point without much penalty. So if I had a block comment in the first line commenting the whole file, it would just be ignored - but that doesn't happen too often so for optimization reasons it first gives the tokenizer a "blank state" and hopes it will do the right job. After a while, it recomputes the right states and that's when you really see what you typed...

I'm preparing some solutions for that now...

Contributor

jucovschi commented Feb 26, 2013

it's not a ShareJS syncing problem. It's a problem with the integration with ACE. More specifically in src/client/ace.coffee:87, I rewrite the ACE tokenizer so that ACE produces "BOLD" at the right place. The function gives two parameters: (line, state) which gives me the current line the tokenizer should process and the current state of the tokenizer at that line. The problem is that ACE supposes that the tokenizer can just restart at any point without much penalty. So if I had a block comment in the first line commenting the whole file, it would just be ignored - but that doesn't happen too often so for optimization reasons it first gives the tokenizer a "blank state" and hopes it will do the right job. After a while, it recomputes the right states and that's when you really see what you typed...

I'm preparing some solutions for that now...

@jucovschi

This comment has been minimized.

Show comment Hide comment
@jucovschi

jucovschi Feb 26, 2013

Contributor

I think that fixes it.

Contributor

jucovschi commented Feb 26, 2013

I think that fixes it.

@luto

This comment has been minimized.

Show comment Hide comment
@luto

luto Feb 26, 2013

Contributor

Nope, write foobar right under Line 2, still says Line 2 for a few seconds.

Edit: argh... wait please.

[ luto luto-portable ~/sharejs-etherpad ] cake build
Compiling Coffee from src to lib

/Users/luto/sharejs-etherpad/Cakefile:31
        throw err;
              ^
Error: Command failed: Error: In src/client/ace.coffee, Parse error on line 97: Unexpected 'POST_IF'
    at Object.parseError (/Users/luto/sharejs-etherpad/node_modules/coffee-script/lib/coffee-script/parser.js:481:11)
    at Object.parse (/Users/luto/sharejs-etherpad/node_modules/coffee-script/lib/coffee-script/parser.js:533:22)
    at Object.exports.compile.compile (/Users/luto/sharejs-etherpad/node_modules/coffee-script/lib/coffee-script/coffee-script.js:46:20)
    at compileScript (/Users/luto/sharejs-etherpad/node_modules/coffee-script/lib/coffee-script/command.js:182:33)
    at fs.stat.notSources.(anonymous function) (/Users/luto/sharejs-etherpad/node_modules/coffee-script/lib/coffee-script/command.js:152:18)
    at fs.readFile (fs.js:176:14)
    at Object.oncomplete (fs.js:297:15)
Contributor

luto commented Feb 26, 2013

Nope, write foobar right under Line 2, still says Line 2 for a few seconds.

Edit: argh... wait please.

[ luto luto-portable ~/sharejs-etherpad ] cake build
Compiling Coffee from src to lib

/Users/luto/sharejs-etherpad/Cakefile:31
        throw err;
              ^
Error: Command failed: Error: In src/client/ace.coffee, Parse error on line 97: Unexpected 'POST_IF'
    at Object.parseError (/Users/luto/sharejs-etherpad/node_modules/coffee-script/lib/coffee-script/parser.js:481:11)
    at Object.parse (/Users/luto/sharejs-etherpad/node_modules/coffee-script/lib/coffee-script/parser.js:533:22)
    at Object.exports.compile.compile (/Users/luto/sharejs-etherpad/node_modules/coffee-script/lib/coffee-script/coffee-script.js:46:20)
    at compileScript (/Users/luto/sharejs-etherpad/node_modules/coffee-script/lib/coffee-script/command.js:182:33)
    at fs.stat.notSources.(anonymous function) (/Users/luto/sharejs-etherpad/node_modules/coffee-script/lib/coffee-script/command.js:152:18)
    at fs.readFile (fs.js:176:14)
    at Object.oncomplete (fs.js:297:15)
@luto

This comment has been minimized.

Show comment Hide comment
@luto

luto Feb 27, 2013

Contributor

Works fine now!

Contributor

luto commented Feb 27, 2013

Works fine now!

@wmertens

This comment has been minimized.

Show comment Hide comment
@wmertens

wmertens Feb 27, 2013

Contributor

Looks great but for some reason the tests now fail. Specifically:

$ nodeunit tests.coffee
Evaling share
Etherpad library not found. Make sure to include Attributepool.js and Changeset.js in your javascript sourcecode
Etherpad library not found. Make sure to include Attributepool.js and Changeset.js in your javascript sourcecode
Evaling json

[…]
✔ types/text-api - etherpad - randomize generating functions
✖ types/text-api - etherpad - randomize emit

AssertionError: 'a' === { length: 1 }
at Object.genTests.randomize emit._i (/Users/wmertens/Documents/Projects/ShareJS/test/types/text-api.coffee:89:18)
at Object.MicroEvent.emit (/Users/wmertens/Documents/Projects/ShareJS/src/client/microevent.coffee:61:14)
at Object.etherpad.api._register (/Users/wmertens/Documents/Projects/ShareJS/src/types/etherpad-api.coffee:201:20)
at Object.MicroEvent.emit (/Users/wmertens/Documents/Projects/ShareJS/src/client/microevent.coffee:61:14)
at Object.genTests.randomize emit (/Users/wmertens/Documents/Projects/ShareJS/test/types/text-api.coffee:94:15)
at Object.wrapTest (/usr/local/lib/node_modules/nodeunit/lib/core.js:235:16)
at Object.wrapTest (/usr/local/lib/node_modules/nodeunit/lib/core.js:235:16)
at Object.wrapTest (/usr/local/lib/node_modules/nodeunit/lib/core.js:235:16)
at wrapTest (/usr/local/lib/node_modules/nodeunit/lib/core.js:235:16)
at Object.exports.runTest (/usr/local/lib/node_modules/nodeunit/lib/core.js:69:9)

Getting there though, I'm looking forward to it!

On Feb 27, 2013, at 10:20 , mluto notifications@github.com wrote:

Works fine now!


Reply to this email directly or view it on GitHub.

Contributor

wmertens commented Feb 27, 2013

Looks great but for some reason the tests now fail. Specifically:

$ nodeunit tests.coffee
Evaling share
Etherpad library not found. Make sure to include Attributepool.js and Changeset.js in your javascript sourcecode
Etherpad library not found. Make sure to include Attributepool.js and Changeset.js in your javascript sourcecode
Evaling json

[…]
✔ types/text-api - etherpad - randomize generating functions
✖ types/text-api - etherpad - randomize emit

AssertionError: 'a' === { length: 1 }
at Object.genTests.randomize emit._i (/Users/wmertens/Documents/Projects/ShareJS/test/types/text-api.coffee:89:18)
at Object.MicroEvent.emit (/Users/wmertens/Documents/Projects/ShareJS/src/client/microevent.coffee:61:14)
at Object.etherpad.api._register (/Users/wmertens/Documents/Projects/ShareJS/src/types/etherpad-api.coffee:201:20)
at Object.MicroEvent.emit (/Users/wmertens/Documents/Projects/ShareJS/src/client/microevent.coffee:61:14)
at Object.genTests.randomize emit (/Users/wmertens/Documents/Projects/ShareJS/test/types/text-api.coffee:94:15)
at Object.wrapTest (/usr/local/lib/node_modules/nodeunit/lib/core.js:235:16)
at Object.wrapTest (/usr/local/lib/node_modules/nodeunit/lib/core.js:235:16)
at Object.wrapTest (/usr/local/lib/node_modules/nodeunit/lib/core.js:235:16)
at wrapTest (/usr/local/lib/node_modules/nodeunit/lib/core.js:235:16)
at Object.exports.runTest (/usr/local/lib/node_modules/nodeunit/lib/core.js:69:9)

Getting there though, I'm looking forward to it!

On Feb 27, 2013, at 10:20 , mluto notifications@github.com wrote:

Works fine now!


Reply to this email directly or view it on GitHub.

@jucovschi

This comment has been minimized.

Show comment Hide comment
@jucovschi

jucovschi Feb 27, 2013

Contributor

solved the text-api issue. For some reason looks like etherpad-api thinks it's in a browser and that's why it prints:
Etherpad library not found. Make sure to include Attributepool.js and Changeset.js in your javascript sourcecode

is this a problem?

Contributor

jucovschi commented Feb 27, 2013

solved the text-api issue. For some reason looks like etherpad-api thinks it's in a browser and that's why it prints:
Etherpad library not found. Make sure to include Attributepool.js and Changeset.js in your javascript sourcecode

is this a problem?

@wmertens

This comment has been minimized.

Show comment Hide comment
@wmertens

wmertens Feb 27, 2013

Contributor

Well the way it is now it will give that error even when you're not using the etherpad functionality... I would prefer the message to be moved to when you use it.

E.g. throw an error from etherpad.create()

Contributor

wmertens commented Feb 27, 2013

Well the way it is now it will give that error even when you're not using the etherpad functionality... I would prefer the message to be moved to when you use it.

E.g. throw an error from etherpad.create()

@jucovschi

This comment has been minimized.

Show comment Hide comment
@jucovschi

jucovschi Feb 27, 2013

Contributor

done.

Contributor

jucovschi commented Feb 27, 2013

done.

@wmertens

This comment has been minimized.

Show comment Hide comment
@wmertens

wmertens Feb 27, 2013

Contributor

Sweet! BTW, just as an FYI you can write things like

if window.ShareJS? && window.ShareJS.Changeset?
  Changeset = window.ShareJS.Changeset
  AttributePool = window.ShareJS.AttributePool

as

{Changeset, AttributePool} = window.ShareJS if window.ShareJS?.ChangeSet?

Isn't CoffeeScript cool? 😁

Contributor

wmertens commented Feb 27, 2013

Sweet! BTW, just as an FYI you can write things like

if window.ShareJS? && window.ShareJS.Changeset?
  Changeset = window.ShareJS.Changeset
  AttributePool = window.ShareJS.AttributePool

as

{Changeset, AttributePool} = window.ShareJS if window.ShareJS?.ChangeSet?

Isn't CoffeeScript cool? 😁

@wmertens wmertens merged commit 5437c45 into josephg:master Feb 27, 2013

@luto

This comment has been minimized.

Show comment Hide comment
@luto

luto Feb 28, 2013

Contributor

The cursor-position is not right in bold-text:
cursor-pos

Contributor

luto commented Feb 28, 2013

The cursor-position is not right in bold-text:
cursor-pos

@CrypticSwarm

This comment has been minimized.

Show comment Hide comment
@CrypticSwarm

CrypticSwarm Feb 28, 2013

Contributor

@mluto If you are using Ace it is probably because it isn't using a mono-spaced font. Ace only gets cursor positions correct on monospaced fonts.

Contributor

CrypticSwarm commented Feb 28, 2013

@mluto If you are using Ace it is probably because it isn't using a mono-spaced font. Ace only gets cursor positions correct on monospaced fonts.

@wmertens

This comment has been minimized.

Show comment Hide comment
@wmertens

wmertens Mar 1, 2013

Contributor

It's because of the bold fontface, looks like it's not the same width as the regular. Perhaps not using Monaco will help.

Contributor

wmertens commented Mar 1, 2013

It's because of the bold fontface, looks like it's not the same width as the regular. Perhaps not using Monaco will help.

@wmertens

This comment has been minimized.

Show comment Hide comment
@wmertens

wmertens Mar 1, 2013

Contributor

Yup it's Monaco. That's hardcoded into ace.

Contributor

wmertens commented Mar 1, 2013

Yup it's Monaco. That's hardcoded into ace.

@josephg

This comment has been minimized.

Show comment Hide comment
@josephg

josephg Apr 14, 2013

Owner

Sorry guys, this is a bad pull request, and in retrospect we shouldn't have accepted it yet.

  • It adds crap into the main share.js file. Why? You have to include extra JS files to make etherpad work anyway. Why make everyone pay that toll?
  • This patch breaks the separation of src/types being independant of the rest of the code.

Etherpad support has been removed from josephg/ot-types and I will remove it from the main share.js build until it is fixed.

Owner

josephg commented Apr 14, 2013

Sorry guys, this is a bad pull request, and in retrospect we shouldn't have accepted it yet.

  • It adds crap into the main share.js file. Why? You have to include extra JS files to make etherpad work anyway. Why make everyone pay that toll?
  • This patch breaks the separation of src/types being independant of the rest of the code.

Etherpad support has been removed from josephg/ot-types and I will remove it from the main share.js build until it is fixed.

+ Changeset = window.ShareJS.Changeset
+ AttributePool = window.ShareJS.AttributePool
+else
+ Changeset = require("./../lib-etherpad/Changeset");

This comment has been minimized.

Show comment Hide comment
@josephg

josephg Apr 14, 2013

Owner

Not ok.

@josephg

josephg Apr 14, 2013

Owner

Not ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment