- WebODF - lists.nlnet.nl

On architecture/layers of WebODF
by Friedrich W. H. Kossebau 23 Jul '13

23 Jul '13

Hi, ((disclaimer: most of the following was done before Jos took back project- lead, so he might have different opinions on some things :) )) so here some notes from the last months regarding parts of the architecture of WebODF: one is a chart showing the MVC-alike structure, to separate the (extended) ODF model things as mapped into the DOM from those which deal with proper display in the rendered html page and those which deal with turning any input into operations on the (extended) ODF model. The other are about plans how to reorganize the classes fo the (extended) ODF model. The attached image is a quick sketch showing the MVC related classes/architecture of WebODF as drafted in November (things have slightly changed since then, but in general still valid). Please ignore UML-mistakes ;) As said before, the idea is to separate display (so showing things to the user) related stuff from pure document model stuff. So the latter can be used independently in some future, e.g. with another XML/DOM-based system, like a node.js server. There was work started on this, working title "xmlodf", but then the stream of patches from Down-Under :) and OT-work stalled things. Which also means the (extended) ODF model classes should have no clue about the display related classes. Instead the latter should connect to signals and other means of the first ones to get any data (updates) they need. The planned structuring/layering of the WebODF module was already partially integrated in master, so the classes of WebODF could be grouped into ... ... model oriented classes/layers G1: loading/saving of pure odf doc G2: changing of pure odf doc G3: cursor extended odf doc G4: changing of cursor extended odf doc ... and display oriented classes/layers G5: displaying of pure odf doc G6: displaying of cursor extended odf doc where dependencies are a DAG and basically Gn+1 depends on Gn...G1, except that G5 only needs G2+G1 ("G" as in "Group"). These days G3 and G4 also includes the EditInfo, while G6 got the EditInfoHandle/EditInfoMarker. The current namespaces (core, gui, odf, ops, xmldom) are not really ideal IMHO. So the proposal from that work branch was like this for the (extended) ODF model classes: xmlodf.utils : (all utility classes) Base64, ByteArray, ByteArrayWriter, RawInflate, Zip, xmldom.LSSerializer, xmldom.LSSerializerFilter EventNotifier, LoopWatchDog xmldom.XPath PositionIterator, PositionFilter, Selection, Cursor, SelectionManager, SelectionMover (create own (sub-)submodule for them?) xmlodf.core : (the project core type classes) StyleInfo (detail of OdfContainer, Formatting) OdfContainer, Formatting OdtDocument, OdtCursor xmlodf.ops : (could possibly moved to xmlodf.core) Operation xmlodf.ops.redset : (our current operation set, "red" pointing to bloody beginners ;) ) OpInsertText, OpRemoveText, OpSplitParagraph, OpSetParagraphStyle, OpUpdateParagraphStyle, OpCloneParagraphStyle, OpDeleteParagraphStyle, OpInsertTable, OpAddCursor, OpRemoveCursor, OpMoveCursor Hopefully this sheeds some lights into things. I hope we soon have some break from the feature rush and can do some more documentation and (complete the) redesign, to make the architecture more obvious and also have everyone in the same boat. Possibly I should do a patch to properly order and group with comments at least all files in the manifest.js/CMakeLists.txt, to help remembering the layers. Cheers Friedrich -- Friedrich W. H. Kossebau // KO GmbH http://kogmbh.com/legal/

1 1

Reverted commit 0c811f2404e306fb7ea84cb282d9e56b958d399d.
by Philip Peitsch 21 Jul '13

21 Jul '13

Hi guys, Just noticed this was reverted. Was hoping someone might be able to provide information as to why? The commit comment lacked any detail unfortunately :( Cheers, -- Philip Peitsch Mob: 0439 810 260

2 1

Different behaviour on delete and backspace for emptied spans?
by Friedrich W. H. Kossebau 03 Jul '13

03 Jul '13

Hi, when testing if there will be any regressions on always normalizing* the position and length in OpRemoveText I found that there is different behaviour if in the following structure the char is removed by a backspace or a delete: <text:p><text:span>A</text:span></text:p> Currently these are the results: ... on a backspace from behind the "A" <text:p><c:cursor/><text:span></text:span></text:p> (ignore the cursor position, different topic, handled in separate email) ... on a delete from before the "A" <text:p><c:cursor/></text:p> I would have expected to get the same results in both cases. Playing with LO I found that there the span is kept in both cases (tested by creating an empty document, inserting an "A", marking it and directly formatting it to bold, removing the A in either ways again and saving as .fodt and inspecting with a text editor, while also seeing that with the empty document the Bold is toggled and any inserted chars are ending up in the span, being formatted with Bold). * normalizing by doing this inside the execute method and restoring original values afterwards: if (length < 0) { position = position + length; length = -length; } What do you think? Should emptied spans be deleted in this case? Or should empty spans only be deleted once a deletion range crosses their border? Cheers Friedrich -- Friedrich W. H. Kossebau // KO GmbH http://kogmbh.com/legal/

4 3

Walkable position inside empty span in empty paragraph, or before?
by Friedrich W. H. Kossebau 27 Jun '13

27 Jun '13

Hi, for the following sample I wonder where walkable position should be: <text:p><text:span></text:span></text:p> There is the operation test "Remove_backspaceOnlyCharacter" which has as <before> this structure: <text:p><text:span>A</text:span></text:p> After inserting a cursor behind A and backspacing the A it expects this: <text:p><c:cursor/><text:span></text:span></text:p> Now README_cursorpositions lists as allowed cursor position this sample: | Who is right? Playing with LO I found that there the cursor is placed inside the span (tested by creating an empty document, inserting an "A", marking it and directly formatting it to bold, removing the A again and saving as .fodt and inspecting with a text editor, while also seeing that with the empty document the Bold is toggled and any inserted chars are ending up in the span, being formatted with Bold). I tend to think that LO and README_cursorpositions.txt are the better solution. Otherwise the direct text-property formatting for would need to be disabled on an empty document, because there is no span which can get text formatting attributes for the current cursor position. What do you think? Cheers Friedrich -- Friedrich W. H. Kossebau // KO GmbH http://kogmbh.com/legal/

2 1

OpSetParagraphStyle and OT
by Friedrich W. H. Kossebau 26 Jun '13

26 Jun '13

Hi WebODFler, currently the spec of OpSetParagraphStyle has a parameter "position" which identifies the paragraph to set the style for by being somewhere inside that paragraph. Worked fine so far, but poses a big problem on doing OT versus all kind of other operations. (There is also anothe issue, in that there are the parameters "styleNameBefore" and "styleNameAfter", of which only "styleNameAfter" is used, and for the other so far no use is seen. So ideally "styleNameBefore" is removed and "styleNameAfter" is renamed to just "styleName") To refresh your memories, this is the background: Two clients A and B create operations based on the same state of the document, which they first apply locally and only then exchange with the other clients. The goal is to have the documents at all clients to converge against each other in the end, ideally being identical, after all operations have been applied at all clients. So A now gets the operations from B and needs to have them transformed against its own locally applied operations, while B gets the ones from A and needs the reverse, A's operations being transformed against its own locally applied operations. There are cases where tie-breaking is needed, usually when the same object gets modified by the two ops which have to be transformed each other, think of two OpUpdateParagraphStyle modifying the same style or two OpInsertText targetting at the same position. Here some arbitrary priority is needed, which needs to be based on a property which is the same everywhere (e.g. order of memberIds or which op is first on the server). Sometimes longer sequences of ops are transformed against each other. Which means that when dealing with the non-first ops these are based on virtual states of the documents and thus querying additional properties from the document is not possible. Applying the ops to some temp document for that is possibly way too expensive (unless we could do some light-weight shadow document, but the remove/inserttext ops need the full DOM), especially on multi-op transformation lots of documents would need to be created. (place of the cross-transformation of the ops does not matter for this) Now here are the samples where things fail ATM (always assuming A's ops have priority): Doc: <text:p>Text</text:p> A: [<setParagraphStyle pos="0" styleNameAfter="S1" memberId="A"/>] B: [<setParagraphStyle pos="3" styleNameAfter="S2" memberId="B"/>] Obviously hard to know if both target the same paragraph. Proposal: fix by identifying the paragraph with by its first position Using this proposal, there is another issue: Doc: <text:p>AB</text:p><text:p>C</text:p><text:p/> A: [<setParagraphStyle pos="3" styleNameAfter="S1" memberId="A"/>] B: [<removeText pos="0" length="1" memberId="B"/>] How can the setParagraphStyle op be transformed against the removeText op, in such a way that the pos attribute again points to the begin of the paragraph? There is no information in the removeText op which paragraphs are affected and how. So while OT between two OpSetParagraphStyle might be fixable by just using the first position in the paragraph as id, this will fail with transformation versus OpRemoveText, because its spec addresses the touched objects only indirectly and thus cannot provide the needed information to transform OpSetParagraphStyle against it. How to solve that? Naively I would now propose to change the spec of OpRemoveText, which has anyway evolved to something that no longer matches its name. It would need to change to an op which is as specific as needed in its spec so that other ops can transform against it. Which surely is a general rule, as I find out by that :) How a new spec might look like? No real idea yet, but it needs to tell which paragraphs and which parts in it are removed. So OpSetParagraphStyle can be properly transformed against it. So no real question here, just loud thinking. Comments welcome. Cheers Friedrich -- Friedrich W. H. Kossebau // KO GmbH http://kogmbh.com/legal/

2 3

Adding spec property "setOwnCursor" for Insert/Remove/Split ops
by Friedrich W. H. Kossebau 24 Jun '13

24 Jun '13

Hi WebODFler, I would like to add a new property to the specs of all insert/remove/split ops: setOwnCursor (or some better name). This property will tell if the cursor of the member which sends the operation should be put after the place/result of the op. Currently this is only done iff the cursor is at the position of the op. But with real OT, where local operations are instantly applied and only then sent to other clients who might have to transform them on conflicts, the transformed operations and positions of cursors can be out-of-sync, breaking the current assumption with UI editing that cursors will always automatically placed behind the places of modification. Example: <text:p><cursor memberid="A"><cursor memberid="B"></text:p> Both clients A and B type at the same time "a" and "b" and apply it locally, before sending to server/other client: At A: <text:p><cursor memberid="B">a<cursor memberid="A"></text:p> Unsynced ops: [<insertText pos="0" text="a" memberId="A"/>] At B: <text:p><cursor memberid="A">b<cursor memberid="B"></text:p> Unsynced ops: [<insertText pos="0" text="b" memberId="B"/>] At server: op stack: [] As this is a case for tiebreaking on conflicting operations, because both insert at the same position and it needs to be decided in which order the insertions will end up in the final document, we assume that * priority is given to ops which are first on the server * priority in this case means insertion will be placed before the other insertion A is quicker with syncing with server: At A: <text:p><cursor memberid="B">a<cursor memberid="A"></text:p> Unsynced ops: [] At server: op stack: [<insertText pos="0" text="a" memberId="A"/>] Now B tries to sync, but sees that there are conflicting ops, which it first has to resolve locally, by transforming its local unsynced op and the new on the server against each other, so the server op is based on the state of B and the unsynced op is based on the (virtual) state of the server: T(<insertText pos="1" text="b" memberId="B"/>, <insertText pos="0" text="a" memberId="A"/> ) -> (<insertText pos="1" text="b" memberId="B"/>, <insertText pos="0" text="a" memberId="A"/> ) given the tiebreaking rules above. After transformation B then applies the transformed op from the server and sends its own transformed unsynced op to the server At B: <text:p>a<cursor memberid="A">b<cursor memberid="B"></text:p> Unsynced ops: [] At server: op stack: [<insertText pos="0" text="a" memberId="A"/>, <insertText pos="1" text="b" memberId="B"/>] Now A syncs again with the server, fetching the new op. And here we are with the problem: At A the cursor of B is still at the position 0, while the operation says that B inserts at position 1. Still we want to have the cursor of B after the op to be at position 3 (not only for convergence of the 2 clients). I would like to solve that by allowing ops to optionally take the cursor from whereever it was and just move it behind the result of the operation, instead of always only moving it if it is at the position of the operation. This would also allow to explicitely do operations at the current cursor position without moving the cursor, which currently is not possible. Operation creators would specify the cursor movement by setting the new property "setOwnCursor" to "false" or "true". It would be added to all operations where the text structure is changed. Any blockers, counterproposals, comments, icecoffee recipes? Cheers Friedrich -- Friedrich W. H. Kossebau // KO GmbH http://kogmbh.com/legal/

3 9

Re: [WebODF] Adding spec property "setOwnCursor" for Insert/Remove/Split ops
by Jeroen van Veen 22 Jun '13

22 Jun '13

Hey Friedrich, Sorry no i just accidently replied only to your email address, instead of the mailinglist :-) I'm using jinfinote already in combination with codemirror(https://github.com/l3mon/l3mon/blob/master/public/browser/lib/mu… It works ok, but could have some improvements. For instance, codemirror uses line positioning while jinfinote expects a char position in comparison to the whole text. The diff-match-patch part should probably be replaced by something more codemirror'ish but it kinda works for now. Hope something like this is possible for webodf as well! Jinfinote also supports undo by the way, which is pretty neat. cheers, jeroen 2013/6/19 Friedrich W. H. Kossebau <friedrich(a)kogmbh.com>: > Hi Jeroen, > > (your email offlist on purpose?) > > Am Mittwoch, 19. Juni 2013, 11:40:30 schrieb Jeroen van Veen: >> Hi, >> >> You guys are probably already aware of these ot-resources, but i'll >> add them anyway just in case :-) > > Thanks, aware, but did not study these ones yet (there are tons of documents > out there on the tubes). And at one point trying beats studying :) > > But your email made me at least take a glance at the Infinote protocol, and I > could learn that they have two variants per document-modifying op, e.g. > <insert/> and <insert-caret/> or <delete/> and <delete-caret/>, where each *- > caret variants "additionally places the cursor of the user that made the > request". So at least one reference for the "setOwnCursor" property solution > :) > > Cheers > Friedrich > -- > Friedrich W. H. Kossebau // KO GmbH > http://kogmbh.com/legal/

1 0

Coordinates, confusion and Confucius
by Philip Peitsch 20 Jun '13

20 Jun '13

Hi all, I'm noticing I'm somewhat confused about which coordinate system belongs in which layers. I'm doing a quick investigation, and documenting the results here, to hopefully spark a discussion about which of these we actually need, which can be better hidden to prevent accidents etc. As near as I can tell, there are effectively two "classes" of positions available: 1. Low level positioning Directly available from PositionIterator. The most common one used is Simple Coordinates + filtered elements. This does NOT correspond to possible cursor positions as an extra layer of filtering is required for that. A position iterator is available from: gui.SelectionMover.createPositionIterator PositionIterator has the following offsets available PositionIterator.offset() - "Simple Coordinates" - 8 usages in prod code, scattered - Filters out empty text nodes. - Only ever instantiated from within SelectionMover, which configures the iterator to also ignore cursor & edit info nodes - Adjacent text nodes count as 1 position to the parent container e.g., " " <editinfo/> " B" "C"|, the cursor just after "C" would report an offset of 1 PositionIterator.domOffset() - "Filtered Coordinates" - 1 usage in entire codebase - Filters out empty text nodes. - Only ever instantiated from within SelectionMover, which configures the iterator to also ignore cursor & edit info nodes - Individual text nodes are counted e.g., " " <editinfo/> " B" "C"|, the cursor just after "C" would report an offset of 2 PositionIterator.unfilteredDomOffset() - "Unfiltered Coordinates" - 7 usages, all in SelectionMover - Filters out nothing. All nodes are counted - Individual text nodes are counted e.g., " " <editinfo/> " B" "C"|, the cursor just after "C" would report an offset of 4 PositionIterator.textOffset - "Aggregated Texts Coordinates" - 1 usage in the entire codebase IN A TEST - Returns the offset within a text node as if the text node was connected to all prior siblings e.g., " " <editinfo/> " B" "C"|, the cursor just after "C" would report an offset of 3 PositionIterator supplies the following two methods for setting it's position PositionIterator.setPosition - 7 usages in prod code, scattered - Takes a filtered container and filtered offset (note, this is Simple Coordinates) - This method CANNOT take Filtered Coordinates (will return the wrong node or run out of children) - This method CANNOT take Aggregated Text Coordinates (will likely throw an error that the offset is longer than the container) - Doesn't check that container is actually valid to pass the filter. If it isn't the iterator starts in a funny spot. PositionIterator.setUnfilteredPosition - 2 usages, scattered - Takes an unfiltered container and unfiltered offset (note, this is Unfiltered Coordinates) - Immediately jumps to the next filtered container & offset - This method cannot take any other coordinates, and will return the wrong position if provided them 2. Cursor-level positioning - "Cursor Steps" Manipulated using the SelectionMover. This indicates the number of cursor steps to move in a direction or count. These are *all* run through the TextPositionFilter which is required for basically every single call to the selection mover. Available from: OdtCursor.getStepCounter. Some things I've noticed: - Most operations work in "Cursor Steps" (OpAddCursor, OpMoveCursor, OpRemoveCursor) - The only way to move a cursor is using it's counting patterns generally, which always require the filter (which is the same for everything), and a root node often (again, usually the same for everything). - Cursor especially exposes a lot of unsafe methods for operating on (e.g., direct access to nodes etc., the returned range selection is in terms of unfiltered container & offsets "Unfiltered Coordinates") - SelectionMover.isPositionWalkable is only ever called once. - It is kind of difficult to go from an arbitrary node + offset to steps & back. E.g., each createOp* command starts with similar sets of calls asking for cursor positions and friends. Next, most op execution begins with "get my cursor, get my cursors position, get my position filter, now start counting" Some things that can be done: - Eliminate extraneous systems, and reduce to two primary coordinates (my vote is that the primary two are Steps & Unfiltered Coordinates). All others should be removed or hidden inside the class that requires them. - Fix iterators to work safely with both systems, and ideally provide a way for the iterator to verify that it's been given the right system, or automatically detect and adapt to the provided type - Figure out who owns a SelectionMover, a NodeFilter, Cursor etc.. Restructure these so that arguments are not continually passed around (e.g., odtDocument filter is passed to everyone, even though its static and central) - Specify the primary communication coordinates for various classes (e.g., "Steps", Unfiltered coordinates). Ideally each class should speak only one coordinate system (with perhaps a little bit of translation) If no-one has major concerns/issues/thoughts, I'll gradually refactor some parts of the stack carefully and submit them tiny bits at a time to try and keep the review workload small and the regression rate tiny. But, either way figured it was worth getting some shared understanding before I started about what the complexity I'm trying to address is, and what I think we should work towards :-) Cheers, Philip

2 1

Pending merge requests
by Philip Peitsch 12 Jun '13

12 Jun '13

Hi all, Just handing over some merge requests waiting for review and merging: https://gitorious.org/webodf/webodf/merge_requests #28 - Small (and rather arcane) but with some decent ramifications. I think there is actually a larger architectural discussion to have here sometime around which classes are *allowed* to take filtered vs. unfiltered DOM offsets. #29 - Make the table of contents behave as expected (i.e., clicking a link scrolls the view to the particular heading) #30 - This is a more controversial patch. Basically, discussion with Jos yesterday whilst hunting some cursor issues made me realise that the selection object a cursor has is not entirely necessary. This patch removes and replaces this with a straight range, under the theory that this makes usage of the cursor slightly more straightforward. I'm happy for feedback if there are some ways people can see to improve this. I'll (hopefully) be around in chat in about 2-3hrs if there is anything contentious in these MRs. Cheers, Philip

2 1

MRs waiting review
by Philip Peitsch 07 Jun '13

07 Jun '13

Hi all, Quick heads up to two new MRs waiting review: https://gitorious.org/webodf/webodf/merge_requests/22 - We have the requirement for being able to turn editing on and off at will. This properly unsubscribes and resubscribes to events when entering and exiting edit mode https://gitorious.org/webodf/webodf/merge_requests/23 - I missed some escape characters in the attribute values… apparently "may" === "must" :/ Thanks for all your work and continual prompt attention to these. I really appreciate you guys being so responsive with our merge requests :) Cheers, Philip

1 0