From 9f71afa6021939e99659f831a6e0b6bee2319cfa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Ho=CC=88=C3=9Fl?= Date: Wed, 8 Mar 2017 19:40:49 +0100 Subject: [PATCH 1/3] Fixes broken diff test cases + Bugfix for: TypeError: Failed to execute 'insertBefore' on 'Node' --- openslides/motions/static/js/motions/diff.js | 22 +++++++++++-- .../static/js/motions/linenumbering.js | 8 +++-- tests/karma/motions/diff.service.test.js | 32 +++++++++++++++---- .../motions/linenumbering.service.test.js | 7 ++++ 4 files changed, 58 insertions(+), 11 deletions(-) diff --git a/openslides/motions/static/js/motions/diff.js b/openslides/motions/static/js/motions/diff.js index 74e538663..8ad99cf73 100644 --- a/openslides/motions/static/js/motions/diff.js +++ b/openslides/motions/static/js/motions/diff.js @@ -901,7 +901,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 +914,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(), diff --git a/openslides/motions/static/js/motions/linenumbering.js b/openslides/motions/static/js/motions/linenumbering.js index 09e6dba5f..7c2aa4854 100644 --- a/openslides/motions/static/js/motions/linenumbering.js +++ b/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); - this._ignoreNextRegularLineNumber = true; + var lineNumberNode = this._createLineNumber(); + if (lineNumberNode) { + node.insertBefore(lineNumberNode, node.firstChild); + this._ignoreNextRegularLineNumber = true; + } } return node; } else if (this._isInlineElement(node)) { diff --git a/tests/karma/motions/diff.service.test.js b/tests/karma/motions/diff.service.test.js index cada4314b..d338c23f6 100644 --- a/tests/karma/motions/diff.service.test.js +++ b/tests/karma/motions/diff.service.test.js @@ -326,12 +326,28 @@ describe('linenumbering', function () { expect(diff).toBe('The brown spotted fox jumleaped over the rolling log.'); }); - it('merges multiple inserts and deletes', function () { + it('too many changes result in separate paragraphs', 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 Test2 Test3 Test4 Test5 Test9

Test1 Test6 Test7 Test8 Test9

'); + }); + + 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 Test2 Test3 Test4 Test5 Test6 Test7 Test8 Test9'); + expect(diff).toBe('Test1 Test2 Test3 Test4 Test5 Test9Test1 Test6 Test7 Test8 Test9'); + }); + + 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 Test2 Test3 Test4 Test5 Test6 Test7 Test8 Test9'); }); it('detects insertions and deletions in a word (1)', function () { @@ -362,12 +378,16 @@ describe('linenumbering', function () { var before = "

liebliche Stimme, aber deine Stimme ist rauh; du bist der Wolf.' Da gieng der

", after = "

liebliche Stimme, aber deine Stimme ist rauh; du bist der Wolf.'

\ \ -

Der Wolf hatte danach richtig schlechte laune, trank eine Flasche Rum,

\ +

Der Wolf hatte danach richtig schlechte laune, trank eine Flasche Rum,

\ \ -

machte eine Weltreise und kam danach wieder um die Ziegen zu fressen. Da ging der

"; - var diff = diffService.diff(before, after); +

machte eine Weltreise und kam danach wieder um die Ziegen zu fressen. Da ging der

", + expected = "

liebliche Stimme, aber deine Stimme ist rauh; du bist der Wolf.' Da gieng der

" + + "

liebliche Stimme, aber deine Stimme ist rauh; du bist der Wolf.'

" + + "

Der Wolf hatte danach richtig schlechte laune, trank eine Flasche Rum,

" + + "

machte eine Weltreise und kam danach wieder um die Ziegen zu fressen. Da ging der

"; - expect(diff).toBe("

liebliche Stimme, aber deine Stimme ist rauh; du bist der Wolf.' Wolf.'

Der Wolf hatte danach richtig schlechte laune, trank eine Flasche Rum,

machte eine Weltreise und kam danach wieder um die Ziegen zu fressen. Da gieng der

"); + var diff = diffService.diff(before, after); + expect(diff).toBe(expected); }); it('handles completely deleted paragraphs', function () { diff --git a/tests/karma/motions/linenumbering.service.test.js b/tests/karma/motions/linenumbering.service.test.js index 7b13ab966..6ea7e18b8 100644 --- a/tests/karma/motions/linenumbering.service.test.js +++ b/tests/karma/motions/linenumbering.service.test.js @@ -241,6 +241,13 @@ describe('linenumbering', function () { expect(outHtml).toBe('

' + noMarkup(1) + 'Duis autem vel eum iriure dolor in hendrerit in vulputate velit esse molestie ' + brMarkup(2) + 'consequat, vel illum dolore eu feugiat nulla facilisis at vero eros et accumsan ' + brMarkup(3) + 'et iusto odio.

'); expect(lineNumberingService.stripLineNumbers(outHtml)).toBe(inHtml); }); + + it('does not fail in a weird case', function () { + var inHtml = "seid Noch

Test 123

"; + var outHtml = lineNumberingService.insertLineNumbers(inHtml, 80); + expect(outHtml).toBe(noMarkup(1) + 'seid Noch

' + noMarkup(2) + 'Test 123

'); + expect(lineNumberingService.stripLineNumbers(outHtml)).toBe(inHtml); + }); }); describe('line numbering in regard to the inline diff', function() { From a6de228f564e1237e034edfcea73feb00b7efd34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Ho=CC=88=C3=9Fl?= Date: Wed, 8 Mar 2017 20:35:15 +0100 Subject: [PATCH 2/3] Prevent broken HTML like Test

 --- openslides/motions/static/js/motions/diff.js | 18 +++++++++++++++++- tests/karma/motions/diff.service.test.js | 13 +++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/openslides/motions/static/js/motions/diff.js b/openslides/motions/static/js/motions/diff.js index 8ad99cf73..1b1ecd64d 100644 --- a/openslides/motions/static/js/motions/diff.js +++ b/openslides/motions/static/js/motions/diff.js @@ -890,6 +890,22 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi return true; } + // If other HTML tags are contained within INS/DEL (e.g. "Test

"), let's better be cautious + // The "!!(found=...)"-construction is only used to make jshint happy :) + var findDel = /(.*?)<\/del>/gi, + findIns = /(.*?)<\/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) { @@ -1010,7 +1026,7 @@ angular.module('OpenSlidesApp.motions.diff', ['OpenSlidesApp.motions.lineNumberi return out; }); - + var diff; if (this._diffDetectBrokenDiffHtml(diffUnnormalized)) { diff = this._diffParagraphs(htmlOld, htmlNew, lineLength, firstLineNumber); diff --git a/tests/karma/motions/diff.service.test.js b/tests/karma/motions/diff.service.test.js index d338c23f6..404681cd5 100644 --- a/tests/karma/motions/diff.service.test.js +++ b/tests/karma/motions/diff.service.test.js @@ -390,6 +390,19 @@ describe('linenumbering', function () { expect(diff).toBe(expected); }); + it('handles inserted paragraphs (2)', function () { + // Specifically, Noch

should not be enclosed by ..., as Noch

would be seriously broken + var before = "

rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid

", + after = "

rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid Noch

" + + "

Test 123

", + expected = "

rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid

" + + "

rief sie alle sieben herbei und sprach 'liebe Kinder, ich will hinaus in den Wald, seid Noch

" + + "

Test 123

"; + + var diff = diffService.diff(before, after); + expect(diff).toBe(expected); + }); + it('handles completely deleted paragraphs', function () { var before = "

Ihr könnt ohne Sorge fortgehen.'Da meckerte die Alte und machte sich getrost auf den Weg.

", after = ""; From c3bd85e5eeaffd875269a6051987bd5fa2c5de39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Ho=CC=88=C3=9Fl?= Date: Wed, 8 Mar 2017 21:29:34 +0100 Subject: [PATCH 3/3] Diff-Bugfix: don't repeat unchanged words --- openslides/motions/static/js/motions/diff.js | 33 +++++++++++++++----- tests/karma/motions/diff.service.test.js | 18 ++++++++++- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/openslides/motions/static/js/motions/diff.js b/openslides/motions/static/js/motions/diff.js index 1b1ecd64d..fce756f35 100644 --- a/openslides/motions/static/js/motions/diff.js +++ b/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); - arr = splitArrayEntries(arr, '>', false); - arr = splitArrayEntries(arr, " ", false); - arr = splitArrayEntries(arr, "\n", false); + var splitArrayEntriesSplitSeparator = function (arr, by) { + var newArr = []; + for (var i = 0; i < arr.length; i++) { + 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; }; diff --git a/tests/karma/motions/diff.service.test.js b/tests/karma/motions/diff.service.test.js index 404681cd5..63c2698ea 100644 --- a/tests/karma/motions/diff.service.test.js +++ b/tests/karma/motions/diff.service.test.js @@ -347,7 +347,7 @@ describe('linenumbering', function () { 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 Test2 Test3 Test4 Test5 Test6 Test7 Test8 Test9'); + expect(diff).toBe('Some additional text to circumvent the threshold Test1 Test2 Test3 Test4 Test5Test6 Test7 Test8 Test9'); }); it('detects insertions and deletions in a word (1)', function () { @@ -416,5 +416,21 @@ describe('linenumbering', function () { var diff = diffService.diff(before, after); expect(diff).toBe('

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

(hier: Missbrauch von bewusstseinsverändernde Mittel - daher Zensiert)

'); }); + + it('does not repeat the last word (1)', function () { + var before = "

sem. Nulla consequat massa quis enim. 

", + after = "

sem. Nulla consequat massa quis enim. TEST
\nTEST

"; + var diff = diffService.diff(before, after); + + expect(diff).toBe('

sem. Nulla consequat massa quis enim. TEST
' + "\n" + "TEST

"); + }); + + it('does not repeat the last word (2)', function () { + var before = "

...so frißt er Euch alle mit Haut und Haar.

", + after = "

...so frißt er Euch alle mit Haut und Haar und Augen und Därme und alles.

"; + var diff = diffService.diff(before, after); + + expect(diff).toBe("

...so frißt er Euch alle mit Haut und Haar und Augen und Därme und alles.

"); + }); }); });