Merge pull request #3465 from CatoTH/Bugfix-More-Tolerant-Diff

Better handling of inserted paragraphs

CHANGELOG

@@ -38,7 +38,7 @@ Motions:
 - Bugfix: Several bugfixes regarding splitting list items in
   change recommendations [#3288].
 - Bugfix: Several bugfixes regarding diff version [#3407, #3408, #3410,
-  #3440, #3450].
+  #3440, #3450, #3465].
 - Added inline Editing for motion reason [#3361].
 - Added multiselect filter for motion comments [#3372].
 - Added support for pinning personal notes to the window [#3360].

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

@@ -626,6 +626,7 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
         this._normalizeHtmlForDiff = function (html) {
             // Convert all HTML tags to uppercase, but leave the values of attributes unchanged
             // All attributes and CSS class names  are sorted alphabetically
+            // If an attribute is empty, it is removed
             html = html.replace(/<(\/?[a-z]*)( [^>]*)?>/ig, function (html, tag, attributes) {
                 var tagNormalized = tag.toUpperCase();
                 if (attributes === undefined) {
@@ -641,11 +642,13 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
                             attrValue = match[5];
                         if (match[2] !== undefined) {
                             if (attrNormalized === ' CLASS') {
-                                attrValue = attrValue.split(' ').sort().join(' ');
+                                attrValue = attrValue.split(' ').sort().join(' ').replace(/^\s+/, '').replace(/\s+$/, '');
                             }
                             attrNormalized += "=" + match[4] + attrValue + match[4];
                         }
-                        attributesList.push(attrNormalized);
+                        if (attrValue !== '') {
+                            attributesList.push(attrNormalized);
+                        }
                     }
                 } while (match);
                 attributes = attributesList.sort().join('');
@@ -1114,6 +1117,24 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
             });
         };
 
+        this._addClassToLastNode = function (html, className) {
+            var node = document.createElement('div');
+            node.innerHTML = html;
+            var foundLast = false;
+            for (var i = node.childNodes.length - 1; i >= 0 && !foundLast; i--) {
+                if (node.childNodes[i].nodeType === ELEMENT_NODE) {
+                    var classes = [];
+                    if (node.childNodes[i].getAttribute("class")) {
+                        classes = node.childNodes[i].getAttribute("class").split(" ");
+                    }
+                    classes.push(className);
+                    node.childNodes[i].setAttribute("class", classes.sort().join(' ').replace(/^\s+/, '').replace(/\s+$/, ''));
+                    foundLast = true;
+                }
+            }
+            return node.innerHTML;
+        };
+
         /**
          * This function removes color-Attributes from the styles of this node or a descendant,
          * as they interfer with the green/red color in HTML and PDF
@@ -1174,6 +1195,22 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
             // to make sure this situation does not happen (and uses invisible pseudo-tags in case something goes wrong)
             var workaroundPrepend = "<DUMMY><PREPEND>";
 
+            // os-split-after should not be considered for detecting changes in paragraphs, so we strip it here
+            // and add it afterwards.
+            // We only do this for P for now, as for more complex types like UL/LI that tend to be nestend,
+            // information would get lost by this that we will need to recursively merge it again later on.
+            var oldIsSplitAfter = false,
+                newIsSplitAfter = false;
+            htmlOld = htmlOld.replace(/(\s*<p[^>]+class\s*=\s*["'][^"']*)os-split-after/gi, function(match, beginning) {
+                oldIsSplitAfter = true;
+                return beginning;
+            });
+            htmlNew = htmlNew.replace(/(\s*<p[^>]+class\s*=\s*["'][^"']*)os-split-after/gi, function(match, beginning) {
+                newIsSplitAfter = true;
+                return beginning;
+            });
+
+            // Performing the actual diff
             var str = this._diffString(workaroundPrepend + htmlOld, workaroundPrepend + htmlNew),
                 diffUnnormalized = str.replace(/^\s+/g, '').replace(/\s+$/g, '').replace(/ {2,}/g, ' ');
 
@@ -1202,6 +1239,20 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
                 }
             );
 
+            // Fixes HTML produced by the diff like this:
+            // from: <del></P></del><ins> Inserted Text</P>\n<P>More inserted text</P></ins>
+            // into: <ins> Inserted Text</ins></P>\n<P>More inserted text</ins></P>
+            diffUnnormalized = diffUnnormalized.replace(
+                /<del><\/p><\/del><ins>([\s\S]*?)<\/p><\/ins>/gim,
+                "<ins>$1</ins></p>"
+            );
+            diffUnnormalized = diffUnnormalized.replace(
+                /<ins>[\s\S]*?<\/ins>/gim,
+                function(match) {
+                    return match.replace(/(<\/p>\s*<p>)/gi, "</ins>$1<ins>");
+                }
+            );
+
             // If only a few characters of a word have changed, don't display this as a replacement of the whole word,
             // but only of these specific characters
             diffUnnormalized = diffUnnormalized.replace(/<del>([a-z0-9,_-]* ?)<\/del><ins>([a-z0-9,_-]* ?)<\/ins>/gi, function (found, oldText, newText) {
@@ -1280,6 +1331,10 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi
                 }
             }
 
+            if (oldIsSplitAfter || newIsSplitAfter) {
+                diff = this._addClassToLastNode(diff, "os-split-after");
+            }
+
             diffCache.put(cacheKey, diff);
             return diff;
         };

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

@@ -502,9 +502,21 @@ describe('linenumbering', function () {
       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>" +
+          expected = "<p>rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid<ins> Noch</ins></p>" +
-              "<P class=\"insert\">rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid Noch</P>" +
+              "<p><ins>Test 123</ins></p>";
-              "<P class=\"insert\">Test 123</P>";
+
+      var diff = diffService.diff(before, after);
+      expect(diff).toBe(expected);
+    });
+
+    it('handles insterted paragraphs (3)', function () {
+      // Hint: os-split-after should be moved from the first paragraph to the second one
+      var before = "<p class=\"os-split-after\"><span class=\"os-line-number line-number-1\" data-line-number=\"1\" contenteditable=\"false\">&nbsp;</span>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, </p>",
+          after = "<p>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum.</p>\n" +
+              "\n" +
+              "<p>Stet clita kasd gubergren, no sea takimata sanctus est.</p>",
+          expected = "<p><span class=\"line-number-1 os-line-number\" contenteditable=\"false\" data-line-number=\"1\">&nbsp;</span>Lorem ipsum dolor sit amet, consetetur sadipscing elitr,<ins> sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum.</ins></p>\n" +
+              "<p class=\"os-split-after\"><ins>Stet clita kasd gubergren, no sea takimata sanctus est.</ins></p>";
 
       var diff = diffService.diff(before, after);
       expect(diff).toBe(expected);