Merge pull request #5514 from CatoTH/create-line-number-performance-tweak

Performance tweak for creating line numbers

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(),
+        const template = document.createElement('template');
-            div = document.createElement('DIV');
+        template.innerHTML = html;
-        div.innerHTML = html;
+        return template.content;
-        while (div.childElementCount) {
-            const child = div.childNodes[0];
-            div.removeChild(child);
-            fragment.appendChild(child);
-        }
-        return fragment;
     }
 
     /**

client/src/app/core/ui-services/linenumbering.service.spec.ts

@@ -6,20 +6,20 @@ describe('LinenumberingService', () => {
     const brMarkup = (no: number): string => {
             return (
                 '<br class="os-line-break">' +
-                '<span class="os-line-number line-number-' +
+                '<span contenteditable="false" class="os-line-number line-number-' +
                 no +
                 '" data-line-number="' +
                 no +
-                '" contenteditable="false">&nbsp;</span>'
+                '">&nbsp;</span>'
             );
         },
         noMarkup = (no: number): string => {
             return (
-                '<span class="os-line-number line-number-' +
+                '<span contenteditable="false" class="os-line-number line-number-' +
                 no +
                 '" data-line-number="' +
                 no +
-                '" contenteditable="false">&nbsp;</span>'
+                '">&nbsp;</span>'
             );
         },
         longstr = (length: number): string => {

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(/<span[^>]+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(),
+        const template = document.createElement('template');
-            div = document.createElement('DIV');
+        template.innerHTML = html;
-        div.innerHTML = html;
+        return template.content;
-        while (div.childElementCount) {
-            const child = div.childNodes[0];
-            div.removeChild(child);
-            fragment.appendChild(child);
-        }
-        return fragment;
     }
 
     /**
@@ -377,27 +378,19 @@ export class LinenumberingService {
      * @returns {LineNumberRange}
      */
     public getLineNumberRange(html: string): LineNumberRange {
-        const cacheKey = this.djb2hash(html);
+        const range = {
-        let range = this.lineNumberCache.get(cacheKey);
+            from: null,
-        if (!range) {
+            to: null
-            const fragment = this.htmlToFragment(html);
+        };
-            range = {
+
-                from: null,
+        let foundLineNumber;
-                to: null
+        while ((foundLineNumber = this.getLineNumberRangeRegexp.exec(html)) !== null) {
-            };
+            if (range.from === null) {
-            const lineNumbers = fragment.querySelectorAll('.os-line-number');
+                range.from = parseInt(foundLineNumber[1], 10);
-            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;
-                }
             }
+            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 = '&nbsp;'; // 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 = '&nbsp;'; // Prevent ckeditor from stripping out empty span's
         return node;
     }
 

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)
                     "
                 >
                     <mat-icon>rate_review</mat-icon>
@@ -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 }}
     </button>
@@ -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 }}
     </button>
@@ -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 }}
     </button>

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<number>('motions_line_length').subscribe(lineLength => {
             this.lineLength = lineLength;
-            this.recalcUnifiedChanges();
+            this.sortedChangingObjects = null;
         });
         this.configService
             .get<LineNumberingMode>('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
+        // When "diff" is the default view mode, the crMode might have been set before the motion,
-        // before the motion and allChangingObjects have been loaded. As the availability of "diff" depends on
+        // amendments and change recommendations have been loaded.
-        // allChangingObjects, we set "diff" first in this case (in the config-listener) and perform the actual
+        // As the availability of "diff" depends on amendments and change recommendations, we set "diff"
-        // check if "diff" is possible now.
+        // 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
+        // Prevent this.sortedChangingObjects to be reordered from within formatMotion
-        const changes: ViewUnifiedChange[] = Object.assign([], this.getAllTextChangingObjects());
+        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