diff --git a/client/src/app/core/ui-services/diff.service.ts b/client/src/app/core/ui-services/diff.service.ts index 2dfcee13a..3772dbe85 100644 --- a/client/src/app/core/ui-services/diff.service.ts +++ b/client/src/app/core/ui-services/diff.service.ts @@ -470,15 +470,9 @@ export class DiffService { * @return {DocumentFragment} */ public htmlToFragment(html: string): DocumentFragment { - const fragment = document.createDocumentFragment(), - div = document.createElement('DIV'); - div.innerHTML = html; - while (div.childElementCount) { - const child = div.childNodes[0]; - div.removeChild(child); - fragment.appendChild(child); - } - return fragment; + const template = document.createElement('template'); + template.innerHTML = html; + return template.content; } /** diff --git a/client/src/app/core/ui-services/linenumbering.service.spec.ts b/client/src/app/core/ui-services/linenumbering.service.spec.ts index e1049c625..2cd4bc69a 100644 --- a/client/src/app/core/ui-services/linenumbering.service.spec.ts +++ b/client/src/app/core/ui-services/linenumbering.service.spec.ts @@ -6,20 +6,20 @@ describe('LinenumberingService', () => { const brMarkup = (no: number): string => { return ( '
' + - ' ' + '"> ' ); }, noMarkup = (no: number): string => { return ( - ' ' + '"> ' ); }, longstr = (length: number): string => { diff --git a/client/src/app/core/ui-services/linenumbering.service.ts b/client/src/app/core/ui-services/linenumbering.service.ts index 0e0f68ee3..0a8936be3 100644 --- a/client/src/app/core/ui-services/linenumbering.service.ts +++ b/client/src/app/core/ui-services/linenumbering.service.ts @@ -137,6 +137,13 @@ export class LinenumberingService { // text with inline diff annotations and get the same line numbering as with the original text (when set to false) private ignoreInsertedText = false; + // A precompiled regular expression that looks for line number nodes in a HTML string + private getLineNumberRangeRegexp = RegExp(/]+data\-line\-number=\"(\d+)\"/, 'g'); + + // .setAttribute and .innerHTML seem to be really slow, so we try to avoid them for static attributes / content + // by creating a static template, cloning it and only set the dynamic attributes each time + private lineNumberToClone: Element = null; + /** * Creates a hash of a given string. This is not meant to be specifically secure, but rather as quick as possible. * @@ -244,15 +251,9 @@ export class LinenumberingService { * @return {DocumentFragment} */ private htmlToFragment(html: string): DocumentFragment { - const fragment: DocumentFragment = document.createDocumentFragment(), - div = document.createElement('DIV'); - div.innerHTML = html; - while (div.childElementCount) { - const child = div.childNodes[0]; - div.removeChild(child); - fragment.appendChild(child); - } - return fragment; + const template = document.createElement('template'); + template.innerHTML = html; + return template.content; } /** @@ -377,27 +378,19 @@ export class LinenumberingService { * @returns {LineNumberRange} */ public getLineNumberRange(html: string): LineNumberRange { - const cacheKey = this.djb2hash(html); - let range = this.lineNumberCache.get(cacheKey); - if (!range) { - const fragment = this.htmlToFragment(html); - range = { - from: null, - to: null - }; - const lineNumbers = fragment.querySelectorAll('.os-line-number'); - for (let i = 0; i < lineNumbers.length; i++) { - const node = lineNumbers.item(i); - const number = parseInt(node.getAttribute('data-line-number'), 10); - if (range.from === null || number < range.from) { - range.from = number; - } - if (range.to === null || number + 1 > range.to) { - range.to = number + 1; - } + const range = { + from: null, + to: null + }; + + let foundLineNumber; + while ((foundLineNumber = this.getLineNumberRangeRegexp.exec(html)) !== null) { + if (range.from === null) { + range.from = parseInt(foundLineNumber[1], 10); } + range.to = parseInt(foundLineNumber[1], 10) + 1; } - this.lineNumberCache.put(cacheKey, range); + return range; } @@ -550,13 +543,18 @@ export class LinenumberingService { this.ignoreNextRegularLineNumber = false; return; } - const node = document.createElement('span'); + + if (this.lineNumberToClone === null) { + this.lineNumberToClone = document.createElement('span'); + this.lineNumberToClone.setAttribute('contenteditable', 'false'); + this.lineNumberToClone.innerHTML = ' '; // Prevent ckeditor from stripping out empty span's + } + const node = this.lineNumberToClone.cloneNode(true) as Element; const lineNumber = this.currentLineNumber; this.currentLineNumber++; node.setAttribute('class', 'os-line-number line-number-' + lineNumber); node.setAttribute('data-line-number', lineNumber + ''); - node.setAttribute('contenteditable', 'false'); - node.innerHTML = ' '; // Prevent ckeditor from stripping out empty span's + return node; } diff --git a/client/src/app/site/motions/modules/motion-detail/components/motion-detail/motion-detail.component.html b/client/src/app/site/motions/modules/motion-detail/components/motion-detail/motion-detail.component.html index 249a901d4..68811b99a 100644 --- a/client/src/app/site/motions/modules/motion-detail/components/motion-detail/motion-detail.component.html +++ b/client/src/app/site/motions/modules/motion-detail/components/motion-detail/motion-detail.component.html @@ -538,7 +538,7 @@ mat-button [matMenuTriggerFor]="changeRecoMenu" *ngIf=" - motion && ((allChangingObjects && allChangingObjects.length) || motion.modified_final_version) + motion && (hasChangingObjects() || motion.modified_final_version) " > rate_review @@ -1009,7 +1009,7 @@ mat-menu-item (click)="setChangeRecoMode(ChangeRecoMode.Changed)" [ngClass]="{ selected: crMode === ChangeRecoMode.Changed }" - *ngIf="allChangingObjects && allChangingObjects.length" + *ngIf="hasChangingObjects()" > {{ 'Changed version' | translate }} @@ -1017,7 +1017,7 @@ mat-menu-item (click)="setChangeRecoMode(ChangeRecoMode.Diff)" [ngClass]="{ selected: crMode === ChangeRecoMode.Diff }" - *ngIf="allChangingObjects && allChangingObjects.length" + *ngIf="hasChangingObjects()" > {{ 'Diff version' | translate }} @@ -1025,7 +1025,7 @@ mat-menu-item (click)="setChangeRecoMode(ChangeRecoMode.Final)" [ngClass]="{ selected: crMode === ChangeRecoMode.Final }" - *ngIf="motion && !motion.isParagraphBasedAmendment() && allChangingObjects && allChangingObjects.length" + *ngIf="motion && !motion.isParagraphBasedAmendment() && hasChangingObjects()" > {{ 'Final version' | translate }} 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 c325c7fbb..d41415a0d 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 @@ -228,17 +228,17 @@ export class MotionDetailComponent extends BaseViewComponent implements OnInit, */ public amendmentChangeRecos: { [amendmentId: string]: ViewMotionChangeRecommendation[] } = {}; + /** + * All change recommendations AND amendments, sorted by line number. + */ + private sortedChangingObjects: ViewUnifiedChange[] = null; + /** * The observables for the `amendmentChangeRecos` field above. * Necessary to track which amendments' change recommendations we have already subscribed to. */ public amendmentChangeRecoSubscriptions: { [amendmentId: string]: Subscription } = {}; - /** - * All change recommendations AND amendments, sorted by line number. - */ - public allChangingObjects: ViewUnifiedChange[] = []; - /** * preload the next motion for direct navigation */ @@ -526,7 +526,7 @@ export class MotionDetailComponent extends BaseViewComponent implements OnInit, .subscribe(enabled => (this.amendmentsEnabled = enabled)); this.configService.get('motions_line_length').subscribe(lineLength => { this.lineLength = lineLength; - this.recalcUnifiedChanges(); + this.sortedChangingObjects = null; }); this.configService .get('motions_default_line_numbering') @@ -616,15 +616,37 @@ export class MotionDetailComponent extends BaseViewComponent implements OnInit, .getChangeRecosOfMotionObservable(amendment.id) .subscribe((changeRecos: ViewMotionChangeRecommendation[]): void => { this.amendmentChangeRecos[amendment.id] = changeRecos; - this.recalcUnifiedChanges(); + this.sortedChangingObjects = null; + this.cd.markForCheck(); }); } }); } + /** + * Retrieves + */ + public getAllChangingObjectsSorted(): ViewUnifiedChange[] { + if (this.sortedChangingObjects === null) { + this.recalcUnifiedChanges(); + } + return this.sortedChangingObjects; + } + + public hasChangingObjects(): boolean { + if (this.sortedChangingObjects !== null) { + return this.sortedChangingObjects.length > 0; + } + + return ( + (this.changeRecommendations && this.changeRecommendations.length > 0) || + (this.amendments && this.amendments.filter(amendment => amendment.isParagraphBasedAmendment()).length > 0) + ); + } + /** * Merges amendments and change recommendations and sorts them by the line numbers. - * Called each time one of these arrays changes. + * Called each time one of these arrays changes (if it is requested using getAllChangingObjectsSorted()). * * TODO: 1. Having logic outside of a service is bad practice * 2. Manipulating class parameters without an subscription should @@ -638,10 +660,10 @@ export class MotionDetailComponent extends BaseViewComponent implements OnInit, return; } - this.allChangingObjects = []; + this.sortedChangingObjects = []; if (this.changeRecommendations) { this.changeRecommendations.forEach((change: ViewMotionChangeRecommendation): void => { - this.allChangingObjects.push(change); + this.sortedChangingObjects.push(change); }); } if (this.amendments) { @@ -655,11 +677,11 @@ export class MotionDetailComponent extends BaseViewComponent implements OnInit, this.repo .getAmendmentAmendedParagraphs(amendment, this.lineLength, toApplyChanges) .forEach((change: ViewUnifiedChange): void => { - this.allChangingObjects.push(change); + this.sortedChangingObjects.push(change); }); }); } - this.allChangingObjects.sort((a: ViewUnifiedChange, b: ViewUnifiedChange) => { + this.sortedChangingObjects.sort((a: ViewUnifiedChange, b: ViewUnifiedChange) => { if (a.getLineFrom() < b.getLineFrom()) { return -1; } else if (a.getLineFrom() > b.getLineFrom()) { @@ -669,10 +691,10 @@ export class MotionDetailComponent extends BaseViewComponent implements OnInit, } }); - // When "diff" is the default view mode, the crMode might have been set - // before the motion and allChangingObjects have been loaded. As the availability of "diff" depends on - // allChangingObjects, we set "diff" first in this case (in the config-listener) and perform the actual - // check if "diff" is possible now. + // When "diff" is the default view mode, the crMode might have been set before the motion, + // amendments and change recommendations have been loaded. + // As the availability of "diff" depends on amendments and change recommendations, we set "diff" + // first in this case (in the config-listener) and perform the actual check if "diff" is possible now. // Test: "diff" as default view. Open a motion, create an amendment. "Original" should be set automatically. if (this.crMode) { this.crMode = this.determineCrMode(this.crMode); @@ -713,7 +735,8 @@ export class MotionDetailComponent extends BaseViewComponent implements OnInit, this.repo.amendmentsTo(motionId).subscribe((amendments: ViewMotion[]): void => { this.amendments = amendments; this.resetAmendmentChangeRecoListener(); - this.recalcUnifiedChanges(); + this.sortedChangingObjects = null; + this.cd.markForCheck(); }), this.repo .getRecommendationReferencingMotions(motionId) @@ -722,7 +745,8 @@ export class MotionDetailComponent extends BaseViewComponent implements OnInit, .getChangeRecosOfMotionObservable(motionId) .subscribe((recos: ViewMotionChangeRecommendation[]) => { this.changeRecommendations = recos; - this.recalcUnifiedChanges(); + this.sortedChangingObjects = null; + this.cd.markForCheck(); }) ); } else { @@ -947,8 +971,13 @@ export class MotionDetailComponent extends BaseViewComponent implements OnInit, * @returns formated motion texts */ public getFormattedTextPlain(): string { - // Prevent this.allChangingObjects to be reordered from within formatMotion - const changes: ViewUnifiedChange[] = Object.assign([], this.getAllTextChangingObjects()); + // Prevent this.sortedChangingObjects to be reordered from within formatMotion + let changes: ViewUnifiedChange[]; + if (this.crMode === ChangeRecoMode.Original) { + changes = []; + } else { + changes = Object.assign([], this.getAllTextChangingObjects()); + } const formatedText = this.repo.formatMotion( this.motion.id, this.crMode, @@ -1013,7 +1042,7 @@ export class MotionDetailComponent extends BaseViewComponent implements OnInit, } public getChangesForDiffMode(): ViewUnifiedChange[] { - return this.allChangingObjects.filter(change => { + return this.getAllChangingObjectsSorted().filter(change => { if (this.showAllAmendments) { return true; } else { @@ -1023,17 +1052,21 @@ export class MotionDetailComponent extends BaseViewComponent implements OnInit, } public getChangesForFinalMode(): ViewUnifiedChange[] { - return this.allChangingObjects.filter(change => { + return this.getAllChangingObjectsSorted().filter(change => { return change.showInFinalView(); }); } public getAllTextChangingObjects(): ViewUnifiedChange[] { - return this.allChangingObjects.filter((obj: ViewUnifiedChange) => !obj.isTitleChange()); + return this.getAllChangingObjectsSorted().filter((obj: ViewUnifiedChange) => !obj.isTitleChange()); } public getTitleChangingObject(): ViewUnifiedChange { - return this.allChangingObjects.find((obj: ViewUnifiedChange) => obj.isTitleChange()); + if (this.changeRecommendations) { + return this.changeRecommendations.find(change => change.isTitleChange()); + } else { + return null; + } } public getTitleWithChanges(): string { @@ -1549,10 +1582,10 @@ export class MotionDetailComponent extends BaseViewComponent implements OnInit, /** * Because without change recos you cannot escape the final version anymore */ - } else if (!this.allChangingObjects?.length) { + } else if (!this.hasChangingObjects()) { return ChangeRecoMode.Original; } - } else if (mode === ChangeRecoMode.Changed && !this.allChangingObjects?.length) { + } else if (mode === ChangeRecoMode.Changed && !this.hasChangingObjects()) { /** * Because without change recos you cannot escape the changed version view * You will not be able to automatically change to the Changed view after creating