Added CodeMirror support

[Cortexelus]

See the demo http://codemirror-sharejs.meteor.com/ Cortexelus:|

[mizzao]

This was already done a similar way in #14; however I was reluctant to include it this way because I don't think every app should have to include both the Ace and CodeMirror JS code just to use one of them.

I was going to implement some kind of dynamic loading solution for the clients.

[mizzao]

Maybe this is okay for now, and we can improve the implementation later. How big is the CodeMirror JS?

[Cortexelus]

Using chrome's developer tools, observe the network burden on the client: http://documents.meteor.com [ace] ~250KB http://codemirror-sharejs.meteor.com/ [ace+cm] ~300KB

[Cortexelus]

codemirror.js unminified is ~308 KB with negligibly sized (1-3 KB) optional addons

[mizzao]

Okay, I think it would be reasonable to put this in for now and optimize later. Just give me some time to review the PR. Hopefully I can fix the package for 0.9.1 as well.

[Cortexelus]

Cool. I heard you met Slava Kim yesterday. How was that? You're at Harvard? I'm going to meet him later this week. Jacob Cole from MIT is a mutual friend.

[mizzao]

Oh, Slava and I have met before. How did you hear of that, and where are you usually located?

[Cortexelus]

I heard from Jacob, who is a mutual friend of Slavas! I think we are checking out the Quora event this evening.

[Cortexelus]

i'm at MIT at the moment, working on a media lab research project

[mizzao]

Cool. I am currently taking HTMAA (MAS 863). :-)

[mizzao]

You guys should come to the Meteor meetup! Slava is here and you can talk about Meteor stuff.

[mizzao]

Hey @Cortexelus, what do you think of splitting off the Ace and CodeMirror components into mizzao:sharejs-ace and mizzao:sharejs-codemirror packages so that they can be installed separately? That way we don't have situations like where you made a branch just to get rid of one of them.

[Cortexelus]

I think this is a good idea. In what real situation would one need to load both editors in a single meteor application anyway? I started a branch that took Ace out because it sends ~5x more bandwidth to the user than just CodeMirror.

However, this makes fixes to mizzao:sharejs twice as much work, doesn't it? Is there not a way to set a config variable in packages.js which will only deliver one lib to the user? (your original idea)

On Thu, Oct 2, 2014 at 3:59 PM, Andrew Mao notifications@github.com wrote:

Hey @Cortexelus https://github.com/Cortexelus, what do you think of splitting off the Ace and CodeMirror components into mizzao:sharejs-ace and mizzao:sharejs-codemirror packages so that they can be installed separately? That way we don't have situations like where you made a branch just to get rid of one of them.

— Reply to this email directly or view it on GitHub https://github.com/mizzao/meteor-sharejs/pull/27#issuecomment-57695881.

[mizzao]

What do you mean it sends 5x more bandwidth - do you mean 5x more code?

No, I don't think it would make fixes harder. Both the sharejs-ace and sharejs-cm packages would depend on the parent sharejs package for the stack. The code is all modularized out already as you saw when you submitted the PR.

I've asked for package config but it's been largely ignored: meteor/meteor#1292. Perhaps you could help poke a little.

See also https://groups.google.com/forum/#!msg/meteor-core/MNzKXHlCGE0/eAqXKKKfDxcJ.

[Cortexelus]

I mean after meteor minifies all the code, it initially delivers the client ~250kb with only ace, and ~50kb with only cm (tested in chrome on remote server).

Ah, package dependencies! Sounds like a good plan! Go for it! CJ

On Thu, Oct 2, 2014 at 5:56 PM, Andrew Mao notifications@github.com wrote:

What do you mean it sends 5x more bandwidth - do you mean 5x more code?

No, I don't think it would make fixes harder. Both the sharejs-ace and sharejs-cm packages would depend on the parent sharejs package for the stack. The code is all modularized out already as you saw when you submitted the PR.

I've asked for package config but it's been largely ignored: meteor/meteor#1292 https://github.com/meteor/meteor/issues/1292.

— Reply to this email directly or view it on GitHub https://github.com/mizzao/meteor-sharejs/pull/27#issuecomment-57716367.

[mizzao]

Hey @Cortexelus, I just merged this. Some issues though:

• Why create CodeMirror from a text area? It seems to replace it with a <div> anyway, just like Ace does.
• If you switch between documents with CodeMirror selected in the demo, the extra divs stay on the screen when switching to other modes. This really messes up the display. Something isn't cleaning up correctly - maybe you can take a look?

Also, note how the app has been split into different packages for each editor. So there is less overhead of pulling in the editor of your choice. Once we get this fixed, you'll be able to use meteor add sharejs-cm instead of the custom fork that you're using now.

[robert-boulanger]

hey mizzao & Cortexelus,

I'm not quiet sure if this is the correct solution and I'm also no coffee script guy so please don't ask me to make a pull request ;), but changing the bottom of mizzao:sharejs-cm/client.coffee to

jscm=null
UI.registerHelper "sharejsCM",  Template('sharejsCM', ->
    if jscm
      jscm.disconnect()
      jscm.destroy()

    jscm = new ShareJSCMConnector(this)
    return jscm.create()
)

did the trick for me. (But I have to mention, that I'm just interested in CodeMirror, But I think it should work for the ACE client file as well.) No more multiple editors when selecting or creating a new document.

Hope this helps

-Robert

[mizzao]

Hey @robert-boulanger, there is no issue with Ace right now, only CodeMirror. I need to look a little more carefully into what's going on, but I wonder if we can't just use CodeMirror on a div directly rather than converting it from a textarea.

Also, I think you should consider writing your patches as commits rather than in messages, or e-mails (as you did before). I really think you'll find it easier! Open-source collaboration via e-mail is 1990's style :P

[robert-boulanger]

I need to look a little more carefully into what's going on, but I wonder if we can't just use CodeMirror on a div directly rather than converting it from a textarea.

I do a lot with code mirror and never used it with a textarea. I don’t see any reason why it should not work on a div. Also, I think you should consider writing your patches as commits rather than in messages, or e-mails (as you did before). I really think you'll find it easier! Open-source collaboration via e-mail is 1990's style :P

I know, you told me already a few months ago. But this time I really was not sure if this is a correct solution. So please understand it as possible hint rather then a real contribution ;)

-Robert

[mizzao]

It seems that you tested it out already. It's quick to just deploy a Meteor app for example, for someone else to take a look at.

[robert-boulanger]

Just tried that, but I get a strange error:

Errors prevented deploying:
While Building the application:               
error: mizzao:sharejs is not compatible with architecture ‚os.linux.x86_64'

I’m here under OS X

Any ideas ?

Am 29.10.2014 um 17:34 schrieb Andrew Mao notifications@github.com:

It seems that you tested it out already. It's quick to just deploy a Meteor app for example, for someone else to take a look at.

— Reply to this email directly or view it on GitHub https://github.com/mizzao/meteor-sharejs/pull/27#issuecomment-60957439.

[mizzao]

Can you check the repo out into your local machine, rather than trying to use the package from Meteor's package server?

[robert-boulanger]

This is what I did. meteor deploy starts building mizzao:sharejs and thats what the server does not like. As soon as I try to delete it it will be rebuilt. Also tried https://github.com/meteorhacks/npm/issues/40 https://github.com/meteorhacks/npm/issues/40 and this https://github.com/meteor/meteor/issues/2473#issuecomment-53834357 https://github.com/meteor/meteor/issues/2473#issuecomment-53834357 but I can’t publish maize:shares since I’m not the maintainer The only idea i have to connect to a linux server set the project up there and try to re-deploy from there.

Am 29.10.2014 um 18:39 schrieb Andrew Mao notifications@github.com:

Can you check the repo out into your local machine, rather than trying to use the package from Meteor's package server?

— Reply to this email directly or view it on GitHub https://github.com/mizzao/meteor-sharejs/pull/27#issuecomment-60969451.

[mizzao]

That wasn't the problem, take a look at https://github.com/mizzao/meteor-sharejs/blob/6362e9d65cac6248073c85849047cf58325ba0b4/sharejs-cm/client.coffee.

Anyway, I have mizzao:sharejs-codemirror published now, want to give it a try? You should be able to take out whatever ad hoc code you have to run this.

[robert-boulanger]

Looks very good at the first moment, anyway there sone issue here I can’t see at documents.meteor.com http://documents.meteor.com/. In my application the „Loading…" string does not disappear after the document is loaded. See attachment. And I now get an error thrown in the console with one of Codemirrors add ons. But the add on is working properly. I have to investigate there more time into this.

[mizzao]

We should be able to get rid of the Loading... string if it's causing issues.

The add-ons are also loaded in a pretty elementary way, based on what @Cortexelus initially included: https://github.com/mizzao/meteor-sharejs/blob/master/sharejs-codemirror/package.js. We may want to do this in a more advanced way like we did with Ace.

Make sure you don't have both your old libraries and the new package loaded, or that might make it hard to figure out the cause of these issues.

[robert-boulanger]

We should be able to get rid of the Loading... string if it's causing issues.

In my opinion this Loading … should go away in any case:

1.) It’s a hardcoded message in english language, no chance for translation or replacing with something else 2.) Some (me included) may prefer to show an own graphical spinner or whatever independently from this library

The add-ons are also loaded in a pretty elementary way, based on what @Cortexelus https://github.com/Cortexelus initially included: https://github.com/mizzao/meteor-sharejs/blob/master/sharejs-codemirror/package.js https://github.com/mizzao/meteor-sharejs/blob/master/sharejs-codemirror/package.js. We may want to do this in a more advanced way like we did with Ace.

First I thought to suggest not to include any add on through the package at all. So for example I’m developing my own codemirror mode based on an existing one, and so far I’m happy with throwing my add-on’s, modes etc. as well as codemirrors standard add-ons, modes, etc. just in meteors client directory.

But maybe this could bring up a new problem: Let’s assume the mizzao:sharejs-codemirror package installs code mirror 5.x one day through "meteor update" and the add-on files manually installed by the user are from 4.x and might therefore be incompatible.

On the other hand I figured out another problem when porting my existing app now to this sharejs-codemirror during the last week: In the past I had my own css rules for several code mirror styles, they are now are always overwritten by the packed codemirror.css.

Maybe a solution for this would be an additional setting in the config section of code mirror called „naked" or so. If „naked“ is true, then the package provides only codemirror.js, nothing else, all css, plugins, add-ons, modes must be provided manually by the developer through the client directory and therefore the developer is responsible for compatibility. If naked is false, Cortexelus packaging is used.

Any thoughts on this ?

Make sure you don't have both your old libraries and the new package loaded, or that might make it hard to figure out the cause of these issues.

All the old stuff is gone. Installation is clean.=

[mizzao]

Hi @robert-boulanger,

Took me a while to find this message. You should create new issues for new problems.

I've nuked the Loading... text from Ace and CodeMirror in the latest commit. As for the CSS stuff, I agree that what @Cortexelus did is suboptimal. Let's discuss ideas to improve it in a separate issue. There is currently no support to load Meteor packages with arguments, although that is being discussed for future versions.

[robert-boulanger]

Did you update the package as well ? "Loading..." still appears after meteor add mizzao:sharejs-codemirror.

[mizzao]

No, not published yet. Thought we could do that after we clear up whatever other issues you are talking about.

[robert-boulanger]

My other main issue was:

On the other hand I figured out another problem when porting my existing app now to this sharejs-codemirror during the last week: In the past I had my own css rules for several code mirror styles, they are now are always overwritten by the packed codemirror.css.

But I figured out this was bad design at my side. When adding new modes or overlays to codemirror you should also add new css-styles for them which I have done in meanwhile. So, from my side this issue is closed.

[mizzao]

Okay. I think the improvement of packaging themes and scripts with CodeMirror could be improved in general, though. Right now it's just a hardcoded subset of what's available.