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 <chris.lord@collabora.com>
Change-Id: I3bc8d040422703375b583fbc8c49bd793547514d
  • Loading branch information
Chris Lord committed Dec 19, 2024
1 parent 91a604e commit bec7104
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 39 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
20 changes: 16 additions & 4 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 = true;

// 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,9 @@ export class Comment extends CanvasSectionObject {
}

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

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

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit bec7104

Please sign in to comment.