Skip to content

Commit

Permalink
Optimisation pass on CommentListSection layout
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
Chris Lord committed Dec 20, 2024
1 parent d65eb3d commit 57c9cab
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 42 deletions.
73 changes: 38 additions & 35 deletions browser/src/canvas/sections/CommentListSection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ export class CommentSection extends app.definitions.canvasSectionObject {
// To associate comment id with its index in commentList array.
private idIndexMap: Map<any, number>;

private annotationMinSize: number;
private annotationMaxSize: number;

constructor () {
super();

Expand All @@ -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') && !(<any>window).mode.isMobile();
this.idIndexMap = new Map<any, number>();
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 {
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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<number>, lastY: number): number {
private layoutUp (subList: any, actualPosition: Array<number>, 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())
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -1844,14 +1849,15 @@ export class CommentSection extends app.definitions.canvasSectionObject {
return startY;
}

private layoutDown (subList: any, actualPosition: Array<number>, lastY: number): number {
private layoutDown (subList: any, actualPosition: Array<number>, 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;

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));
Expand All @@ -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
Expand All @@ -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;
Expand All @@ -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();
}
}

Expand All @@ -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 = <SVGLineElement>(<any>document.getElementById('comment-arrow-line'));
var line: SVGLineElement = <SVGLineElement>(this.sectionProperties.arrow.firstElementChild);
line.setAttribute('x1', String(startPoint[0]));
line.setAttribute('y1', String(startPoint[1]));
line.setAttribute('x2', String(endPoint[0]));
Expand Down Expand Up @@ -1945,7 +1952,7 @@ export class CommentSection extends app.definitions.canvasSectionObject {
}
}

private doLayout (): void {
private doLayout (relayout: boolean = true): void {
if ((<any>window).mode.isMobile() || this.sectionProperties.docLayer._docType === 'spreadsheet') {
if (this.sectionProperties.commentList.length > 0)
this.orderCommentList();
Expand All @@ -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';

Expand Down Expand Up @@ -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]) {
Expand All @@ -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 {
Expand Down Expand Up @@ -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';
}
}
}
Expand Down
31 changes: 24 additions & 7 deletions browser/src/canvas/sections/CommentSection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ export class Comment extends CanvasSectionObject {
map: any;
pendingInit: boolean = true;

cachedCommentHeight: number | null = null;
hidden: boolean | null = null;

// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
constructor (data: any, options: any, commentListSectionPointer: cool.CommentSection) {
super();
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -844,6 +847,8 @@ export class Comment extends CanvasSectionObject {
}

public show(): void {
if (this.hidden === false) return;

this.doPendingInitializationInView(true /* force */);
this.showMarker();

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

if (cool.CommentSection.commentWasAutoAdded)
return;
if (this.sectionProperties.docLayer._docType === 'text')

// We don't cache the hidden state for spreadsheets. Only one comment can be
// visible and they're hidden when scrolling, so it's easier this way.
if (this.sectionProperties.docLayer._docType === 'text') {
this.showWriter();
else if (this.sectionProperties.docLayer._docType === 'presentation' || this.sectionProperties.docLayer._docType === 'drawing')
this.hidden = false;
} else if (this.sectionProperties.docLayer._docType === 'presentation' || this.sectionProperties.docLayer._docType === 'drawing') {
this.showImpressDraw();
else if (this.sectionProperties.docLayer._docType === 'spreadsheet')
this.hidden = false;
} else if (this.sectionProperties.docLayer._docType === 'spreadsheet')
this.showCalc();

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

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

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

public hide (): void {
if (this.isEdit()) {
if (this.hidden === true || this.isEdit()) {
return;
}

Expand Down Expand Up @@ -1511,8 +1523,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 {
Expand Down

0 comments on commit 57c9cab

Please sign in to comment.