Merge pull request #3072 from CatoTH/fix-diff-test-cases

Fixes broken diff test cases

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

@@ -760,17 +760,17 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
         };
 
         this._tokenizeHtml = function (str) {
-            var splitArrayEntries = function (arr, by, prepend) {
+            var splitArrayEntriesEmbedSeparator = function (arr, by, prepend) {
                 var newArr = [];
                 for (var i = 0; i < arr.length; i++) {
-                    if (arr[i][0] == '<' && (by == " " || by == "\n")) {
+                    if (arr[i][0] === '<' && (by === " " || by === "\n")) {
                         // Don't split HTML tags
                         newArr.push(arr[i]);
                         continue;
                     }
 
                     var parts = arr[i].split(by);
-                    if (parts.length == 1) {
+                    if (parts.length === 1) {
                         newArr.push(arr[i]);
                     } else {
                         var j;
@@ -793,10 +793,29 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
                 }
                 return newArr;
             };
-            var arr = splitArrayEntries([str], '<', true);
+            var splitArrayEntriesSplitSeparator = function (arr, by) {
-            arr = splitArrayEntries(arr, '>', false);
+                var newArr = [];
-            arr = splitArrayEntries(arr, " ", false);
+                for (var i = 0; i < arr.length; i++) {
-            arr = splitArrayEntries(arr, "\n", false);
+                    if (arr[i][0] === '<') {
+                        newArr.push(arr[i]);
+                        continue;
+                    }
+                    var parts = arr[i].split(by);
+                    for (var j = 0; j < parts.length; j++) {
+                        if (j > 0) {
+                            newArr.push(by);
+                        }
+                        newArr.push(parts[j]);
+                    }
+                }
+                return newArr;
+            };
+            var arr = splitArrayEntriesEmbedSeparator([str], '<', true);
+            arr = splitArrayEntriesEmbedSeparator(arr, '>', false);
+            arr = splitArrayEntriesSplitSeparator(arr, " ");
+            arr = splitArrayEntriesSplitSeparator(arr, ".");
+            arr = splitArrayEntriesEmbedSeparator(arr, "\n", false);
+
             return arr;
         };
 
@@ -890,6 +909,22 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
                 return true;
             }
 
+            // If other HTML tags are contained within INS/DEL (e.g. "<ins>Test</p></ins>"), let's better be cautious
+            // The "!!(found=...)"-construction is only used to make jshint happy :)
+            var findDel = /<del>(.*?)<\/del>/gi,
+                findIns = /<ins>(.*?)<\/ins>/gi,
+                found;
+            while (!!(found = findDel.exec(html))) {
+                if (found[1].match(/<[^>]*>/)) {
+                    return true;
+                }
+            }
+            while (!!(found = findIns.exec(html))) {
+                if (found[1].match(/<[^>]*>/)) {
+                    return true;
+                }
+            }
+
             // If too much of the text is changed, it's better to separate the old from new new version,
             // otherwise the result looks strange
             if (this._calcChangeRatio(html) > 0.66) {
@@ -901,7 +936,7 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
         };
 
         this._diffParagraphs = function(oldText, newText, lineLength, firstLineNumber) {
-            var oldTextWithBreaks, newTextWithBreaks;
+            var oldTextWithBreaks, newTextWithBreaks, currChild;
 
             if (lineLength !== undefined) {
                 oldTextWithBreaks = lineNumberingService.insertLineNumbersNode(oldText, lineLength, null, firstLineNumber);
@@ -914,10 +949,26 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
             }
 
             for (var i = 0; i < oldTextWithBreaks.childNodes.length; i++) {
-                this.addCSSClass(oldTextWithBreaks.childNodes[i], 'delete');
+                currChild = oldTextWithBreaks.childNodes[i];
+                if (currChild.nodeType === TEXT_NODE) {
+                    var wrapDel = document.createElement('del');
+                    oldTextWithBreaks.insertBefore(wrapDel, currChild);
+                    oldTextWithBreaks.removeChild(currChild);
+                    wrapDel.appendChild(currChild);
+                } else {
+                    this.addCSSClass(currChild, 'delete');
+                }
             }
             for (i = 0; i < newTextWithBreaks.childNodes.length; i++) {
-                this.addCSSClass(newTextWithBreaks.childNodes[i], 'insert');
+                currChild = newTextWithBreaks.childNodes[i];
+                if (currChild.nodeType === TEXT_NODE) {
+                    var wrapIns = document.createElement('ins');
+                    newTextWithBreaks.insertBefore(wrapIns, currChild);
+                    newTextWithBreaks.removeChild(currChild);
+                    wrapIns.appendChild(currChild);
+                } else {
+                    this.addCSSClass(currChild, 'insert');
+                }
             }
 
             var mergedFragment = document.createDocumentFragment(),
@@ -994,7 +1045,7 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
 
                 return out;
             });
-            
+
             var diff;
             if (this._diffDetectBrokenDiffHtml(diffUnnormalized)) {
                 diff = this._diffParagraphs(htmlOld, htmlNew, lineLength, firstLineNumber);

openslides/motions/static/js/motions/linenumbering.js

@@ -343,6 +343,7 @@ angular.module('OpenSlidesApp.motions.lineNumbering', [])
 
             this._currentInlineOffset = 0;
             this._prependLineNumberToFirstText = true;
+            this._ignoreNextRegularLineNumber = false;
 
             return node;
         };
@@ -353,8 +354,11 @@ angular.module('OpenSlidesApp.motions.lineNumbering', [])
             }
             if (this._isIgnoredByLineNumbering(node)) {
                 if (this._currentInlineOffset === 0) {
-                    node.insertBefore(this._createLineNumber(), node.firstChild);
+                    var lineNumberNode = this._createLineNumber();
-                    this._ignoreNextRegularLineNumber = true;
+                    if (lineNumberNode) {
+                        node.insertBefore(lineNumberNode, node.firstChild);
+                        this._ignoreNextRegularLineNumber = true;
+                    }
                 }
                 return node;
             } else if (this._isInlineElement(node)) {

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

@@ -326,12 +326,28 @@ describe('linenumbering', function () {
       expect(diff).toBe('The <strong>brown</strong> spotted fox <del>jum</del><ins>lea</ins>ped over the rolling log.');
     });
 
-    it('merges multiple inserts and deletes', function () {
+    it('too many changes result in separate paragraphs', function () {
+      var before = "<p>Test1 Test2 Test3 Test4 Test5 Test9</p>",
+          after = "<p>Test1 Test6 Test7 Test8 Test9</p>";
+      var diff = diffService.diff(before, after);
+
+      expect(diff).toBe('<P class="delete">Test1 Test2 Test3 Test4 Test5 Test9</P><P class="insert">Test1 Test6 Test7 Test8 Test9</P>');
+    });
+
+    it('too many changes result in separate paragraphs - special case with un-wrapped text', function () {
       var before = "Test1 Test2 Test3 Test4 Test5 Test9",
           after = "Test1 Test6 Test7 Test8 Test9";
       var diff = diffService.diff(before, after);
 
-      expect(diff).toBe('Test1 <del>Test2 Test3 Test4 Test5 </del><ins>Test6 Test7 Test8 </ins>Test9');
+      expect(diff).toBe('<DEL>Test1 Test2 Test3 Test4 Test5 Test9</DEL><INS>Test1 Test6 Test7 Test8 Test9</INS>');
+    });
+
+    it('merges multiple inserts and deletes', function () {
+      var before = "Some additional text to circumvent the threshold Test1 Test2 Test3 Test4 Test5 Test9",
+          after = "Some additional text to circumvent the threshold Test1 Test6 Test7 Test8 Test9";
+      var diff = diffService.diff(before, after);
+
+      expect(diff).toBe('Some additional text to circumvent the threshold Test1 <del>Test2 Test3 Test4 Test5</del><ins>Test6 Test7 Test8</ins> Test9');
     });
 
     it('detects insertions and deletions in a word (1)', function () {
@@ -362,12 +378,29 @@ describe('linenumbering', function () {
       var before = "<P>liebliche Stimme, aber deine Stimme ist rauh; du bist der Wolf.' Da gieng der </P>",
           after = "<p>liebliche Stimme, aber deine Stimme ist rauh; du bist der Wolf.'</p>\
 \
-<p>Der Wolf hatte danach richtig schlechte laune, trank eine Flasche Rum,</p>\
+<p>Der Wolf hatte danach richtig schlechte laune, trank eine Flasche Rum,</p>\
 \
-<p>machte eine Weltreise und kam danach wieder um die Ziegen zu fressen. Da ging der</p>";
+<p>machte eine Weltreise und kam danach wieder um die Ziegen zu fressen. Da ging der</p>",
-      var diff = diffService.diff(before, after);
+      expected = "<P class=\"delete\">liebliche Stimme, aber deine Stimme ist rauh; du bist der Wolf.' Da gieng der </P>" +
+          "<P class=\"insert\">liebliche Stimme, aber deine Stimme ist rauh; du bist der Wolf.'</P>" +
+          "<P class=\"insert\">Der Wolf hatte danach richtig schlechte laune, trank eine Flasche Rum,</P>" +
+          "<P class=\"insert\">machte eine Weltreise und kam danach wieder um die Ziegen zu fressen. Da ging der</P>";
 
-      expect(diff).toBe("<p>liebliche Stimme, aber deine Stimme ist rauh; du bist der <del>Wolf.' </del><ins>Wolf.'</ins></p><p><ins>Der Wolf hatte danach richtig schlechte laune, trank eine Flasche Rum,</ins></p><p><ins>machte eine Weltreise und kam danach wieder um die Ziegen zu fressen. </ins>Da gi<del>e</del>ng der</p>");
+      var diff = diffService.diff(before, after);
+      expect(diff).toBe(expected);
+    });
+
+    it('handles inserted paragraphs (2)', function () {
+      // Specifically, Noch</p> should not be enclosed by <ins>...</ins>, as <ins>Noch </p></ins> would be seriously broken
+      var before = "<P>rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid </P>",
+          after = "<p>rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid Noch</p>" +
+              "<p>Test 123</p>",
+          expected = "<P class=\"delete\">rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid </P>" +
+              "<P class=\"insert\">rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid Noch</P>" +
+              "<P class=\"insert\">Test 123</P>";
+
+      var diff = diffService.diff(before, after);
+      expect(diff).toBe(expected);
     });
 
     it('handles completely deleted paragraphs', function () {
@@ -383,5 +416,21 @@ describe('linenumbering', function () {
         var diff = diffService.diff(before, after);
         expect(diff).toBe('<P class="delete">Dann kam er zurück, klopfte an die Hausthür und rief \'macht auf, ihr lieben Kinder, eure Mutter ist da und hat jedem von Euch etwas mitgebarcht.\' Aber der Wolf hatte seine schwarze Pfote in das Fenster gelegt, das sahen die Kinder und riefen</P><P class="insert">(hier: Missbrauch von bewusstseinsverändernde Mittel - daher Zensiert)</P>');
     });
+
+    it('does not repeat the last word (1)', function () {
+      var before = "<P>sem. Nulla consequat massa quis enim. </P>",
+          after = "<p>sem. Nulla consequat massa quis enim. TEST<br>\nTEST</p>";
+      var diff = diffService.diff(before, after);
+
+      expect(diff).toBe('<p>sem. Nulla consequat massa quis enim.<ins> TEST<br>' + "\n" + "TEST</ins></p>");
+    });
+
+    it('does not repeat the last word (2)', 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>";
+      var diff = diffService.diff(before, after);
+
+      expect(diff).toBe("<p>...so frißt er Euch alle mit Haut und Haar<ins> und Augen und Därme und alles</ins>.</p>");
+    });
   });
 });

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

@@ -241,6 +241,13 @@ describe('linenumbering', function () {
       expect(outHtml).toBe('<p>' + noMarkup(1) + '<span>Duis autem vel eum iriure dolor in hendrerit in vulputate velit esse molestie ' + brMarkup(2) + '<strong>consequat, vel illum dolore eu feugiat nulla facilisis at vero eros et accumsan ' + brMarkup(3) + 'et iusto odio</strong>.</span></p>');
       expect(lineNumberingService.stripLineNumbers(outHtml)).toBe(inHtml);
     });
+
+    it('does not fail in a weird case', function () {
+      var inHtml = "<ins>seid Noch</ins><p></p><p><ins>Test 123</ins></p>";
+      var outHtml = lineNumberingService.insertLineNumbers(inHtml, 80);
+      expect(outHtml).toBe(noMarkup(1) + '<ins>seid Noch</ins><p></p><p>' + noMarkup(2) + '<ins>Test 123</ins></p>');
+      expect(lineNumberingService.stripLineNumbers(outHtml)).toBe(inHtml);
+    });
   });
 
   describe('line numbering in regard to the inline diff', function() {