- WebODF - lists.nlnet.nl

OpRemoveText: A well-engineered stick in the eye
by Philip Peitsch 20 Sep '13

20 Sep '13

Hi folks, So for today's light reading, I thought I'd take a few moments and describe the intent and workings of OpRemoveText. Recent activity in iterators and with images has being bumping into the black-box nature of this, and it seemed like a good topic to discuss. History ======= OpRemoveText started life sometime in November, 2012. Originally written as a direct complement to OpInsertText, the earliest version was simply able to remove text data within a text node. Eventually, OpRemoveText grew to support complete removal of empty text nodes (early April, 2013, Friedrich), then support for removing text across multiple text nodes (late April, 2013, Aditya), and finally space element removal, paragraph merging and list item removal. The initial implementation was 66 lines long (including comments). By late July, this operation had grown to 250 lines, and was accruing a significant number of known limitations. These include things such as: - Inability to remove images, tables and other ODF objects - Erratic removal of ODF character elements (e.g., spaces & tabs) - Destruction of non-odf children when removing ODF containers The number of non-text items that needed to have special handling to be removed made it somewhat clear that the existing text neighbourhood approach was slowly being outgrown. Current ======= Mid July, 2013, an alternative approach for OpRemoveText was put forward. The limitations being specifically addressed in this rewrite were: - Improve the ability to support ODF objects, and make it easy to extend this for future object support - Eliminate the text-oriented design of the existing implementation - Preserve non-ODF elements during deletion (e.g., cursor nodes) without specialised handling The original OpRemoveText could be best described as delete-by-default behaviour. The implementation carried specific logic to handle and cleanup editinfo nodes, and keep the cursor nodes safely intact. The key issue with this approach was that integration of foreign elements into an ODF document were not able to be saved without patching OpRemoveText directly. In contrast, the current design is actually save-by-default. This is achieved by performing all node removal using the DomUtils.mergeIntoParent function. This new approach landed late July, and thus far has survived with very few major changes or bug fixes. How the magic works =================== Removal of content has a few major responsibilities attached: - Paragraph merging: Merging two paragraphs together has some special edge cases around what the resulting joined paragraph style should be. - Empty container cleanup: In order to keep the document in a relatively performant state, removal needs to do things such as discard unnecessary span elements, remove empty frames etc. - Maintain document structure: Removal of some elements should automatically clean up parent containers. For example, removing the last item in a list should result in the parent list also being removed. The basic approach used in OpRemoveText to meet these needs is as follows: 1. Given a position & length, translate this to a DOMRange (via position iterators) 2. Fetch a list of all ODF objects or text elements FULLY contained within this range - For each ODF object in this list, eliminate the object using domUtils.mergeIntoParent to remove the object whilst preserving all it's children - For each text node, delete the node entirely - Check the parent of each object/text node that was removed. If it is a container that should automatically collapse, and it is now empty, remove the parent using mergeIntoParent. Repeat this process on it's own parent. 3. Fetch a list of all ODF paragraphs that intersect the current range. Merge these together as all content within the range will have been removed. (Special rules for styling apply which I won't go into detail here) An auto-collapsing container is - Not a paragraph element (OpRemoveText, line 107) - Not a root element. E.g., office:text (OpRemoveText, line 107) - Not a character element (OpRemovetext, line 82) - Contains no ODF character elements (OpRemoveText, line 90) - Has no text content (OpRemoveText, line 86) The key concepts here are: 1. Collecting the set of objects/text elements that will be removed BEFORE doing any removal prevents counting issues. 2. Deletion of elements is order-agnostic. If the child is processed after the parent element, the parent can still automatically collapse automatically. 3. Auto-collapsing containers vs. specifically removable content starts to clarify ODF elements that are navigable content vs. containers for content. Worked Example: Merging two paragraphs ====================================== <text:p id="p1"> <text:span style="bold">Paragr<c:anchor/>aph 1 t<text:s/>ext</text:span> </text:p> <text:p id="p2"> <text:span style="italic">Paragraph 2<c:cursor/> text</text:span> </text:p> Assuming the user has selected the range between the anchor and cursor nodes and pressed the delete key: 1. Split text nodes at the selection boundaries. This ensures odtDocument.getTextElements will never return partially selected text content 2. Fetch all text elements (line 211). For this selection, this will return: ["aph 1 t", <text:s/>, "ext", "Paragraph 2"] 3. Fetch all intersecting paragraphs: [<text:p id="p1"/>, <text:p id="p2"/>] 4. For each text element, remove it from the DOM using mergeIntoParent (line 216). After this step the DOM looks like: <text:p id="p1"> <text:span style="bold">Paragr<c:anchor/></text:span> </text:p> <text:p id="p2"> <text:span style="italic"><c:cursor/> text</text:span> </text:p> 5. For each paragraph intersecting the selection, merge the paragraph contents into the first paragraph touching the selection (line 220). After this step, the DOM looks like: <text:p id="p1"> <text:span style="bold">Paragr<c:anchor/></text:span> <text:span style="italic"><c:cursor/> text</text:span> </text:p> <text:p id="p2"> </text:p> 6. Finally, remove each paragraph that was merged except the first. <text:p id="p1"> <text:span style="bold">Paragr<c:anchor/></text:span> <text:span style="italic"><c:cursor/> text</text:span> </text:p> 7. Fix the cursor positions (i.e., collapse now collapsed cursors etc.). In this instance, there are is no position difference between the front and end now (i.e., 0 steps difference), so the cursor is collapsed. <text:p id="p1"> <text:span style="bold">Paragr<c:cursor/></text:span> <text:span style="italic"> text</text:span> </text:p> Note, we never had to explicitly find and save the cursor or anchor nodes. These migrated up the hierarchy via the mergeIntoParent removal process of the ODF elements we wanted to delete. Worked Example: Removing a list item ==================================== Now, a slightly more complex removal example. <text:list> <text:list-item> <text:p id="p1"> <text:span style="bold">Paragr<c:anchor/>aph 1 t<text:s/>ext</text:span> </text:p> <text:list-item> <text:list-item> <text:p id="p2"> <text:span style="italic">Paragraph 2<c:cursor/> text</text:span> </text:p> <text:list-item> <text:list> Assuming the user has selected the range between the anchor and cursor nodes: 1. Split text nodes at the selection boundaries. This ensures odtDocument.getTextElements will never return partially selected text content 2. Fetch all text elements (line 211). For this selection, this will return: ["aph 1 t", <text:s/>, "ext", "Paragraph 2"] 3. Fetch all intersecting paragraphs: [<text:p id="p1"/>, <text:p id="p2"/>] 4. For each text element, remove it from the DOM using mergeIntoParent (line 216). After this step the DOM looks like: <text:list> <text:list-item> <text:p id="p1"> <text:span style="bold">Paragr<c:anchor/></text:span> </text:p> <text:list-item> <text:list-item> <text:p id="p2"> <text:span style="italic"><c:cursor/> text</text:span> </text:p> <text:list-item> <text:list> 5. For each paragraph intersecting the selection, merge the paragraph contents into the first paragraph touching the selection (line 220). After this step, the DOM looks like: <text:list> <text:list-item> <text:p id="p1"> <text:span style="bold">Paragr<c:anchor/></text:span> <text:span style="italic"><c:cursor/> text</text:span> </text:p> <text:list-item> <text:list-item> <text:p id="p2"> </text:p> <text:list-item> <text:list> 6. Finally, remove each paragraph that was merged except the first. Paragraph removal allows for the same container collapsing behaviour that text element removal does (line 166 & 169). Both a list, and a list-item fit the definition of an auto-collapsing container based on the previously defined rules, so these could get automatically cleaned up when the paragraph element is removed. After removing p2, the auto-collapse behaviour would see that the text:list-item is removable if it is empty. In this case, there is no more ODF characters or text content, so this element is collapsed. The list-item's parent list would also be checked to see if it can be removed. As the list still has another list-item, and the list-item contains a paragraph, and teh paragraph contains text and character data, the list will NOT be collapsed. <text:list> <text:list-item> <text:p id="p1"> <text:span style="bold">Paragr<c:anchor/></text:span> <text:span style="italic"><c:cursor/> text</text:span> </text:p> <text:list-item> <text:list> 7. Fix the cursor positions. <text:list> <text:list-item> <text:p id="p1"> <text:span style="bold">Paragr<c:cursor/></text:span> <text:span style="italic"> text</text:span> </text:p> <text:list-item> <text:list> Worked Example: Removing a list =============================== Finally, removing a list entirely. <text:p id="p1"> <text:span style="bold">Paragr<c:anchor/>aph 1 t<text:s/>ext</text:span> </text:p> <text:list> <text:list-item> <text:p id="p2"> <text:span style="italic">Paragraph 2<c:cursor/> text</text:span> </text:p> <text:list-item> <text:list> Assuming the user has selected the range between the anchor and cursor nodes: 1. Split text nodes at the selection boundaries. This ensures odtDocument.getTextElements will never return partially selected text content 2. Fetch all text elements (line 211). For this selection, this will return: ["aph 1 t", <text:s/>, "ext", "Paragraph 2"] 3. Fetch all intersecting paragraphs: [<text:p id="p1"/>, <text:p id="p2"/>] 4. For each text element, remove it from the DOM using mergeIntoParent (line 216). After this step the DOM looks like: <text:p id="p1"> <text:span style="bold">Paragr<c:anchor/></text:span> </text:p> <text:list> <text:list-item> <text:p id="p2"> <text:span style="italic"><c:cursor/> text</text:span> </text:p> <text:list-item> <text:list> 5. For each paragraph intersecting the selection, merge the paragraph contents into the first paragraph touching the selection (line 220). After this step, the DOM looks like: <text:p id="p1"> <text:span style="bold">Paragr<c:anchor/></text:span> <text:span style="italic"><c:cursor/> text</text:span> </text:p> <text:list> <text:list-item> <text:p id="p2"> </text:p> <text:list-item> <text:list> 6. Finally, remove each paragraph that was merged except the first. Remembering that paragraph removal includes auto-collapsing containers. When p2 is removed, it's text:list-item is checked to see if it can collapse. In this case, there is no content left, so the list-item will remove itself. It will then check if the list can collapse, which again is empty. As a result of these checks, the list is properly cleaned up once the content is removed. <text:p id="p1"> <text:span style="bold">Paragr<c:anchor/></text:span> <text:span style="italic"><c:cursor/> text</text:span> </text:p> 7. Fix the cursor positions as per usual <text:p id="p1"> <text:span style="bold">Paragr<c:cursor/></text:span> <text:span style="italic"> text</text:span> </text:p> Well... if that hasn't put you to sleep yet I'm impressed. Any queries, questions, or complaints... as ever, I'm happy to answer anything I can. Happy hacking! Philip

1 0

Performance again - event queue
by Tobias Hintze 20 Sep '13

20 Sep '13

Hi, I just read a mail from some time ago which I seem to have missed before on this ML. It's from 2013-08-06: '[WebODF] 2 month performance blitz for webodf'. There is one thing I would like to add to the specific example of terrible performance of our yet unoptimized code in this regard. Philip gave the example of 500ms delay for deleting a single character. That is of course bad - but it is even worse: What does the Average Joe do to delete 5 words? Right! He would press AND HOLD the delete key and stops when the words are gone. Ever tried that in the 500ms scenario? The problem is that the key event is processed immediately and the browser is busy for the 500ms, and when the browser comes back from being so busy, there will be like 100 more key events queued. The problem is obvious: the browser will be busy again for 500ms x 100 times, without any time in between for anything else. In my opinion we need to separate the event-reading from the event-processing. The event processing needs to happen in a way which gives the browser a small time slot to take care of its other duties as well. So my proposal is to read the key-event; perhaps also create the resultant Op from it, but then just enqueue the Op for execution. Then we have a chance to do sane scheduling of Op execution and making sure the browser does not starve by giving it small time slots for survival. The effect is that the browser does not appear locked for a long period to the user, but instead the users will see one execution rendered before the next one starts. We had a similar effect when playing long lists of Ops. Changing this to "chunked" Op-replay improved things. PS: I am aware that the scheduling would not prevent the queue to fill with 100 key-events while the browser is busy in execution. But it will change the user perception from "browser dead - webodf crashed" - to "it is a bit slow down here". th -- Tobias Hintze // KO GmbH http://kogmbh.com/legal/

1 0

Questions about core.PositionIterator
by Friedrich W. H. Kossebau 16 Sep '13

16 Sep '13

Hi, thank you Philip for your email about the point translation system, it put quite some light on the topic for me, and additionally also showed your POV, which is very valuable to have. Still I need to grasp some things and have some questions I need you all to answer, let me please do it class by class. Grep for "Q:" to see where I need help. So, here core.PositionIterator: I am still puzzled about what its properties are and what states does it have. Let's see: Current public methods: // modifying nextPosition() previousPosition() moveToEnd() moveToEndOfNode(node) setUnfilteredPosition(container, offset) // querying container() domOffset() getCurrentNode() unfilteredDomOffset() rightNode() leftNode() getPreviousSibling() getNextSibling() // querying, just used from tests getNodeFilter() So these properties: * a filter (at least one for empty textnodes, but can be extended by one passed as arg) * a root node from pure, unfiltered DOM, its subtree is where the iterator moves inside * a position, defined by a container and the current position in it Any other things queried are only derived from these core properties. Q: Correct? Q: What is the actual semantic of the container and the positions in it, to what possible points in the DOM can/should container and position point to? Q: Most API ignores the concept of container and offset and instead talks about nodes/siblings. But both use the same value of the "currentPos" var. Confuses me completely. Why is the container concept irrelevant in those calls? Or do these two different views on the iterator perfectly go along? So the states: The filter is at least the one for empty textnodes, so always exists and works. The root node is required in the API dox, so always exists (but could be moved out of the DOM during the lifetime of the iterator, API dox needs to require that this is not happening perhaps) The position is stored via the "walker" object and the "currentPos" variable. The passed root node could have a subtree with no positions (after filtering). Q: What values would the properties have in that state? Q: Or do we have to require that the root node allows at least one position? Q: Should the iterator always be in positions which are allowed by the filter? "moveToEndOfNode()" could resolve in a position that is not valid, because of "walker.currentNode = node;" which will not check against the root node or the filter (/me to blame for that code, but noone cleaned it as well ;) ). "setUnfilteredPosition()" is also not checking if the container is inside the root. Seems to me the states of the position are not properly defined, or at least slightly fragile with some methods. Questions for the implementation: Q: Why does container() return the parentNode in case of currentPos === 0 && t !== Node.TEXT_NODE, and not only if t !== Node.TEXT_NODE? Q: So what is the semantics of "container", where is the difference to "currentNode"? Q: In init() why is the currentPos set to 1 if walker.firstChild() === null? Q: Why seems currentPos === 0 only valid with existing visible child nodes, what? And than these naming issues continue to confuse me: * getter methods named "get*" vs "*" * getter and setter methods too similar, e.g. "nextPosition" vs. "rightNode" * mixing of "right"/"left" and "next"/previous" * "dom" in the "*Offset" methods is redundant, no other offset method * "setUnfilteredPosition" sounds to me as if there is an unfiltered position possible and that gets set Q: What do you think about these renamings: // modifying moveToNextPosition() <- nextPosition() moveToPreviousPosition() <- previousPosition() moveToEnd() moveToEndOfNode(node) setPositionByUnfilteredPosition(container, offset) <- setUnfilteredPosition(*) (hm, ideas for better name? moveCloseToUnfilteredPosition?) // querying getContainer() <- container() getOffset() <- domOffset() getCurrentNode() getUnfilteredOffset() <- unfilteredDomOffset() getRightNode() <- rightNode() getLeftNode() <- leftNode() getPreviousSibling() getNextSibling() // querying, just used from tests getNodeFilter() Sorry, I too long postponed getting a clue of all this (again), but better now than never. Cheers Friedrich -- Friedrich W. H. Kossebau // KO GmbH http://kogmbh.com/legal/

2 1

WebODF point translation system: PositionIterators, Filters, SelectionMovers and other rabbits...
by Philip Peitsch 13 Sep '13

13 Sep '13

Hi all, Based on some recent activities (and some questions by Friedrich), it seemed like a good time to sit down and try and disseminate some knowledge about some more confusing concepts such as filtering, position iterators and other friends. I suppose the first point to start at is what need is being solved. Anyone who has worked with javascript for a small amount of time is likely familiar with DOM Ranges, elements, and other DOM manipulation methods. A DOM Range shows quite clearly the normal coordinate system that the browser uses internally, being Node + Offset. A particular node reference is guarenteed to be unique within the document. This makes up the concept I'll refer to as the "DOM coordinate" system, as it uses in-memory references to active nodes in the document. For collaborative editing though, we need to be able to share information between different computers. In this case, we can't use DOM coordinates because there is no built-in way to serialize a reference to a particular DOM and offset such that another computer will be able to select the exact point. So, another coordinate system needs to be layered on top of the DOM coordinates. In WebODF, this is most clearly seen on Operations, which carry single integers (usually position + length pairs). I'll refer to this concept as the "ODF coordinate" system. For WebODF, ODF coords are actually cursor positions. This wasn't a deliberate design from what I can tell (and I have a few misgivings about it), but it is functional enough to allow collaboration with the current codebase. Now, I want to clarify up front that none of this is my design. I'm simply repeating what I've discovered from the codebase, so please don't ask me to justify why it was created this way. Design ====== The high level classes used in translating between DOM / ODF coordinate systems: --------------------- core.PositionIterator --------------------- A relatively light wrapper around the standard HTML TreeWalker concept. This class provides methods to navigate across available positions within a DOM (a position being a DOM coordinate pair of a node + offset). It has some basic logic to skip some types of equivalent positions during iteration. This class takes a "node filter" that can be used to exclude certain nodes. It also provides a setUnfilteredPosition method that will set the PositionIterator instance to the next acceptable node based on the node filter (this is something the normal TreeWalker.currentNode assignemnt does not guarantee). Historically, this used to contain some other translation functions. These other methods have been retired during the great coordinates purge of a few months back[1]. ------------------------------------------------- core.PositionIterator.FilteredEmptyTextNodeFilter gui.SelectionMover.CursorFilter ------------------------------------------------- These are examples of node filters. These are passed into the PositionIterator on construction. A node level filter can return whether a particular node should be returned as a result of a call to one of the PositionIterator's navigation functions (e.g., iterator.nextPosition). For example, a CursorFilter is used to prevent the WebODF cursor element from being returned as an available DOM coordinate. ------------------ gui.SelectionMover ------------------ The SelectionMover class is the heart & soul of the coordinate translation system. The SelectionMover is bound to an OdtCursor. It provides a bunch of functions that take a "position filter", and can do things such as move the cursor a set number of valid positions in a direction, or can report the number of valid positions to a particular point. This is where the confusion starts. A "position filter" is an object that supplies a single function "acceptPosition" and when given a DOM coordinate reports whether the DOM coordinate is a valid position as far as the filter is concerned. The most commonly used "position filter" is ops.OdtDocument.TextPositionFilter, which is an implementation of valid ODT cursor positions as described in README_cursorpositions. The code for converting DOM to ODF coordinates is generally something like: iterator = new core.PositionIterator(new core.CursorFilter()) iterator.setUnfilteredPosition(documentRoot, 0) // Starting at the beginning of the doc positionFilter = new TextPositionFilter() odfCoordinate = 0 // nextPosition uses the "node filter" to find the next non-cursor node while (iterator.nextPosition() && hasNotPassed(DOMCoordinate)) { // the "position filter" is used to figure out how many valid positions have // been passed if (positionFilter.acceptPosition(iterator)) { odfCoordinate += 1; } } Conversion from ODF to DOM coordinates is almost identical: iterator = new core.PositionIterator(new core.CursorFilter()) iterator.setUnfilteredPosition(documentRoot, 0) // Starting at the beginning of the doc positionFilter = new TextPositionFilter() // nextPosition uses the "node filter" to find the next non-cursor node while (odfCoordinates !== 0 && iterator.nextPosition()) { // the "position filter" is used to figure out how many valid positions have // been passed if (positionFilter.acceptPosition(iterator)) { odfCoordinate -= 1; } } DOMCoordinate = (iterator.node, iterator.offset) --------------- ops.OdtDocument --------------- This is the primary object for accessing cursors, and translating between ODF and DOM coordinates. The most commonly used functions are: getCursorPosition - Returns the ODF coordinate of a specific cursor getCursorSelection - Returns the ODF coordinate including selection of a specific cursor getDistanceFromCursor - Returns the difference in ODF coordinates between the cursor and the specified DOM coordinate Other useful functions to know about: getPositionFilter - Returns the "position filter" instance that is used for converting between ODF and DOM coordinates. This is an instance of the TextPositionFilter generally (with some extra magic around document roots, which I'm ignoring for now) Summary ======= So, to recap this architecture: - A PositionIterator takes a NodeFilter as a construction argument. - The SelectionMover has a PositionIterator, and supplies the appropriate NodeFilter to this iterator - Each SelectionMover is bound is tied to a single OdtCursor - The SelectionMover uses a per-call PositionFilter to convert between ODF and DOM coordinates - The OdtDocument creates all OdtCursors, SelectionMovers and PositionFilters, and links these all together appropriately - The only used PositionFilter is a TextPositionFilter, as described in README_cursorpositions (shhh Adityab. Don't bring up the root filters yet :D) Ok. I'll leave this email here at this point :-). Any questions/queries/clarifications... don't hesitate to ask... but don't forget... I didn't design this... I'm just the messenger ;-) Cheers, Philip Footnotes 1. https://open.nlnet.nl/pipermail/webodf/2013-June/000029.html

1 0

Heads up: demo/example html pages of programs/editor have changed
by Friedrich W. H. Kossebau 03 Sep '13

03 Sep '13

Hi, programs/editor/cdn-index.html & Co. are no longer in current master, please welcome the new pages: for from-the-build-dir programs/editor/collabeditor.html programs/editor/localeditor.html programs/editor/index.html (to select between, could get some style makeup) and for the from-the-sources programs/editor/src-collabeditor.html programs/editor/src-localeditor.html Just in case you were wondering why your bookmarks/cached links are no longer working. boot_editor.js has been put to rest, freeing it from the am-both-local-editor- and-server-based-editor-perhaps split nature. Now there is just a simple example for a single-user/local editor and a simple example for a server-based editor (modulo example server, but that hopefully changes in some near future). Cheers Friedrich -- Friedrich W. H. Kossebau // KO GmbH http://kogmbh.com/legal/

1 0

Foreign elements & cursor positioning
by Philip Peitsch 27 Aug '13

27 Aug '13

Hi everyone, I have been looking at some funky "valid" ODT documents and discovering some fun behaviours around cursor positioning. I thought this was a good time to bring up some potential changes, as I noticed on IRC the other night that Friedrich had also discovered sometimes the user is unable to place the cursor inside a text:a block. The existing rules for valid cursor positions only allow placing the cursor inside what is known as a "grouping element", which is either a span, p or h. This has already caused me some challenges, because for one of the highlight overlays I'm doing on top of webodf I need to wrap document text in a normal HTML span, which then prevents the cursor from entering. In the situation Friedrich found, there is actually no requirement saying the text content within a text:a element must be placed in a span. The suggestion on IRC yesterday was to just add text:a into the grouping element definitions (a reasonable one), but I got to thinking a little more about whether this is the best long-term solution. Especially as there are other elements that could possible contain character data. And for extensibility purposes, ideally we don't want to have to redo this core cursor positioning logic every time the UI requires some extra containers and wrappers to help display things :). Re-reading the ODF specs[1], the preferred approach laid out is actually a blacklist, not a whitelist as we're currently doing. The blacklist as required for processing is already defined & used in StyleHelper.isAcceptedNode, is used for OpRemoveText and OpApplyDirectStyling. Would anyone have any problems if I changed OdtDocument.TestPositionFilter to use the blacklist approach instead? The blacklist will need an additional entry to exclude the cursor as well, so I'll put that in also. >From my testing with this, it appears to function identically to the existing approach, with the added bonus of being able to navigate within text:a tags that don't contain a span :) Cheers, Philip P.S., sorry for all the list spam lately! Apparently I have too much time for philosophy :) [1] http://docs.oasis-open.org/office/v1.2/os/OpenDocument-v1.2-os-part1.html#F…

2 2

2 month performance blitz for webodf
by Philip Peitsch 14 Aug '13

14 Aug '13

Hi all, I'm just putting together a quick plan of what I'll be looking at with regards to performance, to address some queries & requests raised as part of MR#114 (https://gitorious.org/webodf/webodf/merge_requests/114) As part of this next cycle, my goal is to have editing of a 20 page ODT and flat ODT with images up to a level that is responsive. At the moment, deleting a single character at the end of a sample 11 page document produces an extremely noticable 500ms delay before the character is gone. >From my initial investigation so far my plan of attack for this is: 1. Improve obviously suboptimal paths: OdtDocument * TextPositionFilter - Most of the container checks should be filtered nodes 2. Eliminate the number of times the average run needs to step through the document to find a position: * e.g., upgradeWhiteSpace is called at a specific position. Any Op using this therefore steps through the document up to that specific position usually 2 or 3 times. 3. Implement a bookmark system to quickly retrieve iterators at specific positions within the document. I've used one of these internally for several months now at a different layer above webodf, and have found this to be the most significant improvement. The plan with each of these is to introduce benchmarking numbers to allow the performance improvements to be proven. As such, I don't plan on addressing any of the performance related concerns in MR#114, as they are literally (from initial profiling checks) drops in a very large ocean of improvement. If people have other ideas, complaints, etc., as ever, I'm open to anything :) Cheers, Philip

3 2

MR#122
by Philip Peitsch 13 Aug '13

13 Aug '13

Hi, Just wanted to politely bump https://gitorious.org/webodf/webodf/merge_requests/122 for review. It's been waiting about a week now for any feedback. Cheers, Philip

1 0

Undo/redo actions
by Philip Peitsch 31 Jul '13

31 Jul '13

Hi everybody, I've been considering the following thought experiment for undo/redo: 1. Doc state = A 2. Op A is performed by Mandy, Doc state = B 3. Op B is performed by Fred, Doc state = C 4. Mandy hits undo on her local editor 5. Doc state = ?? What does Mandy expect to have happen? Does this undo Op A or Op B? Etherepad's timeline view is a global undo stack, i.e., you can undo changes from any person to revert the document back to a previous state. In contrast, when typing on a collaborative document, does the user expect undo to revert *their* most recent change, or the *document's* most recent change? This leads directly into the next question. Does undo either: a) revert the document to a previous state, resulting in a document in an identical state as before the operation was originally performed e.g., 1. Doc state = A 2. Op A is performed, Doc state = B 3. Op A is undone via magic, Doc state = A OR b) undo a previous action, resulting in a document that is *similar* but not identical to before the operation was performed 1. Doc state = A 2. Op A is performed, Doc state = B 3. Op A is undone via magic, Doc state = C What is the difference you ask? Undo for direct formatting, range removal, etc. is really tricky because in the current implementations data is irretreviably lost as part of execution. The interactions are also far more complex than most other operations. E.g., What should happen if Fred inserts new text into the middle of a block Mandy is removing? What should happen if Fred inserts new text into the middle of a block Mandy is formatting? To implement undo option A with these types of operations the ops must be re-written to do less, pushing the complexity out into what generates the op. E.g., OpRemoveText is only allowed to remove text nodes, and SessionController (or a helper) is responsible for coming up with the exact list of text nodes to be removed, and generating an OpRemoveText for each of these. If however Option B is acceptable (i.e., the document should be visually indistinguishable, but is allowed to be different at the DOM level) there is another potential solution. Assuming option B and local undo stack: * Each client will generate their own local undo stack as each local operation is executed. * This local undo stack is rebased on every external change using the standard OT behaviours, and is not shared. * At the point the user undoes their recent change, the undo operations are forwarded from their local client as if it was a normal change. Thoughts, criticisms, concerns or shouting are all welcome :) Philip

3 2

RFC: Making OpApplyStyle and OpUpdateParagraphStyle more consistent
by Friedrich W. H. Kossebau 27 Jul '13

27 Jul '13

Hi, OpApplyStyle and OpUpdateParagraphStyle are currently quite different: a) how they encode the style attributes to change b) OpApplyStyle does not really support unsetting of attributes c) OpApplyStyle and related code only does textstyle properties d) OpApplyStyle does not directly modify existing styles, but create any needed automatic styles to have the given styling e) OpApplyStyle has a too generic name Before I start to write anything for OT of OpApplyStyle things needs to be cleaned up a little, to reduce the complexity due to the different behaviour. For a) OpApplyStyle takes some "info" payload that are actually directly the ODF element names and properties+values that should be set to the direct formatting. The names of the property and attributes use the usual prefix to indicate the namespace. OpUpdateParagraphStyle instead has a "setProperties" payload that carries the properties+values to set, with an own id scheme that first needs to be mapped to the ODF values. Q: anyone would mind to rename that "info" property to "setProperties", for some more entropy in the name? Okay to remove the custom property names and mapping in OpUpdateParagraphStyle and use directly the ODF attribute names as well? In matters of b), OpUpdateParagraphStyle has a payload "removedProperties" which stores the names of all attributes that should be removed. That was done like that instead of listing the attribute with value >null<, >undefined< or "" in the "setProperties" to express that is should be removed, because >null< and >undefined< might not exist in other languages and "" could be an actual valid value. Q: does it make sense to add something similar like "removedProperties" to OpApplyStyle? I guess it does. For c) I wonder what the plans are for paragraph styling. There is no TODO or anything else mentioned. And I am only thinking of the op spec, not of the actual implementation how to execute this op. Q: Should the same op be used for direct paragraph styling? So should there be, like in OpUpdateParagraphStyle, another section "paragraphProperties", next to "textProperties"? The difference mentioned in d) seems wanted, as this op is rather a direct mapping of some UI action, like we have with OpRemoveText or OpSplitParagraph. So no question about that issue. WRT e), I propose the name "OpApplyDirectStyling" Q: Better name? Cheers Friedrich -- Friedrich W. H. Kossebau // KO GmbH http://kogmbh.com/legal/

3 3