From bec7104ea68c37cc649883a9cd67991ed1d74497 Mon Sep 17 00:00:00 2001 From: Chris Lord Date: Thu, 19 Dec 2024 13:36:44 +0000 Subject: [PATCH] Optimisation pass on CommentListSection layout CommentListSection layout appears very high in the profile when scrolling. This commit makes various optimisations, mainly around doing less unnecessary work while scrolling and caching regularly accessed values. Comment layout no longer appears anywhere significant in the profile in Writer after this patch. Signed-off-by: Chris Lord Change-Id: I3bc8d040422703375b583fbc8c49bd793547514d --- .../src/canvas/sections/CommentListSection.ts | 73 ++++++++++--------- browser/src/canvas/sections/CommentSection.ts | 20 ++++- 2 files changed, 54 insertions(+), 39 deletions(-) diff --git a/browser/src/canvas/sections/CommentListSection.ts b/browser/src/canvas/sections/CommentListSection.ts index 629c133fd08a..60b7d1f5391f 100644 --- a/browser/src/canvas/sections/CommentListSection.ts +++ b/browser/src/canvas/sections/CommentListSection.ts @@ -94,6 +94,9 @@ export class CommentSection extends app.definitions.canvasSectionObject { // To associate comment id with its index in commentList array. private idIndexMap: Map; + private annotationMinSize: number; + private annotationMaxSize: number; + constructor () { super(); @@ -119,6 +122,8 @@ export class CommentSection extends app.definitions.canvasSectionObject { this.sectionProperties.commentsAreListed = (this.sectionProperties.docLayer._docType === 'text' || this.sectionProperties.docLayer._docType === 'presentation' || this.sectionProperties.docLayer._docType === 'drawing') && !(window).mode.isMobile(); this.idIndexMap = new Map(); this.mobileCommentModalId = this.map.uiManager.generateModalId(this.mobileCommentId); + this.annotationMinSize = Number(getComputedStyle(document.documentElement).getPropertyValue('--annotation-min-size')); + this.annotationMaxSize = Number(getComputedStyle(document.documentElement).getPropertyValue('--annotation-max-size')); } public onInitialize (): void { @@ -1252,7 +1257,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { var previousAnimationState = this.disableLayoutAnimation; this.disableLayoutAnimation = true; - this.update(true); + this.update(true, false); this.disableLayoutAnimation = previousAnimationState; } @@ -1801,10 +1806,10 @@ export class CommentSection extends app.definitions.canvasSectionObject { return this.disableLayoutAnimation ? 0 : undefined; // undefined means it will use default value } - private layoutUp (subList: any, actualPosition: Array, lastY: number): number { + private layoutUp (subList: any, actualPosition: Array, lastY: number, relayout: boolean = true): number { var height: number; for (var i = 0; i < subList.length; i++) { - height = subList[i].getCommentHeight(); + height = subList[i].getCommentHeight(relayout); lastY = subList[i].sectionProperties.data.anchorPix[1] + height < lastY ? subList[i].sectionProperties.data.anchorPix[1]: lastY - (height * app.dpiScale); (new L.PosAnimation()).run(subList[i].sectionProperties.container, {x: Math.round(actualPosition[0] / app.dpiScale), y: Math.round(lastY / app.dpiScale)}, this.getAnimationDuration()); if (!subList[i].isEdit()) @@ -1813,7 +1818,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { return lastY; } - private loopUp (startIndex: number, x: number, startY: number): number { + private loopUp (startIndex: number, x: number, startY: number, relayout: boolean = true): number { var tmpIdx = 0; var checkSelectedPart: boolean = this.mustCheckSelectedPart(); startY -= this.sectionProperties.marginY; @@ -1834,7 +1839,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { } while (tmpIdx > -1 && this.sectionProperties.commentList[tmpIdx].sectionProperties.data.parent === this.sectionProperties.commentList[tmpIdx + 1].sectionProperties.data.id); if (subList.length > 0) { - startY = this.layoutUp(subList, [x, subList[0].sectionProperties.data.anchorPix[1]], startY); + startY = this.layoutUp(subList, [x, subList[0].sectionProperties.data.anchorPix[1]], startY, relayout); i = i - subList.length; } else { i = tmpIdx; @@ -1844,7 +1849,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { return startY; } - private layoutDown (subList: any, actualPosition: Array, lastY: number): number { + private layoutDown (subList: any, actualPosition: Array, lastY: number, relayout: boolean = true): number { var selectedComment = subList[0] === this.sectionProperties.selectedComment; for (var i = 0; i < subList.length; i++) { lastY = subList[i].sectionProperties.data.anchorPix[1] > lastY ? subList[i].sectionProperties.data.anchorPix[1]: lastY; @@ -1852,6 +1857,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { var isRTL = document.documentElement.dir === 'rtl'; if (selectedComment) { + // FIXME: getBoundingClientRect is expensive and this is a hot path (called continuously during animations and scrolling) const posX = (this.sectionProperties.showSelectedBigger ? Math.round((document.getElementById('document-container').getBoundingClientRect().width - subList[i].sectionProperties.container.getBoundingClientRect().width)/2) : Math.round(actualPosition[0] / app.dpiScale) - this.sectionProperties.deflectionOfSelectedComment * (isRTL ? -1 : 1)); @@ -1860,14 +1866,14 @@ export class CommentSection extends app.definitions.canvasSectionObject { else (new L.PosAnimation()).run(subList[i].sectionProperties.container, {x: Math.round(actualPosition[0] / app.dpiScale), y: Math.round(lastY / app.dpiScale)}, this.getAnimationDuration()); - lastY += (subList[i].getCommentHeight() * app.dpiScale); + lastY += (subList[i].getCommentHeight(relayout) * app.dpiScale); if (!subList[i].isEdit()) subList[i].show(); } return lastY; } - private loopDown (startIndex: number, x: number, startY: number): number { + private loopDown (startIndex: number, x: number, startY: number, relayout: boolean = true): number { var tmpIdx = 0; var checkSelectedPart: boolean = this.mustCheckSelectedPart(); // Pass over all comments present @@ -1887,7 +1893,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { } while (tmpIdx < this.sectionProperties.commentList.length && this.sectionProperties.commentList[tmpIdx].sectionProperties.data.parent !== '0'); if (subList.length > 0) { - startY = this.layoutDown(subList, [x, subList[0].sectionProperties.data.anchorPix[1]], startY); + startY = this.layoutDown(subList, [x, subList[0].sectionProperties.data.anchorPix[1]], startY, relayout); i = i + subList.length; } else { i = tmpIdx; @@ -1901,6 +1907,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { if (this.sectionProperties.arrow) { document.getElementById('document-container').removeChild(this.sectionProperties.arrow); this.sectionProperties.arrow = null; + app.sectionContainer.requestReDraw(); } } @@ -1916,7 +1923,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { endPoint[1] = Math.floor(endPoint[1] / app.dpiScale); if (this.sectionProperties.arrow !== null) { - var line: SVGLineElement = (document.getElementById('comment-arrow-line')); + var line: SVGLineElement = (this.sectionProperties.arrow.firstElementChild); line.setAttribute('x1', String(startPoint[0])); line.setAttribute('y1', String(startPoint[1])); line.setAttribute('x2', String(endPoint[0])); @@ -1945,7 +1952,7 @@ export class CommentSection extends app.definitions.canvasSectionObject { } } - private doLayout (): void { + private doLayout (relayout: boolean = true): void { if ((window).mode.isMobile() || this.sectionProperties.docLayer._docType === 'spreadsheet') { if (this.sectionProperties.commentList.length > 0) this.orderCommentList(); @@ -1954,7 +1961,8 @@ export class CommentSection extends app.definitions.canvasSectionObject { if (this.sectionProperties.commentList.length > 0) { this.orderCommentList(); - this.resetCommentsSize(); + if (relayout) + this.resetCommentsSize(); var isRTL = document.documentElement.dir === 'rtl'; @@ -1986,21 +1994,20 @@ export class CommentSection extends app.definitions.canvasSectionObject { this.showArrow([tempCrd[0], tempCrd[1]], [posX, tempCrd[1]]); } } - else { + else this.hideArrow(); - app.sectionContainer.requestReDraw(); - } var lastY = 0; if (selectedIndex) { - this.loopUp(selectedIndex - 1, x, yOrigin); - lastY = this.loopDown(selectedIndex, x, yOrigin); + this.loopUp(selectedIndex - 1, x, yOrigin, relayout); + lastY = this.loopDown(selectedIndex, x, yOrigin, relayout); } else { - lastY = this.loopDown(0, x, topRight[1]); + lastY = this.loopDown(0, x, topRight[1], relayout); } } - this.resizeComments(); + if (relayout) + this.resizeComments(); lastY += this.containerObject.getDocumentTopLeft()[1]; if (lastY > app.file.size.pixels[1]) { @@ -2013,21 +2020,21 @@ export class CommentSection extends app.definitions.canvasSectionObject { this.disableLayoutAnimation = false; } - private layout (immediate: any = null): void { + private layout (immediate: any = null, relayout: boolean = true): void { if (immediate) - this.doLayout(); + this.doLayout(relayout); else if (!this.sectionProperties.layoutTimer) { - this.sectionProperties.layoutTimer = setTimeout(function() { + this.sectionProperties.layoutTimer = setTimeout(() => { delete this.sectionProperties.layoutTimer; - this.doLayout(); - }.bind(this), 10 /* ms */); + this.doLayout(relayout); + }, 10 /* ms */); } // else - avoid excessive re-layout } - private update (immediate: boolean = false): void { - if (this.sectionProperties.docLayer._docType === 'text') + private update (immediate: boolean = false, relayout: boolean = true): void { + if (relayout && this.sectionProperties.docLayer._docType === 'text') this.updateThreadInfoIndicator(); - this.layout(immediate); + this.layout(immediate, relayout); } private updateThreadInfoIndicator(): void { @@ -2143,15 +2150,11 @@ export class CommentSection extends app.definitions.canvasSectionObject { // reset theis size to default (100px text) private resetCommentsSize (): void { if (this.sectionProperties.docLayer._docType === 'text') { - const minMaxHeight = Number(getComputedStyle(document.documentElement).getPropertyValue('--annotation-min-size')); for (var i = 0; i < this.sectionProperties.commentList.length;i++) { - if (this.sectionProperties.commentList[i].sectionProperties.contentNode.style.display !== 'none') - this.sectionProperties.commentList[i].sectionProperties.contentNode.style.maxHeight = minMaxHeight + 'px'; - } - if (this.sectionProperties.selectedComment) { - if (this.sectionProperties.selectedComment.sectionProperties.contentNode.style.display !== 'none') { - const maxMaxHeight = Number(getComputedStyle(document.documentElement).getPropertyValue('--annotation-max-size')); - this.sectionProperties.selectedComment.sectionProperties.contentNode.style.maxHeight = maxMaxHeight + 'px'; + if (this.sectionProperties.commentList[i].sectionProperties.contentNode.style.display !== 'none') { + const maxHeight = (this.sectionProperties.commentList[i] === this.sectionProperties.selectedComment) ? + this.annotationMaxSize : this.annotationMinSize; + this.sectionProperties.commentList[i].sectionProperties.contentNode.style.maxHeight = maxHeight + 'px'; } } } diff --git a/browser/src/canvas/sections/CommentSection.ts b/browser/src/canvas/sections/CommentSection.ts index 95fab1cb7dcc..4d7c7e42ec9f 100644 --- a/browser/src/canvas/sections/CommentSection.ts +++ b/browser/src/canvas/sections/CommentSection.ts @@ -44,6 +44,9 @@ export class Comment extends CanvasSectionObject { map: any; pendingInit: boolean = true; + cachedCommentHeight: number | null = null; + hidden: boolean = true; + // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types constructor (data: any, options: any, commentListSectionPointer: cool.CommentSection) { super(); @@ -321,7 +324,7 @@ export class Comment extends CanvasSectionObject { posY: this.sectionProperties.children[i].sectionProperties.container._leaflet_pos.y}); } childPositions.sort((a, b) => { return a.posY - b.posY; }); - let lastPosY = this.sectionProperties.container._leaflet_pos.y + this.getCommentHeight(); + let lastPosY = this.sectionProperties.container._leaflet_pos.y + this.getCommentHeight(false); let i = 0; for (; i < childPositions.length; i++) { if (this.sectionProperties.childLines[i] === undefined) { @@ -844,6 +847,9 @@ export class Comment extends CanvasSectionObject { } public show(): void { + if (!this.hidden) return; + this.hidden = false; + this.doPendingInitializationInView(true /* force */); this.showMarker(); @@ -908,9 +914,10 @@ export class Comment extends CanvasSectionObject { } public hide (): void { - if (this.isEdit()) { + if (this.hidden || this.isEdit()) { return; } + this.hidden = true; if (this.sectionProperties.data.id === 'new') { this.sectionProperties.commentListSection.removeItem(this.sectionProperties.data.id); @@ -1511,8 +1518,13 @@ export class Comment extends CanvasSectionObject { return 1; // Comment list not fully initialized but we know we are not root } - public getCommentHeight(): number { - return this.sectionProperties.container.getBoundingClientRect().height - this.sectionProperties.childLinesNode.getBoundingClientRect().height; + public getCommentHeight(invalidateCache: boolean = true): number { + if (invalidateCache) + this.cachedCommentHeight = null; + if (this.cachedCommentHeight === null) + this.cachedCommentHeight = this.sectionProperties.container.getBoundingClientRect().height + - this.sectionProperties.childLinesNode.getBoundingClientRect().height; + return this.cachedCommentHeight; } public setCollapsed(): void {