Merge pull request #3125 from CatoTH/pass-line-numbers-through-diff

Beginning: passing line numbers through the diff

openslides/motions/static/js/motions/base.js

@@ -756,7 +756,8 @@ angular.module('OpenSlidesApp.motions', [
                         oldText = data.outerContextStart + data.innerContextStart +
                             data.html + data.innerContextEnd + data.outerContextEnd;
 
-                    var diff = diffService.diff(oldText, this.text, lineLength, this.line_from);
+                    oldText = lineNumberingService.insertLineNumbers(oldText, lineLength, null, null, this.line_from);
+                    var diff = diffService.diff(oldText, this.text);
 
                     if (highlight > 0) {
                         diff = lineNumberingService.highlightLine(diff, highlight);

openslides/motions/static/js/motions/diff.js

@@ -816,15 +816,14 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
             arr = splitArrayEntriesSplitSeparator(arr, ".");
             arr = splitArrayEntriesEmbedSeparator(arr, "\n", false);
 
-            return arr;
+            var arrWithoutEmptes = [];
-        };
+            for (var i = 0; i < arr.length; i++) {
-
+                if (arr[i] !== '') {
-        this._outputcharcode = function (pre, str) {
+                    arrWithoutEmptes.push(arr[i]);
-            var arr = [];
+                }
-            for (var i = 0; i < str.length; i++) {
-                arr.push(str.charCodeAt(i));
             }
-            console.log(str, pre, arr);
+
+            return arrWithoutEmptes;
         };
 
         /**
@@ -842,13 +841,11 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
 
             if (out.n.length === 0) {
                 for (i = 0; i < out.o.length; i++) {
-                    //this._outputcharcode('del', out.o[i]);
                     str += '<del>' + out.o[i] + "</del>";
                 }
             } else {
                 if (out.n[0].text === undefined) {
                     for (var k = 0; k < out.o.length && out.o[k].text === undefined; k++) {
-                        //this._outputcharcode('del', out.o[k]);
                         str += '<del>' + out.o[k] + "</del>";
                     }
                 }
@@ -862,7 +859,6 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
                         var pre = "";
 
                         for (var j = out.n[i].row + 1; j < out.o.length && out.o[j].text === undefined; j++) {
-                            //this._outputcharcode('del', out.o[j]);
                             pre += '<del>' + out.o[j] + "</del>";
                         }
                         str += out.n[i].text + pre;
@@ -1005,10 +1001,33 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
                 return cached;
             }
 
-            var str = this._diffString(htmlOld, htmlNew),
+            // This fixes a really strange artefact with the diff that occures under the following conditions:
+            // - The first tag of the two texts is identical, e.g. <p>
+            // - A change happens in the next tag, e.g. inserted text
+            // - The first tag occures a second time in the text, e.g. another <p>
+            // In this condition, the first tag is deleted first and inserted afterwards again
+            // Test case: "does not break when an insertion followes a beginning tag occuring twice"
+            // The work around inserts to tags at the beginning and removes them afterwards again,
+            // to make sure this situation does not happen (and uses invisible pseudo-tags in case something goes wrong)
+            var workaroundPrepend = "<DUMMY><PREPEND>";
+
+            var str = this._diffString(workaroundPrepend + htmlOld, workaroundPrepend + htmlNew),
                 diffUnnormalized = str.replace(/^\s+/g, '').replace(/\s+$/g, '').replace(/ {2,}/g, ' ')
                 .replace(/<\/ins><ins>/gi, '').replace(/<\/del><del>/gi, '');
 
+            diffUnnormalized = diffUnnormalized.replace(
+                /<ins>(\s*)(<p( [^>]*)?>[\s\S]*?<\/p>)(\s*)<\/ins>/gim,
+                function(match, whiteBefore, inner, tagInner, whiteAfter) {
+                    return whiteBefore +
+                        inner
+                        .replace(/<p( [^>]*)?>/gi, function(match) {
+                            return match + "<ins>";
+                        })
+                        .replace(/<\/p>/gi, "</ins></p>") +
+                        whiteAfter;
+                }
+            );
+
             diffUnnormalized = diffUnnormalized.replace(/<del>([a-z0-9,_-]* ?)<\/del><ins>([a-z0-9,_-]* ?)<\/ins>/gi, function (found, oldText, newText) {
                 var foundDiff = false, commonStart = '', commonEnd = '',
                     remainderOld = oldText, remainderNew = newText;
@@ -1046,6 +1065,17 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
                 return out;
             });
 
+            diffUnnormalized = diffUnnormalized.replace(
+                /<del>((<BR CLASS="OS-LINE-BREAK">)?<span[^>]+OS-LINE-NUMBER.+?)<\/del>/gi,
+                function(found,tag) {
+                    return tag.toLowerCase().replace(/> <\/span/gi, ">&nbsp;</span");
+                }
+            );
+
+            if (diffUnnormalized.substr(0, workaroundPrepend.length) === workaroundPrepend) {
+                diffUnnormalized = diffUnnormalized.substring(workaroundPrepend.length);
+            }
+
             var diff;
             if (this._diffDetectBrokenDiffHtml(diffUnnormalized)) {
                 diff = this._diffParagraphs(htmlOld, htmlNew, lineLength, firstLineNumber);

tests/karma/motions/diff.service.test.js

@@ -11,8 +11,9 @@ describe('linenumbering', function () {
         return '<span class="os-line-number line-number-' + no + '" data-line-number="' + no + '" contenteditable="false">&nbsp;</span>';
       };
 
-  beforeEach(inject(function (_diffService_) {
+  beforeEach(inject(function (_diffService_, _lineNumberingService_) {
     diffService = _diffService_;
+    lineNumberingService = _lineNumberingService_;
 
     baseHtml1 = '<p>' +
           noMarkup(1) + 'Line 1 ' + brMarkup(2) + 'Line 2 ' +
@@ -432,5 +433,53 @@ describe('linenumbering', function () {
 
       expect(diff).toBe("<p>...so frißt er Euch alle mit Haut und Haar<ins> und Augen und Därme und alles</ins>.</p>");
     });
+
+    it('does not break when an insertion followes a beginning tag occuring twice', function () {
+      var before = "<P>...so frißt er Euch alle mit Haut und Haar.</P>\n<p>Test</p>",
+          after = "<p>Einfügung 1 ...so frißt er Euch alle mit Haut und Haar und Augen und Därme und alles.</p>\n<p>Test</p>";
+      var diff = diffService.diff(before, after);
+
+      expect(diff).toBe("<p><ins>Einfügung 1 </ins>...so frißt er Euch alle mit Haut und Haar<ins> und Augen und Därme und alles</ins>.</p>\n<p>Test</p>");
+    });
+  });
+
+  describe('ignoring line numbers', function () {
+    it('works despite line numbers, part 1', function () {
+      var before = "<P>...so frißt er Euch alle mit Haut und Haar.</P>",
+          after = "<p>...so frißt er Euch alle mit Haut und Haar und Augen und Därme und alles.</p>";
+      before = lineNumberingService.insertLineNumbers(before, 15, null, null, 2);
+      var diff = diffService.diff(before, after);
+
+      expect(diff).toBe("<p>" + noMarkup(2) + "...so frißt er " + brMarkup(3) + "Euch alle mit " + brMarkup(4) + "Haut und Haar<ins> und Augen und Därme und alles</ins>.</p>");
+    });
+
+    it('works with an inserted paragraph', function () {
+      var before = "<P>their grammar, their pronunciation and their most common words. Everyone realizes why a </P>",
+          after = "<P>their grammar, their pronunciation and their most common words. Everyone realizes why a</P>\n" +
+              "<P>NEW PARAGRAPH 2.</P>";
+
+      before = lineNumberingService.insertLineNumbers(before, 80, null, null, 2);
+      var diff = diffService.diff(before, after);
+      expect(diff).toBe("<p>" + noMarkup(2) + "their grammar, their pronunciation and their most common words. Everyone " + brMarkup(3) + "realizes why a</p>\n" +
+          "<p><ins>NEW PARAGRAPH 2.</ins></p>");
+    });
+
+    it('works with two inserted paragraphs', function () {
+      // Hint: If the last paragraph is a P again, the Diff still fails and falls back to paragraph-based diff
+      // This leaves room for future improvements
+      var before = "<P>their grammar, their pronunciation and their most common words. Everyone realizes why a </P>\n<div>Go on</div>",
+          after = "<P>their grammar, their pronunciation and their most common words. Everyone realizes why a</P>\n" +
+              "<P>NEW PARAGRAPH 1.</P>\n" +
+              "<P>NEW PARAGRAPH 2.</P>\n" +
+              "<div>Go on</div>";
+
+      before = lineNumberingService.insertLineNumbers(before, 80, null, null, 2);
+      var diff = diffService.diff(before, after);
+      expect(diff).toBe("<p>" + noMarkup(2) + "their grammar, their pronunciation and their most common words. Everyone " + brMarkup(3) + "realizes why a</p>\n" +
+          "<p><ins>NEW PARAGRAPH 1.</ins></p>\n" +
+          "<p><ins>NEW PARAGRAPH 2.</ins></p>\n" +
+          "<div>" + noMarkup(4) + "Go on</div>"
+      );
+    });
   });
 });