From 6b1c72d52671188b4fa597b3dbe3ecad68bbabbd Mon Sep 17 00:00:00 2001 From: Sean Engelhardt Date: Fri, 11 Oct 2019 14:11:45 +0200 Subject: [PATCH] Fix format motions hidden change discrepancy Fixes an issue where the "formatMotion" function was working on the wrong array. This resulted in unexpected results of both PDF and CSV export Fixes as issue in the diff services that deleted replaced lines rather than overwriting them --- .../motions/motion-repository.service.ts | 54 +++++++-------- .../src/app/core/ui-services/diff.service.ts | 65 +++++++------------ .../motion-detail/motion-detail.component.ts | 9 ++- 3 files changed, 55 insertions(+), 73 deletions(-) diff --git a/client/src/app/core/repositories/motions/motion-repository.service.ts b/client/src/app/core/repositories/motions/motion-repository.service.ts index f5d432285..aaab70c6e 100644 --- a/client/src/app/core/repositories/motions/motion-repository.service.ts +++ b/client/src/app/core/repositories/motions/motion-repository.service.ts @@ -600,43 +600,37 @@ export class MotionRepositoryService extends BaseIsAgendaItemAndListOfSpeakersCo highlightLine ); case ChangeRecoMode.Diff: - let text = ''; - const changesToShow = changes.filter(change => { - return change.showInDiffView(); - }); - changesToShow.forEach((change: ViewUnifiedChange, idx: number) => { - if (idx === 0) { - text += this.diff.extractMotionLineRange( + const text = []; + const changesToShow = changes.filter(change => change.showInDiffView()); + + for (let i = 0; i < changesToShow.length; i++) { + text.push( + this.diff.extractMotionLineRange( targetMotion.text, { - from: 1, - to: change.getLineFrom() + from: i === 0 ? 1 : changesToShow[i - 1].getLineTo(), + to: changesToShow[i].getLineFrom() }, true, lineLength, highlightLine - ); - } else if (changes[idx - 1].getLineTo() < change.getLineFrom()) { - text += this.diff.extractMotionLineRange( - targetMotion.text, - { - from: changes[idx - 1].getLineTo(), - to: change.getLineFrom() - }, - true, - lineLength, - highlightLine - ); - } - text += this.diff.getChangeDiff(targetMotion.text, change, lineLength, highlightLine); - }); - text += this.diff.getTextRemainderAfterLastChange( - targetMotion.text, - changesToShow, - lineLength, - highlightLine + ) + ); + + text.push( + this.diff.getChangeDiff(targetMotion.text, changesToShow[i], lineLength, highlightLine) + ); + } + + text.push( + this.diff.getTextRemainderAfterLastChange( + targetMotion.text, + changesToShow, + lineLength, + highlightLine + ) ); - return text; + return text.join(''); case ChangeRecoMode.Final: const appliedChanges: ViewUnifiedChange[] = changes.filter(change => change.showInFinalView()); return this.diff.getTextWithChanges(targetMotion.text, appliedChanges, lineLength, highlightLine); diff --git a/client/src/app/core/ui-services/diff.service.ts b/client/src/app/core/ui-services/diff.service.ts index d632ca0e5..c2a658681 100644 --- a/client/src/app/core/ui-services/diff.service.ts +++ b/client/src/app/core/ui-services/diff.service.ts @@ -1333,7 +1333,6 @@ export class DiffService { } const fragment = this.htmlToFragment(htmlIn); - this.insertInternalLineMarkers(fragment); if (toLine === null) { const internalLineMarkers = fragment.querySelectorAll('OS-LINEBREAK'), @@ -1363,6 +1362,7 @@ export class DiffService { fromChildTraceAbs.shift(); const previousHtml = this.serializePartialDomToChild(fragment, fromChildTraceAbs, false); toChildTraceAbs.shift(); + const followingHtml = this.serializePartialDomFromChild(fragment, toChildTraceAbs, false); let currNode: Node = fromLineNode, @@ -1552,54 +1552,38 @@ export class DiffService { * @returns {Node[]} */ public replaceLinesMergeNodeArrays(nodes1: Node[], nodes2: Node[]): Node[] { - if (nodes1.length === 0) { - return nodes2; - } - if (nodes2.length === 0) { - return nodes1; + if (nodes1.length === 0 || nodes2.length === 0) { + return nodes1.length ? nodes1 : nodes2; } - const out = []; - for (let i = 0; i < nodes1.length - 1; i++) { - out.push(nodes1[i]); - } - - const lastNode = nodes1[nodes1.length - 1], - firstNode = nodes2[0]; + const out: Node[] = nodes1.slice(0, -1); + const lastNode: Node = nodes1[nodes1.length - 1]; + const firstNode: Node = nodes2[0]; if (lastNode.nodeType === TEXT_NODE && firstNode.nodeType === TEXT_NODE) { - const newTextNode = lastNode.ownerDocument.createTextNode(lastNode.nodeValue + firstNode.nodeValue); + const newTextNode: Text = lastNode.ownerDocument.createTextNode(lastNode.nodeValue + firstNode.nodeValue); out.push(newTextNode); } else if (lastNode.nodeName === firstNode.nodeName) { - const lastElement = lastNode, - newNode = lastNode.ownerDocument.createElement(lastNode.nodeName); - for (let i = 0; i < lastElement.attributes.length; i++) { - const attr = lastElement.attributes[i]; + const lastElement: Element = lastNode as Element; + const newNode: HTMLElement = lastNode.ownerDocument.createElement(lastNode.nodeName); + + for (const attr of Array.from(lastElement.attributes)) { newNode.setAttribute(attr.name, attr.value); } // Remove #text nodes inside of List elements (OL/UL), as they are confusing - let lastChildren, firstChildren; + let lastChildren: Node[]; + let firstChildren: Node[]; if (lastElement.nodeName === 'OL' || lastElement.nodeName === 'UL') { - lastChildren = []; - firstChildren = []; - for (let i = 0; i < firstNode.childNodes.length; i++) { - if (firstNode.childNodes[i].nodeType === ELEMENT_NODE) { - firstChildren.push(firstNode.childNodes[i]); - } - } - for (let i = 0; i < lastElement.childNodes.length; i++) { - if (lastElement.childNodes[i].nodeType === ELEMENT_NODE) { - lastChildren.push(lastElement.childNodes[i]); - } - } + lastChildren = Array.from(lastElement.childNodes).filter(child => child.nodeType === ELEMENT_NODE); + firstChildren = Array.from(firstNode.childNodes).filter(child => child.nodeType === ELEMENT_NODE); } else { - lastChildren = lastElement.childNodes; - firstChildren = firstNode.childNodes; + lastChildren = Array.from(lastElement.childNodes); + firstChildren = Array.from(firstNode.childNodes); } - const children = this.replaceLinesMergeNodeArrays(lastChildren, firstChildren); - for (let i = 0; i < children.length; i++) { - newNode.appendChild(children[i]); + const children = this.replaceLinesMergeNodeArrays(lastChildren, firstChildren) as Node[]; + for (const child of children) { + newNode.appendChild(child); } out.push(newNode); @@ -1612,11 +1596,7 @@ export class DiffService { } } - for (let i = 1; i < nodes2.length; i++) { - out.push(nodes2[i]); - } - - return out; + return out.concat(nodes2.slice(1, nodes2.length)); } /** @@ -1755,7 +1735,8 @@ export class DiffService { this.removeCSSClass(forgottenSplitClasses[i], 'os-split-after'); } - return this.serializeDom(mergedFragment, true); + const replacedHtml = this.serializeDom(mergedFragment, true); + return replacedHtml; } /** diff --git a/client/src/app/site/motions/modules/motion-detail/components/motion-detail/motion-detail.component.ts b/client/src/app/site/motions/modules/motion-detail/components/motion-detail/motion-detail.component.ts index 161a6515b..f562cd0ed 100644 --- a/client/src/app/site/motions/modules/motion-detail/components/motion-detail/motion-detail.component.ts +++ b/client/src/app/site/motions/modules/motion-detail/components/motion-detail/motion-detail.component.ts @@ -854,7 +854,14 @@ export class MotionDetailComponent extends BaseViewComponent implements OnInit, public getFormattedTextPlain(): string { // Prevent this.allChangingObjects to be reordered from within formatMotion const changes: ViewUnifiedChange[] = Object.assign([], this.getAllTextChangingObjects()); - return this.repo.formatMotion(this.motion.id, this.crMode, changes, this.lineLength, this.highlightedLine); + const formatedText = this.repo.formatMotion( + this.motion.id, + this.crMode, + changes, + this.lineLength, + this.highlightedLine + ); + return formatedText; } /**