ag-admin
/
OpenSlides
0
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Improve performance of line range detection + html2fragment / avoid recalculation of full changeset

This commit is contained in:
Tobias Hößl 2020-08-18 19:22:35 +02:00
parent 2e8e32454e
commit fcdfad1c2e
No known key found for this signature in database
GPG Key ID: 1D780C7599C2D2A2
5 changed files with 99 additions and 74 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
client/src/app/core/ui-services/diff.service.ts
View File

@ -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;
}
/**

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

@ -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 => {

60
client/src/app/core/ui-services/linenumbering.service.ts
View File

@ -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(),
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 = '&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;
}

8
client/src/app/site/motions/modules/motion-detail/components/motion-detail/motion-detail.component.html
View File

@ -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>

85
client/src/app/site/motions/modules/motion-detail/components/motion-detail/motion-detail.component.ts
View File

@ -224,17 +224,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
*/
@ -522,7 +522,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')
@ -612,15 +612,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
@ -634,10 +656,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) {
@ -651,11 +673,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()) {
@ -665,10 +687,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);
@ -709,7 +731,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)
@ -718,7 +741,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 {
@ -943,8 +967,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,
@ -1009,7 +1038,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 {
@ -1019,17 +1048,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 {
@ -1545,10 +1578,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