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

Commit

Permalink
Fix progress bar regression throughout the app (#9219)
Browse files Browse the repository at this point in the history
* Fix useSmoothAnimation enablement not working properly by getting rid of it

Passing duration=0 is more logical and less superfluous

* Refactor UploadBar to handle state more correctly

* Change ProgressBar to new useSmoothAnimation signature and default animated to true for consistency

* Add type guard

* Make types stricter

* Write tests for the ProgressBar component

* Make the new test conform to tsc --strict

* Update UploadBar.tsx

* Update UploadBar.tsx

* Update UploadBar.tsx

(cherry picked from commit 9b99c96)
  • Loading branch information
t3chguy authored and github-actions[bot] committed Aug 25, 2022
1 parent 0a0a46c commit 1ec61ba
Show file tree
Hide file tree
Showing 5 changed files with 105 additions and 37 deletions.
75 changes: 46 additions & 29 deletions src/components/structures/UploadBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,41 +17,55 @@ limitations under the License.
import React from 'react';
import { Room } from "matrix-js-sdk/src/models/room";
import filesize from "filesize";
import { IEventRelation } from 'matrix-js-sdk/src/matrix';
import { IAbortablePromise, IEventRelation } from 'matrix-js-sdk/src/matrix';
import { Optional } from "matrix-events-sdk";

import ContentMessages from '../../ContentMessages';
import dis from "../../dispatcher/dispatcher";
import { _t } from '../../languageHandler';
import { ActionPayload } from "../../dispatcher/payloads";
import { Action } from "../../dispatcher/actions";
import ProgressBar from "../views/elements/ProgressBar";
import AccessibleButton from "../views/elements/AccessibleButton";
import AccessibleButton, { ButtonEvent } from "../views/elements/AccessibleButton";
import { IUpload } from "../../models/IUpload";
import MatrixClientContext from "../../contexts/MatrixClientContext";
import { ActionPayload } from '../../dispatcher/payloads';
import { UploadPayload } from "../../dispatcher/payloads/UploadPayload";

interface IProps {
room: Room;
relation?: IEventRelation;
}

interface IState {
currentUpload?: IUpload;
uploadsHere: IUpload[];
currentFile?: string;
currentPromise?: IAbortablePromise<any>;
currentLoaded?: number;
currentTotal?: number;
countFiles: number;
}

export default class UploadBar extends React.Component<IProps, IState> {
function isUploadPayload(payload: ActionPayload): payload is UploadPayload {
return [
Action.UploadStarted,
Action.UploadProgress,
Action.UploadFailed,
Action.UploadFinished,
Action.UploadCanceled,
].includes(payload.action as Action);
}

export default class UploadBar extends React.PureComponent<IProps, IState> {
static contextType = MatrixClientContext;

private dispatcherRef: string;
private mounted: boolean;
private dispatcherRef: Optional<string>;
private mounted = false;

constructor(props) {
super(props);

// Set initial state to any available upload in this room - we might be mounting
// earlier than the first progress event, so should show something relevant.
const uploadsHere = this.getUploadsInRoom();
this.state = { currentUpload: uploadsHere[0], uploadsHere };
this.state = this.calculateState();
}

componentDidMount() {
Expand All @@ -61,53 +75,56 @@ export default class UploadBar extends React.Component<IProps, IState> {

componentWillUnmount() {
this.mounted = false;
dis.unregister(this.dispatcherRef);
dis.unregister(this.dispatcherRef!);
}

private getUploadsInRoom(): IUpload[] {
const uploads = ContentMessages.sharedInstance().getCurrentUploads(this.props.relation);
return uploads.filter(u => u.roomId === this.props.room.roomId);
}

private calculateState(): IState {
const [currentUpload, ...otherUploads] = this.getUploadsInRoom();
return {
currentFile: currentUpload?.fileName,
currentPromise: currentUpload?.promise,
currentLoaded: currentUpload?.loaded,
currentTotal: currentUpload?.total,
countFiles: otherUploads.length + 1,
};
}

private onAction = (payload: ActionPayload) => {
switch (payload.action) {
case Action.UploadStarted:
case Action.UploadProgress:
case Action.UploadFinished:
case Action.UploadCanceled:
case Action.UploadFailed: {
if (!this.mounted) return;
const uploadsHere = this.getUploadsInRoom();
this.setState({ currentUpload: uploadsHere[0], uploadsHere });
break;
}
if (!this.mounted) return;
if (isUploadPayload(payload)) {
this.setState(this.calculateState());
}
};

private onCancelClick = (ev) => {
private onCancelClick = (ev: ButtonEvent) => {
ev.preventDefault();
ContentMessages.sharedInstance().cancelUpload(this.state.currentUpload.promise, this.context);
ContentMessages.sharedInstance().cancelUpload(this.state.currentPromise!, this.context);
};

render() {
if (!this.state.currentUpload) {
if (!this.state.currentFile) {
return null;
}

// MUST use var name 'count' for pluralization to kick in
const uploadText = _t(
"Uploading %(filename)s and %(count)s others", {
filename: this.state.currentUpload.fileName,
count: this.state.uploadsHere.length - 1,
filename: this.state.currentFile,
count: this.state.countFiles - 1,
},
);

const uploadSize = filesize(this.state.currentUpload.total);
const uploadSize = filesize(this.state.currentTotal!);
return (
<div className="mx_UploadBar">
<div className="mx_UploadBar_filename">{ uploadText } ({ uploadSize })</div>
<AccessibleButton onClick={this.onCancelClick} className='mx_UploadBar_cancel' />
<ProgressBar value={this.state.currentUpload.loaded} max={this.state.currentUpload.total} />
<ProgressBar value={this.state.currentLoaded!} max={this.state.currentTotal!} />
</div>
);
}
Expand Down
4 changes: 2 additions & 2 deletions src/components/views/elements/ProgressBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ interface IProps {
}

const PROGRESS_BAR_ANIMATION_DURATION = 300;
const ProgressBar: React.FC<IProps> = ({ value, max, animated }) => {
const ProgressBar: React.FC<IProps> = ({ value, max, animated = true }) => {
// Animating progress bars via CSS transition isn’t possible in all of our supported browsers yet.
// As workaround, we’re using animations through JS requestAnimationFrame
const currentValue = useSmoothAnimation(0, value, PROGRESS_BAR_ANIMATION_DURATION, animated);
const currentValue = useSmoothAnimation(0, value, animated ? PROGRESS_BAR_ANIMATION_DURATION : 0);
return <progress className="mx_ProgressBar" max={max} value={currentValue} />;
};

Expand Down
2 changes: 1 addition & 1 deletion src/dispatcher/payloads/UploadPayload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { ActionPayload } from "../payloads";
import { Action } from "../actions";
import { IUpload } from "../../models/IUpload";

interface UploadPayload extends ActionPayload {
export interface UploadPayload extends ActionPayload {
/**
* The upload with fields representing the new upload state.
*/
Expand Down
8 changes: 3 additions & 5 deletions src/hooks/useSmoothAnimation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,12 @@ const debuglog = (...args: any[]) => {
* Utility function to smoothly animate to a certain target value
* @param initialValue Initial value to be used as initial starting point
* @param targetValue Desired value to animate to (can be changed repeatedly to whatever is current at that time)
* @param duration Duration that each animation should take
* @param enabled Whether the animation should run or not
* @param duration Duration that each animation should take, specify 0 to skip animating
*/
export function useSmoothAnimation(
initialValue: number,
targetValue: number,
duration: number,
enabled: boolean,
): number {
const state = useRef<{ timestamp: DOMHighResTimeStamp | null, value: number }>({
timestamp: null,
Expand Down Expand Up @@ -79,7 +77,7 @@ export function useSmoothAnimation(
[currentStepSize, targetValue],
);

useAnimation(enabled, update);
useAnimation(duration > 0, update);

return currentValue;
return duration > 0 ? currentValue : targetValue;
}
53 changes: 53 additions & 0 deletions test/components/views/elements/ProgressBar-test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
Copyright 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.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import React from "react";
import { act, render } from "@testing-library/react";

import ProgressBar from "../../../../src/components/views/elements/ProgressBar";

jest.useFakeTimers();

describe("<ProgressBar/>", () => {
it("works when animated", () => {
const { container, rerender } = render(<ProgressBar max={100} value={50} animated={true} />);
const progress = container.querySelector<HTMLProgressElement>("progress")!;

// The animation always starts from 0
expect(progress.value).toBe(0);

// Await the animation to conclude to our initial value of 50
act(() => { jest.runAllTimers(); });
expect(progress.position).toBe(0.5);

// Move the needle to 80%
rerender(<ProgressBar max={100} value={80} animated={true} />);
expect(progress.position).toBe(0.5);

// Let the animaiton run a tiny bit, assert it has moved from where it was to where it needs to go
act(() => { jest.advanceTimersByTime(150); });
expect(progress.position).toBeGreaterThan(0.5);
expect(progress.position).toBeLessThan(0.8);
});

it("works when not animated", () => {
const { container, rerender } = render(<ProgressBar max={100} value={50} animated={false} />);
const progress = container.querySelector<HTMLProgressElement>("progress")!;

// Without animation all positional updates are immediate, not requiring timers to run
expect(progress.position).toBe(0.5);
rerender(<ProgressBar max={100} value={80} animated={false} />);
expect(progress.position).toBe(0.8);
});
});

0 comments on commit 1ec61ba

Please sign in to comment.