Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anchor comments directly to the scroll position and optimise comment layout #10772

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Anchor comments directly to the scroll position and optimise comment layout #10772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d65eb3d
Synchronise comment blocks with document position
Dec 17, 2024
57c9cab
Optimisation pass on CommentListSection layout
Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 42 additions & 36 deletions browser/src/canvas/sections/CommentListSection.ts
View file
Open in desktop
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 @@ -1250,7 +1255,10 @@ export class CommentSection extends app.definitions.canvasSectionObject {
this.sectionProperties.selectedComment.hide();
}

this.update();
var previousAnimationState = this.disableLayoutAnimation;
this.disableLayoutAnimation = true;
this.update(true, false);
this.disableLayoutAnimation = previousAnimationState;
}

private showHideComments (): void {
Expand Down Expand Up @@ -1798,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 @@ -1810,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 @@ -1831,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 @@ -1841,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 @@ -1857,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 @@ -1884,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 @@ -1898,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 @@ -1913,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 @@ -1942,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 @@ -1951,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 @@ -1983,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 @@ -2010,21 +2020,21 @@ export class CommentSection extends app.definitions.canvasSectionObject {
this.disableLayoutAnimation = false;
}

private layout (zoom: any = null): void {
if (zoom)
this.doLayout();
private layout (immediate: any = null, relayout: boolean = true): void {
if (immediate)
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 (): 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();
this.layout(immediate, relayout);
}

private updateThreadInfoIndicator(): void {
Expand Down Expand Up @@ -2140,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
36 changes: 27 additions & 9 deletions browser/src/canvas/sections/CommentSection.ts
View file
Open in desktop
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 All @@ -337,9 +340,10 @@ export class Comment extends CanvasSectionObject {
lastPosY = childPositions[i].posY + 24;
}
if (i < this.sectionProperties.childLines.length) {
for (let j = i; j < this.sectionProperties.childLines.length; j++)
for (let j = i; j < this.sectionProperties.childLines.length; j++) {
this.sectionProperties.childLinesNode.removeChild(this.sectionProperties.childLines[i]);
this.sectionProperties.childLines.splice(i);
this.sectionProperties.childLines.splice(i);
}
}

}
Expand Down Expand Up @@ -843,6 +847,8 @@ export class Comment extends CanvasSectionObject {
}

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

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

Expand All @@ -852,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;
Copy link
Contributor

@gokaysatir gokaysatir Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "hidden" cannot be true if we are here. I guess we could initiate hidden as "false" and remove these assignments. Not that important but may be confusing in the future while editing the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh whoops, it's not this that's wrong (I don't think), but the check at the top of the function - makes me wonder how all the tests passed - I guess because they don't make multiple changes and at first the value isn't cached (intentionally, to mimic the old behaviour of the comment being in an indeterminate state on creation).

} 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 @@ -868,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 @@ -893,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 @@ -907,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 @@ -1510,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
11 changes: 11 additions & 0 deletions browser/src/dom/PosAnimation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@ L.PosAnimation = L.Class.extend({

this.fire('start');

// Skip some work if the duration is zero
if (duration <= 0) {
el.style[L.DomUtil.TRANSITION] = '';
L.DomUtil.setPosition(el, newPos);
this._el._leaflet_pos = newPos;
this._inProgress = false;
this.fire('step').fire('end');
this._el.dataset.transitioning = false;
return;
}

el.style[L.DomUtil.TRANSITION] = 'all ' + (isNaN(duration) ? 0.25 : 0) +
's cubic-bezier(0,0,' + (easeLinearity || 0.5) + ',1)';

Expand Down
Loading