RFC: Ensure emission of signals only after an operation is done, and only due to operations

webodf/lib/gui/SessionController.js

@@ -304,7 +304,7 @@ gui.SessionControllerOptions = function () {
          * @return {undefined}
          */
         function forwardUndoStackChange(e) {
-            odtDocument.emit(ops.OdtDocument.signalUndoStackChanged, e);
+            odtDocument.DONOTUSE_emitSignalUndoStackChanged(e);
         }
 
         /**
@@ -374,7 +374,7 @@ gui.SessionControllerOptions = function () {
                             newSelectionRange.setEnd(shadowCursorIterator.container(), shadowCursorIterator.unfilteredDomOffset());
                         }
                         shadowCursor.setSelectedRange(newSelectionRange, handleEnd === 'right');
-                        odtDocument.emit(ops.Document.signalCursorMoved, shadowCursor);
+                        odtDocument.DONOTUSE_emitSignalCursorMoved(shadowCursor);
                     }
                 }
             }
@@ -400,7 +400,7 @@ gui.SessionControllerOptions = function () {
                         selectionController.expandToParagraphBoundaries(selectionRange.range);
                     }
                     shadowCursor.setSelectedRange(selectionRange.range, selectionRange.hasForwardSelection);
-                    odtDocument.emit(ops.Document.signalCursorMoved, shadowCursor);
+                    odtDocument.DONOTUSE_emitSignalCursorMoved(shadowCursor);
                 }
             }
         }

webodf/lib/gui/SessionView.js

@@ -410,7 +410,7 @@ gui.SessionViewOptions = function () {
             var annotationViewManager = odfCanvas.getAnnotationViewManager();
             if (annotationViewManager) {
                 annotationViewManager.rehighlightAnnotations();
-                odtDocument.fixCursorPositions();
+                odtDocument.fixCursorPositions(false); // TODO: workaround, ideally this would not be needed
             }
         }
 

webodf/lib/ops/OdtDocument.js

@@ -154,7 +154,7 @@ ops.OdtDocument = function OdtDocument(odfCanvas) {
         initialDoc = rootElement.cloneNode(true);
         odfCanvas.refreshAnnotations();
         // workaround AnnotationViewManager not fixing up cursor positions after creating the highlighting
-        self.fixCursorPositions();
+        self.fixCursorPositions(false);
         return initialDoc;
     };
 
@@ -165,18 +165,13 @@ ops.OdtDocument = function OdtDocument(odfCanvas) {
         var odfContainer = odfCanvas.odfContainer(),
             rootNode;
 
-        eventNotifier.unsubscribe(ops.OdtDocument.signalStepsInserted, stepsTranslator.handleStepsInserted);
-        eventNotifier.unsubscribe(ops.OdtDocument.signalStepsRemoved, stepsTranslator.handleStepsRemoved);
-
         // TODO Replace with a neater hack for reloading the Odt tree
         // Once this is fixed, SelectionView.addOverlays can be removed
         odfContainer.setRootElement(documentElement);
         odfCanvas.setOdfContainer(odfContainer, true);
         odfCanvas.refreshCSS();
         rootNode = getRootNode();
         stepsTranslator = new ops.OdtStepsTranslator(rootNode, createPositionIterator(rootNode), filter, 500);
-        eventNotifier.subscribe(ops.OdtDocument.signalStepsInserted, stepsTranslator.handleStepsInserted);
-        eventNotifier.subscribe(ops.OdtDocument.signalStepsRemoved, stepsTranslator.handleStepsRemoved);
     };
 
     /**
@@ -449,15 +444,20 @@ ops.OdtDocument = function OdtDocument(odfCanvas) {
      * document's metadata, such as dc:creator,
      * meta:editing-cycles, and dc:creator.
      * @param {!ops.Operation} op
+     * @param {!Array.<!ops.Operation.Event>} events
+     * @return {undefined}
      */
-    function handleOperationExecuted(op) {
+    function finishOperationExecution(op, events) {
         var opspec = op.spec(),
             memberId = opspec.memberid,
             date = new Date(opspec.timestamp).toISOString(),
             odfContainer = odfCanvas.odfContainer(),
-            /**@type{!{setProperties: !Object, removedProperties: ?Array.<!string>}}*/
+            /**@type{!ops.Operation.Event}*/
+            changedMetadataEvent,
+            /**@type{!{setProperties: !Object, removedProperties: !Array.<!string>}}*/
             changedMetadata,
-            fullName;
+            fullName,
+            i;
 
         // If the operation is an edit (that changes the
         // ODF that will be saved), then update metadata.
@@ -468,13 +468,25 @@ ops.OdtDocument = function OdtDocument(odfCanvas) {
                 "dc:date": date
             }, null);
 
-            changedMetadata = {
-                setProperties: {
-                    "dc:creator": fullName,
-                    "dc:date": date
-                },
-                removedProperties: []
-            };
+            // find any existing metadataupdated event or create one
+            for (i = 0; i < events.length; i += 1) {
+                if (events[i].eventid === ops.OdtDocument.signalMetadataUpdated) {
+                    changedMetadataEvent = events[i];
+                    changedMetadata = /**@type{!{setProperties: !Object, removedProperties: !Array.<!string>}}*/(changedMetadataEvent.args);
+                    break;
+                }
+            }
+            if (!changedMetadataEvent) {
+                changedMetadata = { setProperties: {}, removedProperties: [] };
+                changedMetadataEvent = {
+                    eventid: ops.OdtDocument.signalMetadataUpdated,
+                    args: changedMetadata
+                };
+                events.push(changedMetadataEvent);
+            }
+
+            changedMetadata.setProperties["dc:creator"] = fullName;
+            changedMetadata.setProperties["dc:date"] = date;
 
             // If no previous op was found in this session,
             // then increment meta:editing-cycles by 1.
@@ -492,10 +504,49 @@ ops.OdtDocument = function OdtDocument(odfCanvas) {
             }
 
             lastEditingOp = op;
-            self.emit(ops.OdtDocument.signalMetadataUpdated, changedMetadata);
         }
+
+        eventNotifier.emit(ops.OdtDocument.signalOperationEnd, op);
+
+        events.forEach(function(event) {
+            eventNotifier.emit(event.eventid, event.args);
+        });
     }
 
+    /**
+     * @param {!ops.Operation} op
+     * @return {!boolean}
+     */
+    this.executeOperation = function(op) {
+        var events;
+
+        eventNotifier.emit(ops.OdtDocument.signalOperationStart, op);
+
+        events = op.execute(self);
+        if (events === null) {
+            return false;
+        }
+
+        finishOperationExecution(op, events);
+        return true;
+    };
+
+    /**
+     * @param {*} args
+     * @return {undefined}
+     */
+    this.prepareBatchProcessing = function(args) {
+        eventNotifier.emit(ops.OdtDocument.signalProcessingBatchStart, args);
+    };
+
+    /**
+     * @param {*} args
+     * @return {undefined}
+     */
+    this.finishBatchProcessing = function (args) {
+        eventNotifier.emit(ops.OdtDocument.signalProcessingBatchEnd, args);
+    };
+
     /**
      * Upgrades literal whitespaces (' ') to <text:s> </text:s>,
      * when given a textNode containing the whitespace and an offset
@@ -673,8 +724,14 @@ ops.OdtDocument = function OdtDocument(odfCanvas) {
      * walkable positions; if not, move the cursor 1 filtered step backward
      * which guarantees walkable state for all cursors,
      * while keeping them inside the same root. An event will be raised for this cursor if it is moved
+     * @param {!Array.<!ops.Operation.Event>|!boolean} events  array to add events for moved cursors, needs to be otherwise explicitely set to false
+     * @return {undefined}
      */
-    this.fixCursorPositions = function () {
+    this.fixCursorPositions = function (events) {
+        var /** @type{!Array.<!ops.OdtCursor>}*/ movedCursors = [];
+
+        runtime.assert(Array.isArray(events) || (typeof events === "boolean" && events === false), "second parameter to fixCursorPositions needs to be set to false or an event");
+
         Object.keys(cursors).forEach(function (memberId) {
             var cursor = cursors[memberId],
                 root = getRoot(cursor.getNode()),
@@ -717,9 +774,19 @@ ops.OdtDocument = function OdtDocument(odfCanvas) {
 
             if (cursorMoved) {
                 cursor.setSelectedRange(selectedRange, cursor.hasForwardSelection());
-                self.emit(ops.Document.signalCursorMoved, cursor);
+                movedCursors.push(cursor);
             }
         });
+
+        if (events) {
+            movedCursors.forEach(function(cursor) {
+                events.push({ eventid: ops.Document.signalCursorMoved, args: cursor });
+            });
+        } else {
+            movedCursors.forEach(function(cursor) {
+                eventNotifier.emit(ops.Document.signalCursorMoved, cursor);
+            });
+        }
     };
 
     /**
@@ -856,7 +923,6 @@ ops.OdtDocument = function OdtDocument(odfCanvas) {
         if (cursor) {
             cursor.removeFromDocument();
             delete cursors[memberid];
-            self.emit(ops.Document.signalCursorRemoved, memberid);
             return true;
         }
         return false;
@@ -890,15 +956,6 @@ ops.OdtDocument = function OdtDocument(odfCanvas) {
         return odfCanvas.getFormatting();
     };
 
-    /**
-     * @param {!string} eventid
-     * @param {*} args
-     * @return {undefined}
-     */
-    this.emit = function (eventid, args) {
-        eventNotifier.emit(eventid, args);
-    };
-
     /**
      * @param {!string} eventid
      * @param {!Function} cb
@@ -942,6 +999,60 @@ ops.OdtDocument = function OdtDocument(odfCanvas) {
         callback();
     };
 
+    /**
+     * Process steps being inserted into the document. Will add a steps inserted signal
+     * to the passed events list
+     * @param {!{position: !number}} args
+     * @param {!Array.<!ops.Operation.Event>} events
+     * @return {undefined}
+     */
+    this.handleStepsInserted = function(args, events) {
+        stepsTranslator.handleStepsInserted(args);
+        // signal not used in webodf, but 3rd-party (NVivo)
+        events.push({
+            eventid: ops.OdtDocument.signalStepsInserted,
+            args: args
+        });
+    };
+
+    /**
+     * Process steps being removed from the document. Will emit a steps removed signal on
+     * behalf of the caller
+     * @param {!{position: !number}} args
+     * @param {!Array.<!ops.Operation.Event>} events
+     * @return {undefined}
+     */
+    this.handleStepsRemoved = function(args, events) {
+        stepsTranslator.handleStepsRemoved(args);
+        // signal not used in webodf, but 3rd-party (NVivo)
+        events.push({
+            eventid: ops.OdtDocument.signalStepsRemoved,
+            args: args
+        });
+    };
+
+    /**
+     * Emit the signal that the passed cursor moved.
+     * TODO: get rid of this method, noone should be able to emit signals by direct calls, only by op execution
+     * @internal
+     * @param {!ops.OdtCursor} cursor
+     * @return {undefined}
+     */
+    this.DONOTUSE_emitSignalCursorMoved = function(cursor) {
+        eventNotifier.emit(ops.Document.signalCursorMoved, cursor);
+    };
+
+    /**
+     * Emit the signal that the passed cursor moved.
+     * TODO: get rid of this method, noone should be able to emit signals by direct calls, only by op execution
+     * @internal
+     * @param {?Event} e
+     * @return {undefined}
+     */
+    this.DONOTUSE_emitSignalUndoStackChanged = function(e) {
+        eventNotifier.emit(ops.OdtDocument.signalUndoStackChanged, e);
+    };
+
     /**
      * @return {undefined}
      */
@@ -951,9 +1062,6 @@ ops.OdtDocument = function OdtDocument(odfCanvas) {
         filter = new ops.TextPositionFilter();
         stepUtils = new odf.StepUtils();
         stepsTranslator = new ops.OdtStepsTranslator(rootNode, createPositionIterator(rootNode), filter, 500);
-        eventNotifier.subscribe(ops.OdtDocument.signalStepsInserted, stepsTranslator.handleStepsInserted);
-        eventNotifier.subscribe(ops.OdtDocument.signalStepsRemoved, stepsTranslator.handleStepsRemoved);
-        eventNotifier.subscribe(ops.OdtDocument.signalOperationEnd, handleOperationExecuted);
         eventNotifier.subscribe(ops.OdtDocument.signalProcessingBatchEnd, core.Task.processTasks);
     }
     init();

webodf/lib/ops/OpAddAnnotation.js

@@ -137,13 +137,15 @@ ops.OpAddAnnotation = function OpAddAnnotation() {
 
     /**
      * @param {!ops.Document} document
+     * @return {?Array.<!ops.Operation.Event>}
      */
     this.execute = function (document) {
         var odtDocument = /**@type{ops.OdtDocument}*/(document),
             annotation, annotationEnd,
             cursor = odtDocument.getCursor(memberid),
             selectedRange,
-            paragraphElement;
+            paragraphElement,
+            events = [];
 
         doc = odtDocument.getDOMDocument();
 
@@ -158,7 +160,7 @@ ops.OpAddAnnotation = function OpAddAnnotation() {
             insertNodeAtPosition(odtDocument, annotationEnd, position + length);
         }
         insertNodeAtPosition(odtDocument, annotation, position);
-        odtDocument.emit(ops.OdtDocument.signalStepsInserted, {position: position});
+        odtDocument.handleStepsInserted({position: position}, events);
 
         // Move the cursor inside the new annotation,
         // by selecting the paragraph's range.
@@ -168,14 +170,14 @@ ops.OpAddAnnotation = function OpAddAnnotation() {
             selectedRange.selectNodeContents(paragraphElement);
             cursor.setSelectedRange(selectedRange, false);
             cursor.setSelectionType(ops.OdtCursor.RangeSelection);
-            odtDocument.emit(ops.Document.signalCursorMoved, cursor);
+            events.push({eventid: ops.Document.signalCursorMoved, args: cursor});
         }
         // Track this annotation
         odtDocument.getOdfCanvas().addAnnotation(annotation);
-        odtDocument.fixCursorPositions();
-        odtDocument.emit(ops.OdtDocument.signalAnnotationAdded, { memberId: memberid, annotation: annotation });
+        odtDocument.fixCursorPositions(events);
+        events.push({eventid: ops.OdtDocument.signalAnnotationAdded, args: { memberId: memberid, annotation: annotation }});
 
-        return true;
+        return events;
     };
 
     /**

webodf/lib/ops/OpAddCursor.js

@@ -46,20 +46,21 @@ ops.OpAddCursor = function OpAddCursor() {
 
     /**
      * @param {!ops.Document} document
+     * @return {?Array.<!ops.Operation.Event>}
      */
     this.execute = function (document) {
         var odtDocument = /**@type{ops.OdtDocument}*/(document),
             cursor = odtDocument.getCursor(memberid);
 
         // there should be none
         if (cursor) {
-            return false;
+            return null;
         }
 
         cursor = new ops.OdtCursor(memberid, odtDocument);
         odtDocument.addCursor(cursor);
-        odtDocument.emit(ops.Document.signalCursorAdded, cursor);
-        return true;
+
+        return [{eventid: ops.Document.signalCursorAdded, args: cursor}];
     };
 
     /**

webodf/lib/ops/OpAddMember.js

@@ -52,19 +52,19 @@ ops.OpAddMember = function OpAddMember() {
 
     /**
      * @param {!ops.Document} document
+     * @return {?Array.<!ops.Operation.Event>}
      */
     this.execute = function (document) {
         var odtDocument = /**@type{ops.OdtDocument}*/(document),
             member;
         if (odtDocument.getMember(memberid)) {
-            return false;
+            return null;
         }
 
         member = new ops.Member(memberid, setProperties);
         odtDocument.addMember(member);
-        odtDocument.emit(ops.Document.signalMemberAdded, member);
 
-        return true;
+        return [{eventid: ops.Document.signalMemberAdded, args: member}];
     };
 
     /**