Adding Etherpad/Etherpad-lite document type to ShareJS #112

Closed
wants to merge 6 commits into
from

Projects

None yet

4 participants

@jucovschi

Adding Etherpad-lite document type which allows text+attribute OT. The added example does not yet make use of attributes though...

@wmertens

There's a lof of JS-isms in this code. For example, this could be written instead as:

  @connection.send
    doc: @name
    meta: state

Could you fix these to adhere to the style of the project?

@wmertens

Likewise:

etherpad.serialize = (snapshot) ->
  text: snapshot.text
  attribs: snapshot.attribs
  pool: snapshot.pool.toJsonable()
@wmertens

Not sure about this. Obviously it's a lot of work to convert this to coffeescript. Maybe this should be in a lib dir somewhere? It's not really code you'll change right?

No I will not change any of these libraries and so I'd rather put them in some lib directory. In the end there should be a nice way of using legacy JS code. BTW: I really do not like how I solved the problem of getting the Changeset library available in the browser (the window.ShareJS.Changeset). Are you ok with it?

@josephg your call :-)

Owner

... I don't mind, so long as someone's happy to maintain this code. My biggest concern is the lack of tests for any of this code.

Ok so how about putting the etherpad js under src/lib-etherpad and having just the coffeescript in src/types?

@wmertens
wmertens commented Aug 1, 2012

@josephg how do you feel about the javascript ported from the etherpad project? Should it be translated to coffee? Should it be moved elsewhere?

@wmertens wmertens referenced this pull request Aug 7, 2012
Open

Rich text support #1

@josephg josephg commented on an outdated diff Aug 14, 2012
src/types/etherpad-api.coffee
@@ -0,0 +1,117 @@
+# Text document API for text
+# :tabSize=4:indentSize=4:
@josephg
josephg Aug 14, 2012 owner

ShareJS uses 2 spaces, not tabs.

@josephg josephg commented on an outdated diff Aug 14, 2012
src/types/etherpad.coffee
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS-IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+var _opt = null;
+
+/**
+ * ==================== General Util Functions =======================
+ */
+
+Changeset = {};
@josephg
josephg Aug 14, 2012 owner
var Changeset = {};
@josephg
Owner
josephg commented Aug 14, 2012

Great work on getting this working, @jucovschi.

Nitpicks aside:

  • It doesn't have any tests. Even just hooking up the random op generator would make me feel a lot better about this. Take a look at test/types/text.coffee.
  • This is 2.5k lines, which increases the number of lines of code in sharejs by about 50%. The JSON OT code (which is probably the most complicated thing in sharejs) is only 441 lines. The web client (compiled to unminified JS) fits in about 1000 lines.

My instinct is that it shouldn't live in sharejs, but I have no idea where it should live. I don't want to maintain 2.5k lines of someone else's javascript. But also, I want rich text. @wmertens @nornagon - thoughts? What do?

@wmertens

I think it's not too bad to have this added code because it's separated from the other types and the added code is really only touched by the etherpad type.

Perhaps it would be good to have a mechanism in place to only require types that the server is going to allow. That way, the server only pays the large JS codebase price when it's being used. The client has to include it separately anyway so that's fine. As an added bonus, it will make the memory footprint of text- or json-only server a little smaller.

@nornagon

I don't think the problem is pure runtime weight of JS (especially not on the server side), it's maintenance cost.

That said, I haven't touched the JSON OT code since I wrote it, which hasn't really turned out to be a problem.

@wmertens

Ok so it comes down to code support. How about we put in the docs that this code uses rather a lot of third-party javascript and it may break in the future?
The coffee code still contains a lot of JS-isms though - is that a dealbreaker?

We could pull it, mark it experimental and ship 0.6?

@jucovschi

Sorry for not being very active lately on this issue. So I've added the tests from the old etherpad repo. So that's out of the way. I also cleaned up the code so that it does not have JS-isms. Please recheck.

About code support: I don't think anyone wants to change Changeset and AttributePool libraries. So I guess maintenance cost is minimal. Now it also has test cases...

@wmertens wmertens commented on an outdated diff Sep 7, 2012
buildtype 'text-tp2'
# TODO: This should also be closure compiled.
extrafiles = expandNames extras
e "coffee --compile --output webclient/ #{extrafiles}", ->
- # For backwards compatibility. (The ace.js file used to be called share-ace.js)
- e "cp webclient/ace.js webclient/share-ace.js"
+ # For backwards compatibility. (The ace.js file used to be called share-ace.js)
@wmertens
wmertens Sep 7, 2012

Why do you undent here? You can only copy ace when it's compiled...

@jucovschi

sorry :) fixed.

@wmertens wmertens commented on the diff Sep 7, 2012
src/types/etherpad-api.coffee
@@ -0,0 +1,93 @@
+# Text document API for text
+# :tabSize=2:indentSize=2:
+
+if WEB?
+ if window.ShareJS? && window.ShareJS.Changeset?
+ Changeset = window.ShareJS.Changeset
+ AttributePool = window.ShareJS.AttributePool
+ else
+ console.log("Etherpad library not found. Make sure to include Attributepool.js and Changeset.js in your javascript sourcecode");
+else
+ etherpad = require './../lib-etherpad/etherpad'
@wmertens
wmertens Sep 7, 2012

Use '../lib-etherpad/etherpad'. Also, that doesn't exist :-). The path to Changeset below is also wrong.

@wmertens wmertens commented on the diff Sep 7, 2012
src/types/etherpad-api.coffee
+ AttributePool = require './../lib-etherpad/AttributePool'
+ Changeset = require './Changeset'
+
+etherpad.api =
+ provides: { text:true }
+
+ # The number of characters in the string
+ getLength: -> @snapshot.text.length
+
+ # Get the text contents of a document
+ getText: -> @snapshot.text
+
+ # Get metadata starting from offset startOffset and having length length
+ getMeta: (startOffset, length) ->
+ if typeof @snapshot.pool.getAttrib == "undefined"
+ @snapshot = etherpad.tryDeserializeSnapshot(@snapshot);
@wmertens
wmertens Sep 7, 2012

@snapshot = etherpad.tryDeserializeSnapshot @snapshot

@wmertens wmertens commented on the diff Sep 7, 2012
src/types/etherpad-api.coffee
+ # The number of characters in the string
+ getLength: -> @snapshot.text.length
+
+ # Get the text contents of a document
+ getText: -> @snapshot.text
+
+ # Get metadata starting from offset startOffset and having length length
+ getMeta: (startOffset, length) ->
+ if typeof @snapshot.pool.getAttrib == "undefined"
+ @snapshot = etherpad.tryDeserializeSnapshot(@snapshot);
+ snapshot = @snapshot;
+ iter = Changeset.opIterator(snapshot.attribs)
+ offset = 0;
+ result = [];
+ rangeStart = Changeset.numToString(@snapshot.pool.putAttrib(["range.start",1], true));
+ rangeEnd = Changeset.numToString(@snapshot.pool.putAttrib(["range.end",1], true));
@wmertens
wmertens Sep 7, 2012

rangeEnd = Changeset.numToString @snapshot.pool.putAttrib ["range.end",1], true

@wmertens wmertens commented on the diff Sep 7, 2012
src/types/etherpad.coffee
+# }
+
+# The Changesets have the structure
+# {
+# "changeset" - serialized version of the changeset
+# "pool" - the pool
+# }
+
+if WEB?
+ if window.ShareJS? && window.ShareJS.Changeset?
+ Changeset = window.ShareJS.Changeset
+ AttributePool = window.ShareJS.AttributePool
+ else
+ console.log("Etherpad library not found. Make sure to include Attributepool.js and Changeset.js in your javascript sourcecode");
+else
+ Changeset = require("./../lib-etherpad/Changeset");
@wmertens
wmertens Sep 7, 2012

Changeset = require "../lib-etherpad/Changeset"

@wmertens
wmertens commented Sep 7, 2012

Starting to look good!

@nornagon
nornagon commented Sep 8, 2012

Do we have a fuzzer for it yet?

@wmertens

Errm, what is a fuzzer?

@wmertens

Hi @jucovschi, there have been some changes since your pull request (e.g. ace), would you mind freshening up the code? Also, could you address the comments I made above?

Thanks!

@jucovschi

Created a remake (#172). Closing this pull.

@jucovschi jucovschi closed this Feb 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment