Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Commit

Permalink
Minor code quality updates for ScrollPanel (#9639)
Browse files Browse the repository at this point in the history 
* Minor code quality updates for ScrollPanel

and removal of unused function `scrollRelative` in MessagePanel (discovered when changing the scrollRelative types in ScrollPanel).

Changes:
* Clean up logging a bit, make it clearer
* Remove dead code
* Add extra types for tsc --strict compliance (not complete)
* Fix IDE warnings around spelling and missing awaits/promise return values
* Modernize usage of logging

* Fix more tsc --strict errors while we're here

* It's a good thing we have a linter

* Fix even more strict errors
  • Loading branch information
@turt2live
turt2live committed Nov 29, 2022
1 parent f642765 commit 09282d9
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 56 deletions.
13 changes: 1 addition & 12 deletions src/components/structures/MessagePanel.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ interface IProps {
onFillRequest?(backwards: boolean): Promise<boolean>;

// helper function to access relations for an event
onUnfillRequest?(backwards: boolean, scrollToken: string): void;
onUnfillRequest?(backwards: boolean, scrollToken: string | null): void;

getRelationsForEvent?: GetRelationsForEvent;

Expand Down Expand Up @@ -413,17 +413,6 @@ export default class MessagePanel extends React.Component<IProps, IState> {
}
}

/**
* Page up/down.
*
* @param {number} mult: -1 to page up, +1 to page down
*/
public scrollRelative(mult: number): void {
if (this.scrollPanel.current) {
this.scrollPanel.current.scrollRelative(mult);
}
}

/**
* Scroll up/down in response to a scroll key
*
Expand Down
98 changes: 54 additions & 44 deletions src/components/structures/ScrollPanel.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright 2015 - 2021 The Matrix.org Foundation C.I.C.
Copyright 2015 - 2022 The Matrix.org Foundation C.I.C.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -30,8 +30,8 @@ const UNPAGINATION_PADDING = 6000;
// The number of milliseconds to debounce calls to onUnfillRequest,
// to prevent many scroll events causing many unfilling requests.
const UNFILL_REQUEST_DEBOUNCE_MS = 200;
// updateHeight makes the height a ceiled multiple of this so we don't have to update the height too often.
// It also allows the user to scroll past the pagination spinner a bit so they don't feel blocked so
// updateHeight makes the height a `Math.ceil` multiple of this, so we don't have to update the height too often.
// It also allows the user to scroll past the pagination spinner a bit, so they don't feel blocked so
// much while the content loads.
const PAGE_SIZE = 400;

Expand Down Expand Up @@ -134,7 +134,7 @@ interface IProps {
*
* - fixed, in which the viewport is conceptually tied at a specific scroll
* offset. We don't save the absolute scroll offset, because that would be
* affected by window width, zoom level, amount of scrollback, etc. Instead
* affected by window width, zoom level, amount of scrollback, etc. Instead,
* we save an identifier for the last fully-visible message, and the number
* of pixels the window was scrolled below it - which is hopefully near
* enough.
Expand All @@ -161,7 +161,8 @@ interface IPreventShrinkingState {
}

export default class ScrollPanel extends React.Component<IProps> {
static defaultProps = {
// noinspection JSUnusedLocalSymbols
public static defaultProps = {
stickyBottom: true,
startAtBottom: true,
onFillRequest: function(backwards: boolean) { return Promise.resolve(false); },
Expand Down Expand Up @@ -200,21 +201,21 @@ export default class ScrollPanel extends React.Component<IProps> {
this.resetScrollState();
}

componentDidMount() {
public componentDidMount(): void {
this.checkScroll();
}

componentDidUpdate() {
public componentDidUpdate(): void {
// after adding event tiles, we may need to tweak the scroll (either to
// keep at the bottom of the timeline, or to maintain the view after
// adding events to the top).
//
// This will also re-check the fill state, in case the paginate was inadequate
// This will also re-check the fill state, in case the pagination was inadequate
this.checkScroll(true);
this.updatePreventShrinking();
}

componentWillUnmount() {
public componentWillUnmount(): void {
// set a boolean to say we've been unmounted, which any pending
// promises can use to throw away their results.
//
Expand All @@ -224,19 +225,20 @@ export default class ScrollPanel extends React.Component<IProps> {
this.props.resizeNotifier?.removeListener("middlePanelResizedNoisy", this.onResize);
}

private onScroll = ev => {
private onScroll = (ev: Event | React.UIEvent): void => {
// skip scroll events caused by resizing
if (this.props.resizeNotifier && this.props.resizeNotifier.isResizing) return;
debuglog("onScroll", this.getScrollNode().scrollTop);
debuglog("onScroll called past resize gate; scroll node top:", this.getScrollNode().scrollTop);
this.scrollTimeout.restart();
this.saveScrollState();
this.updatePreventShrinking();
this.props.onScroll(ev);
this.props.onScroll?.(ev as Event);
// noinspection JSIgnoredPromiseFromCall
this.checkFillState();
};

private onResize = () => {
debuglog("onResize");
private onResize = (): void => {
debuglog("onResize called");
this.checkScroll();
// update preventShrinkingState if present
if (this.preventShrinkingState) {
Expand All @@ -246,11 +248,14 @@ export default class ScrollPanel extends React.Component<IProps> {

// after an update to the contents of the panel, check that the scroll is
// where it ought to be, and set off pagination requests if necessary.
public checkScroll = (isFromPropsUpdate = false) => {
public checkScroll = (isFromPropsUpdate = false): void => {
if (this.unmounted) {
return;
}
// We don't care if these two conditions race - they're different trees.
// noinspection JSIgnoredPromiseFromCall
this.restoreSavedScrollState();
// noinspection JSIgnoredPromiseFromCall
this.checkFillState(0, isFromPropsUpdate);
};

Expand All @@ -259,7 +264,7 @@ export default class ScrollPanel extends React.Component<IProps> {
// note that this is independent of the 'stuckAtBottom' state - it is simply
// about whether the content is scrolled down right now, irrespective of
// whether it will stay that way when the children update.
public isAtBottom = () => {
public isAtBottom = (): boolean => {
const sn = this.getScrollNode();
// fractional values (both too big and too small)
// for scrollTop happen on certain browsers/platforms
Expand All @@ -277,7 +282,7 @@ export default class ScrollPanel extends React.Component<IProps> {

// returns the vertical height in the given direction that can be removed from
// the content box (which has a height of scrollHeight, see checkFillState) without
// pagination occuring.
// pagination occurring.
//
// padding* = UNPAGINATION_PADDING
//
Expand Down Expand Up @@ -329,7 +334,7 @@ export default class ScrollPanel extends React.Component<IProps> {
const isFirstCall = depth === 0;
const sn = this.getScrollNode();

// if there is less than a screenful of messages above or below the
// if there is less than a screen's worth of messages above or below the
// viewport, try to get some more messages.
//
// scrollTop is the number of pixels between the top of the content and
Expand Down Expand Up @@ -408,6 +413,7 @@ export default class ScrollPanel extends React.Component<IProps> {
const refillDueToPropsUpdate = this.pendingFillDueToPropsUpdate;
this.fillRequestWhileRunning = false;
this.pendingFillDueToPropsUpdate = false;
// noinspection ES6MissingAwait
this.checkFillState(0, refillDueToPropsUpdate);
}
};
Expand All @@ -424,7 +430,7 @@ export default class ScrollPanel extends React.Component<IProps> {
const tiles = this.itemlist.current.children;

// The scroll token of the first/last tile to be unpaginated
let markerScrollToken = null;
let markerScrollToken: string | null = null;

// Subtract heights of tiles to simulate the tiles being unpaginated until the
// excess height is less than the height of the next tile to subtract. This
Expand All @@ -434,7 +440,7 @@ export default class ScrollPanel extends React.Component<IProps> {
// If backwards is true, we unpaginate (remove) tiles from the back (top).
let tile;
for (let i = 0; i < tiles.length; i++) {
tile = tiles[backwards ? i : tiles.length - 1 - i];
tile = tiles[backwards ? i : (tiles.length - 1 - i)];
// Subtract height of tile as if it were unpaginated
excessHeight -= tile.clientHeight;
//If removing the tile would lead to future pagination, break before setting scroll token
Expand All @@ -455,8 +461,8 @@ export default class ScrollPanel extends React.Component<IProps> {
}
this.unfillDebouncer = setTimeout(() => {
this.unfillDebouncer = null;
debuglog("unfilling now", backwards, origExcessHeight);
this.props.onUnfillRequest(backwards, markerScrollToken);
debuglog("unfilling now", { backwards, origExcessHeight });
this.props.onUnfillRequest?.(backwards, markerScrollToken!);
}, UNFILL_REQUEST_DEBOUNCE_MS);
}
}
Expand All @@ -465,11 +471,11 @@ export default class ScrollPanel extends React.Component<IProps> {
private maybeFill(depth: number, backwards: boolean): Promise<void> {
const dir = backwards ? 'b' : 'f';
if (this.pendingFillRequests[dir]) {
debuglog("Already a "+dir+" fill in progress - not starting another");
return;
debuglog("Already a fill in progress - not starting another; direction=", dir);
return Promise.resolve();
}

debuglog("starting "+dir+" fill");
debuglog("starting fill; direction=", dir);

// onFillRequest can end up calling us recursively (via onScroll
// events) so make sure we set this before firing off the call.
Expand All @@ -490,7 +496,7 @@ export default class ScrollPanel extends React.Component<IProps> {
// Unpaginate once filling is complete
this.checkUnfillState(!backwards);

debuglog(""+dir+" fill complete; hasMoreResults:"+hasMoreResults);
debuglog("fill complete; hasMoreResults=", hasMoreResults, "direction=", dir);
if (hasMoreResults) {
// further pagination requests have been disabled until now, so
// it's time to check the fill state again in case the pagination
Expand Down Expand Up @@ -562,11 +568,12 @@ export default class ScrollPanel extends React.Component<IProps> {
/**
* Page up/down.
*
* @param {number} mult: -1 to page up, +1 to page down
* @param {number} multiple: -1 to page up, +1 to page down
*/
public scrollRelative = (mult: number): void => {
public scrollRelative = (multiple: -1 | 1): void => {
const scrollNode = this.getScrollNode();
const delta = mult * scrollNode.clientHeight * 0.9;
// TODO: Document what magic number 0.9 is doing
const delta = multiple * scrollNode.clientHeight * 0.9;
scrollNode.scrollBy(0, delta);
this.saveScrollState();
};
Expand Down Expand Up @@ -608,7 +615,7 @@ export default class ScrollPanel extends React.Component<IProps> {
pixelOffset = pixelOffset || 0;
offsetBase = offsetBase || 0;

// set the trackedScrollToken so we can get the node through getTrackedNode
// set the trackedScrollToken, so we can get the node through getTrackedNode
this.scrollState = {
stuckAtBottom: false,
trackedScrollToken: scrollToken,
Expand All @@ -621,7 +628,7 @@ export default class ScrollPanel extends React.Component<IProps> {
// would position the trackedNode towards the top of the viewport.
// This because when setting the scrollTop only 10 or so events might be loaded,
// not giving enough content below the trackedNode to scroll downwards
// enough so it ends up in the top of the viewport.
// enough, so it ends up in the top of the viewport.
debuglog("scrollToken: setting scrollTop", { offsetBase, pixelOffset, offsetTop: trackedNode.offsetTop });
scrollNode.scrollTop = (trackedNode.offsetTop - (scrollNode.clientHeight * offsetBase)) + pixelOffset;
this.saveScrollState();
Expand All @@ -640,15 +647,16 @@ export default class ScrollPanel extends React.Component<IProps> {

const itemlist = this.itemlist.current;
const messages = itemlist.children;
let node = null;
let node: HTMLElement | null = null;

// TODO: do a binary search here, as items are sorted by offsetTop
// loop backwards, from bottom-most message (as that is the most common case)
for (let i = messages.length - 1; i >= 0; --i) {
if (!(messages[i] as HTMLElement).dataset.scrollTokens) {
const htmlMessage = messages[i] as HTMLElement;
if (!htmlMessage.dataset?.scrollTokens) { // dataset is only specified on HTMLElements
continue;
}
node = messages[i];
node = htmlMessage;
// break at the first message (coming from the bottom)
// that has it's offsetTop above the bottom of the viewport.
if (this.topFromBottom(node) > viewportBottom) {
Expand All @@ -661,8 +669,8 @@ export default class ScrollPanel extends React.Component<IProps> {
debuglog("unable to save scroll state: found no children in the viewport");
return;
}
const scrollToken = node.dataset.scrollTokens.split(',')[0];
debuglog("saving anchored scroll state to message", node.innerText, scrollToken);
const scrollToken = node!.dataset.scrollTokens.split(',')[0];
debuglog("saving anchored scroll state to message", scrollToken);
const bottomOffset = this.topFromBottom(node);
this.scrollState = {
stuckAtBottom: false,
Expand Down Expand Up @@ -714,12 +722,14 @@ export default class ScrollPanel extends React.Component<IProps> {
if (this.scrollTimeout.isRunning()) {
debuglog("updateHeight waiting for scrolling to end ... ");
await this.scrollTimeout.finished();
debuglog("updateHeight actually running now");
} else {
debuglog("updateHeight getting straight to business, no scrolling going on.");
debuglog("updateHeight running without delay");
}

// We might have unmounted since the timer finished, so abort if so.
if (this.unmounted) {
debuglog("updateHeight: abort due to unmount");
return;
}

Expand Down Expand Up @@ -768,32 +778,32 @@ export default class ScrollPanel extends React.Component<IProps> {
}
}

private getTrackedNode(): HTMLElement {
private getTrackedNode(): HTMLElement | undefined {
const scrollState = this.scrollState;
const trackedNode = scrollState.trackedNode;

if (!trackedNode?.parentElement) {
let node: HTMLElement;
let node: HTMLElement | undefined = undefined;
const messages = this.itemlist.current.children;
const scrollToken = scrollState.trackedScrollToken;

for (let i = messages.length - 1; i >= 0; --i) {
const m = messages[i] as HTMLElement;
// 'data-scroll-tokens' is a DOMString of comma-separated scroll tokens
// There might only be one scroll token
if (m.dataset.scrollTokens?.split(',').includes(scrollToken)) {
if (scrollToken && m.dataset.scrollTokens?.split(',').includes(scrollToken!)) {
node = m;
break;
}
}
if (node) {
debuglog("had to find tracked node again for " + scrollState.trackedScrollToken);
debuglog("had to find tracked node again for token:", scrollState.trackedScrollToken);
}
scrollState.trackedNode = node;
}

if (!scrollState.trackedNode) {
debuglog("No node with ; '"+scrollState.trackedScrollToken+"'");
debuglog("No node with token:", scrollState.trackedScrollToken);
return;
}

Expand Down Expand Up @@ -842,7 +852,7 @@ export default class ScrollPanel extends React.Component<IProps> {
};

/**
Mark the bottom offset of the last tile so we can balance it out when
Mark the bottom offset of the last tile, so we can balance it out when
anything below it changes, by calling updatePreventShrinking, to keep
the same minimum bottom offset, effectively preventing the timeline to shrink.
*/
Expand Down Expand Up @@ -921,7 +931,7 @@ export default class ScrollPanel extends React.Component<IProps> {
}
};

render() {
public render(): ReactNode {
// TODO: the classnames on the div and ol could do with being updated to
// reflect the fact that we don't necessarily contain a list of messages.
// it's not obvious why we have a separate div and ol anyway.
Expand Down

0 comments on commit 09282d9

Please sign in to comment.