Skip to content

Commit 216e8f8

Browse files
Chris Lordcaolanm
Chris Lord
authored and
caolanm
committed
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 <[email protected]> Change-Id: I3bc8d040422703375b583fbc8c49bd793547514d
1 parent 7db5c0a commit 216e8f8

File tree

2 files changed

+62
-42
lines changed

2 files changed

+62
-42
lines changed

‎browser/src/canvas/sections/CommentListSection.ts

+38-35
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,9 @@ export class CommentSection extends app.definitions.canvasSectionObject {
9494
// To associate comment id with its index in commentList array.
9595
private idIndexMap: Map<any, number>;
9696

97+
private annotationMinSize: number;
98+
private annotationMaxSize: number;
99+
97100
constructor () {
98101
super();
99102

@@ -119,6 +122,8 @@ export class CommentSection extends app.definitions.canvasSectionObject {
119122
this.sectionProperties.commentsAreListed = (this.sectionProperties.docLayer._docType === 'text' || this.sectionProperties.docLayer._docType === 'presentation' || this.sectionProperties.docLayer._docType === 'drawing') && !(<any>window).mode.isMobile();
120123
this.idIndexMap = new Map<any, number>();
121124
this.mobileCommentModalId = this.map.uiManager.generateModalId(this.mobileCommentId);
125+
this.annotationMinSize = Number(getComputedStyle(document.documentElement).getPropertyValue('--annotation-min-size'));
126+
this.annotationMaxSize = Number(getComputedStyle(document.documentElement).getPropertyValue('--annotation-max-size'));
122127
}
123128

124129
public onInitialize (): void {
@@ -1252,7 +1257,7 @@ export class CommentSection extends app.definitions.canvasSectionObject {
12521257

12531258
var previousAnimationState = this.disableLayoutAnimation;
12541259
this.disableLayoutAnimation = true;
1255-
this.update(true);
1260+
this.update(true, false);
12561261
this.disableLayoutAnimation = previousAnimationState;
12571262
}
12581263

@@ -1801,10 +1806,10 @@ export class CommentSection extends app.definitions.canvasSectionObject {
18011806
return this.disableLayoutAnimation ? 0 : undefined; // undefined means it will use default value
18021807
}
18031808

1804-
private layoutUp (subList: any, actualPosition: Array<number>, lastY: number): number {
1809+
private layoutUp (subList: any, actualPosition: Array<number>, lastY: number, relayout: boolean = true): number {
18051810
var height: number;
18061811
for (var i = 0; i < subList.length; i++) {
1807-
height = subList[i].getCommentHeight();
1812+
height = subList[i].getCommentHeight(relayout);
18081813
lastY = subList[i].sectionProperties.data.anchorPix[1] + height < lastY ? subList[i].sectionProperties.data.anchorPix[1]: lastY - (height * app.dpiScale);
18091814
(new L.PosAnimation()).run(subList[i].sectionProperties.container, {x: Math.round(actualPosition[0] / app.dpiScale), y: Math.round(lastY / app.dpiScale)}, this.getAnimationDuration());
18101815
if (!subList[i].isEdit())
@@ -1813,7 +1818,7 @@ export class CommentSection extends app.definitions.canvasSectionObject {
18131818
return lastY;
18141819
}
18151820

1816-
private loopUp (startIndex: number, x: number, startY: number): number {
1821+
private loopUp (startIndex: number, x: number, startY: number, relayout: boolean = true): number {
18171822
var tmpIdx = 0;
18181823
var checkSelectedPart: boolean = this.mustCheckSelectedPart();
18191824
startY -= this.sectionProperties.marginY;
@@ -1834,7 +1839,7 @@ export class CommentSection extends app.definitions.canvasSectionObject {
18341839
} while (tmpIdx > -1 && this.sectionProperties.commentList[tmpIdx].sectionProperties.data.parent === this.sectionProperties.commentList[tmpIdx + 1].sectionProperties.data.id);
18351840

18361841
if (subList.length > 0) {
1837-
startY = this.layoutUp(subList, [x, subList[0].sectionProperties.data.anchorPix[1]], startY);
1842+
startY = this.layoutUp(subList, [x, subList[0].sectionProperties.data.anchorPix[1]], startY, relayout);
18381843
i = i - subList.length;
18391844
} else {
18401845
i = tmpIdx;
@@ -1844,14 +1849,15 @@ export class CommentSection extends app.definitions.canvasSectionObject {
18441849
return startY;
18451850
}
18461851

1847-
private layoutDown (subList: any, actualPosition: Array<number>, lastY: number): number {
1852+
private layoutDown (subList: any, actualPosition: Array<number>, lastY: number, relayout: boolean = true): number {
18481853
var selectedComment = subList[0] === this.sectionProperties.selectedComment;
18491854
for (var i = 0; i < subList.length; i++) {
18501855
lastY = subList[i].sectionProperties.data.anchorPix[1] > lastY ? subList[i].sectionProperties.data.anchorPix[1]: lastY;
18511856

18521857
var isRTL = document.documentElement.dir === 'rtl';
18531858

18541859
if (selectedComment) {
1860+
// FIXME: getBoundingClientRect is expensive and this is a hot path (called continuously during animations and scrolling)
18551861
const posX = (this.sectionProperties.showSelectedBigger ?
18561862
Math.round((document.getElementById('document-container').getBoundingClientRect().width - subList[i].sectionProperties.container.getBoundingClientRect().width)/2) :
18571863
Math.round(actualPosition[0] / app.dpiScale) - this.sectionProperties.deflectionOfSelectedComment * (isRTL ? -1 : 1));
@@ -1860,14 +1866,14 @@ export class CommentSection extends app.definitions.canvasSectionObject {
18601866
else
18611867
(new L.PosAnimation()).run(subList[i].sectionProperties.container, {x: Math.round(actualPosition[0] / app.dpiScale), y: Math.round(lastY / app.dpiScale)}, this.getAnimationDuration());
18621868

1863-
lastY += (subList[i].getCommentHeight() * app.dpiScale);
1869+
lastY += (subList[i].getCommentHeight(relayout) * app.dpiScale);
18641870
if (!subList[i].isEdit())
18651871
subList[i].show();
18661872
}
18671873
return lastY;
18681874
}
18691875

1870-
private loopDown (startIndex: number, x: number, startY: number): number {
1876+
private loopDown (startIndex: number, x: number, startY: number, relayout: boolean = true): number {
18711877
var tmpIdx = 0;
18721878
var checkSelectedPart: boolean = this.mustCheckSelectedPart();
18731879
// Pass over all comments present
@@ -1887,7 +1893,7 @@ export class CommentSection extends app.definitions.canvasSectionObject {
18871893
} while (tmpIdx < this.sectionProperties.commentList.length && this.sectionProperties.commentList[tmpIdx].sectionProperties.data.parent !== '0');
18881894

18891895
if (subList.length > 0) {
1890-
startY = this.layoutDown(subList, [x, subList[0].sectionProperties.data.anchorPix[1]], startY);
1896+
startY = this.layoutDown(subList, [x, subList[0].sectionProperties.data.anchorPix[1]], startY, relayout);
18911897
i = i + subList.length;
18921898
} else {
18931899
i = tmpIdx;
@@ -1901,6 +1907,7 @@ export class CommentSection extends app.definitions.canvasSectionObject {
19011907
if (this.sectionProperties.arrow) {
19021908
document.getElementById('document-container').removeChild(this.sectionProperties.arrow);
19031909
this.sectionProperties.arrow = null;
1910+
app.sectionContainer.requestReDraw();
19041911
}
19051912
}
19061913

@@ -1916,7 +1923,7 @@ export class CommentSection extends app.definitions.canvasSectionObject {
19161923
endPoint[1] = Math.floor(endPoint[1] / app.dpiScale);
19171924

19181925
if (this.sectionProperties.arrow !== null) {
1919-
var line: SVGLineElement = <SVGLineElement>(<any>document.getElementById('comment-arrow-line'));
1926+
var line: SVGLineElement = <SVGLineElement>(this.sectionProperties.arrow.firstElementChild);
19201927
line.setAttribute('x1', String(startPoint[0]));
19211928
line.setAttribute('y1', String(startPoint[1]));
19221929
line.setAttribute('x2', String(endPoint[0]));
@@ -1945,7 +1952,7 @@ export class CommentSection extends app.definitions.canvasSectionObject {
19451952
}
19461953
}
19471954

1948-
private doLayout (): void {
1955+
private doLayout (relayout: boolean = true): void {
19491956
if ((<any>window).mode.isMobile() || this.sectionProperties.docLayer._docType === 'spreadsheet') {
19501957
if (this.sectionProperties.commentList.length > 0)
19511958
this.orderCommentList();
@@ -1954,7 +1961,8 @@ export class CommentSection extends app.definitions.canvasSectionObject {
19541961

19551962
if (this.sectionProperties.commentList.length > 0) {
19561963
this.orderCommentList();
1957-
this.resetCommentsSize();
1964+
if (relayout)
1965+
this.resetCommentsSize();
19581966

19591967
var isRTL = document.documentElement.dir === 'rtl';
19601968

@@ -1986,21 +1994,20 @@ export class CommentSection extends app.definitions.canvasSectionObject {
19861994
this.showArrow([tempCrd[0], tempCrd[1]], [posX, tempCrd[1]]);
19871995
}
19881996
}
1989-
else {
1997+
else
19901998
this.hideArrow();
1991-
app.sectionContainer.requestReDraw();
1992-
}
19931999

19942000
var lastY = 0;
19952001
if (selectedIndex) {
1996-
this.loopUp(selectedIndex - 1, x, yOrigin);
1997-
lastY = this.loopDown(selectedIndex, x, yOrigin);
2002+
this.loopUp(selectedIndex - 1, x, yOrigin, relayout);
2003+
lastY = this.loopDown(selectedIndex, x, yOrigin, relayout);
19982004
}
19992005
else {
2000-
lastY = this.loopDown(0, x, topRight[1]);
2006+
lastY = this.loopDown(0, x, topRight[1], relayout);
20012007
}
20022008
}
2003-
this.resizeComments();
2009+
if (relayout)
2010+
this.resizeComments();
20042011

20052012
lastY += this.containerObject.getDocumentTopLeft()[1];
20062013
if (lastY > app.file.size.pixels[1]) {
@@ -2013,21 +2020,21 @@ export class CommentSection extends app.definitions.canvasSectionObject {
20132020
this.disableLayoutAnimation = false;
20142021
}
20152022

2016-
private layout (immediate: any = null): void {
2023+
private layout (immediate: any = null, relayout: boolean = true): void {
20172024
if (immediate)
2018-
this.doLayout();
2025+
this.doLayout(relayout);
20192026
else if (!this.sectionProperties.layoutTimer) {
2020-
this.sectionProperties.layoutTimer = setTimeout(function() {
2027+
this.sectionProperties.layoutTimer = setTimeout(() => {
20212028
delete this.sectionProperties.layoutTimer;
2022-
this.doLayout();
2023-
}.bind(this), 10 /* ms */);
2029+
this.doLayout(relayout);
2030+
}, 10 /* ms */);
20242031
} // else - avoid excessive re-layout
20252032
}
20262033

2027-
private update (immediate: boolean = false): void {
2028-
if (this.sectionProperties.docLayer._docType === 'text')
2034+
private update (immediate: boolean = false, relayout: boolean = true): void {
2035+
if (relayout && this.sectionProperties.docLayer._docType === 'text')
20292036
this.updateThreadInfoIndicator();
2030-
this.layout(immediate);
2037+
this.layout(immediate, relayout);
20312038
}
20322039

20332040
private updateThreadInfoIndicator(): void {
@@ -2143,15 +2150,11 @@ export class CommentSection extends app.definitions.canvasSectionObject {
21432150
// reset theis size to default (100px text)
21442151
private resetCommentsSize (): void {
21452152
if (this.sectionProperties.docLayer._docType === 'text') {
2146-
const minMaxHeight = Number(getComputedStyle(document.documentElement).getPropertyValue('--annotation-min-size'));
21472153
for (var i = 0; i < this.sectionProperties.commentList.length;i++) {
2148-
if (this.sectionProperties.commentList[i].sectionProperties.contentNode.style.display !== 'none')
2149-
this.sectionProperties.commentList[i].sectionProperties.contentNode.style.maxHeight = minMaxHeight + 'px';
2150-
}
2151-
if (this.sectionProperties.selectedComment) {
2152-
if (this.sectionProperties.selectedComment.sectionProperties.contentNode.style.display !== 'none') {
2153-
const maxMaxHeight = Number(getComputedStyle(document.documentElement).getPropertyValue('--annotation-max-size'));
2154-
this.sectionProperties.selectedComment.sectionProperties.contentNode.style.maxHeight = maxMaxHeight + 'px';
2154+
if (this.sectionProperties.commentList[i].sectionProperties.contentNode.style.display !== 'none') {
2155+
const maxHeight = (this.sectionProperties.commentList[i] === this.sectionProperties.selectedComment) ?
2156+
this.annotationMaxSize : this.annotationMinSize;
2157+
this.sectionProperties.commentList[i].sectionProperties.contentNode.style.maxHeight = maxHeight + 'px';
21552158
}
21562159
}
21572160
}

‎browser/src/canvas/sections/CommentSection.ts

+24-7
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,9 @@ export class Comment extends CanvasSectionObject {
4444
map: any;
4545
pendingInit: boolean = true;
4646

47+
cachedCommentHeight: number | null = null;
48+
hidden: boolean | null = null;
49+
4750
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
4851
constructor (data: any, options: any, commentListSectionPointer: cool.CommentSection) {
4952
super();
@@ -321,7 +324,7 @@ export class Comment extends CanvasSectionObject {
321324
posY: this.sectionProperties.children[i].sectionProperties.container._leaflet_pos.y});
322325
}
323326
childPositions.sort((a, b) => { return a.posY - b.posY; });
324-
let lastPosY = this.sectionProperties.container._leaflet_pos.y + this.getCommentHeight();
327+
let lastPosY = this.sectionProperties.container._leaflet_pos.y + this.getCommentHeight(false);
325328
let i = 0;
326329
for (; i < childPositions.length; i++) {
327330
if (this.sectionProperties.childLines[i] === undefined) {
@@ -844,6 +847,8 @@ export class Comment extends CanvasSectionObject {
844847
}
845848

846849
public show(): void {
850+
if (this.hidden === true) return;
851+
847852
this.doPendingInitializationInView(true /* force */);
848853
this.showMarker();
849854

@@ -853,11 +858,16 @@ export class Comment extends CanvasSectionObject {
853858

854859
if (cool.CommentSection.commentWasAutoAdded)
855860
return;
856-
if (this.sectionProperties.docLayer._docType === 'text')
861+
862+
// We don't cache the hidden state for spreadsheets. Only one comment can be
863+
// visible and they're hidden when scrolling, so it's easier this way.
864+
if (this.sectionProperties.docLayer._docType === 'text') {
857865
this.showWriter();
858-
else if (this.sectionProperties.docLayer._docType === 'presentation' || this.sectionProperties.docLayer._docType === 'drawing')
866+
this.hidden = false;
867+
} else if (this.sectionProperties.docLayer._docType === 'presentation' || this.sectionProperties.docLayer._docType === 'drawing') {
859868
this.showImpressDraw();
860-
else if (this.sectionProperties.docLayer._docType === 'spreadsheet')
869+
this.hidden = false;
870+
} else if (this.sectionProperties.docLayer._docType === 'spreadsheet')
861871
this.showCalc();
862872

863873
this.setLayoutClass();
@@ -869,6 +879,7 @@ export class Comment extends CanvasSectionObject {
869879
this.sectionProperties.nodeReply.style.display = 'none';
870880
this.sectionProperties.showSelectedCoordinate = false;
871881
L.DomUtil.removeClass(this.sectionProperties.container, 'cool-annotation-collapsed-show');
882+
this.hidden = true;
872883
}
873884

874885
private hideCalc() {
@@ -894,6 +905,7 @@ export class Comment extends CanvasSectionObject {
894905
this.sectionProperties.nodeReply.style.display = 'none';
895906
}
896907
L.DomUtil.removeClass(this.sectionProperties.container, 'cool-annotation-collapsed-show');
908+
this.hidden = true;
897909
}
898910

899911
// check if this is "our" autosaved comment
@@ -908,7 +920,7 @@ export class Comment extends CanvasSectionObject {
908920
}
909921

910922
public hide (): void {
911-
if (this.isEdit()) {
923+
if (this.hidden === true || this.isEdit()) {
912924
return;
913925
}
914926

@@ -1511,8 +1523,13 @@ export class Comment extends CanvasSectionObject {
15111523
return 1; // Comment list not fully initialized but we know we are not root
15121524
}
15131525

1514-
public getCommentHeight(): number {
1515-
return this.sectionProperties.container.getBoundingClientRect().height - this.sectionProperties.childLinesNode.getBoundingClientRect().height;
1526+
public getCommentHeight(invalidateCache: boolean = true): number {
1527+
if (invalidateCache)
1528+
this.cachedCommentHeight = null;
1529+
if (this.cachedCommentHeight === null)
1530+
this.cachedCommentHeight = this.sectionProperties.container.getBoundingClientRect().height
1531+
- this.sectionProperties.childLinesNode.getBoundingClientRect().height;
1532+
return this.cachedCommentHeight;
15161533
}
15171534

15181535
public setCollapsed(): void {

0 commit comments

Comments
 (0)