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

Colorful progress bars #82

Closed
wants to merge 2 commits into from
Closed

Colorful progress bars #82

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
33172ab
Implement colorful progress bars
melyux Sep 20, 2023
d992cd5
Simplify to using ProgressBar status enum
melyux Sep 20, 2023
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
4 changes: 3 additions & 1 deletion src/components/createtorrentform.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { Box, Button, Checkbox, Flex, Group, Slider, Text, TextInput, Textarea,
import { useForm } from "@mantine/form";
import { dialog } from "@tauri-apps/api";
import React, { useCallback, useEffect, useRef, useState } from "react";
import { Status } from "rpc/transmission";
import { appVersion } from "./modals/version";
import { ProgressBar } from "./progressbar";
const { appWindow, invoke } = await import(/* webpackChunkName: "taurishim" */"taurishim");
Expand Down Expand Up @@ -261,7 +262,8 @@ export default function CreateTorrentForm() {
now={pieces.done}
max={Math.max(pieces.total, 1)}
label={`Hashing, done ${pieces.done} of ${pieces.total}`}
animate />}
animate
status={Status.verifying} />}
{state.state === "done" &&
<Text>{`Torrent infohash: ${state.hash}`}</Text>}
</Box>
Expand Down
5 changes: 4 additions & 1 deletion src/components/details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,14 @@ interface DetailsProps {
function DownloadBar(props: { torrent: Torrent }) {
let prefix = "";
let percent = props.torrent.percentDone as number;
let status = props.torrent.status;
if (props.torrent.status === Status.verifying) {
prefix = "Verified";
percent = props.torrent.recheckProgress;
} else if (props.torrent.status === Status.downloading && props.torrent.pieceCount === 0) {
prefix = "Downloading metadata";
percent = props.torrent.metadataPercentComplete;
status = Status.magnetizing;
} else if (props.torrent.status === Status.stopped) {
prefix = "Stopped";
} else {
Expand All @@ -57,9 +59,10 @@ function DownloadBar(props: { torrent: Torrent }) {

const now = Math.floor(percent * 1000);
const nowStr = `${prefix}: ${now / 10}%`;
const active = props.torrent.rateDownload > 0 || props.torrent.rateUpload > 0;
return (
<Box w="100%" my="0.5rem">
<ProgressBar now={now} max={1000} label={nowStr} />
<ProgressBar now={now} max={1000} label={nowStr} animate={active} status={status} />
</Box>
);
}
Expand Down
9 changes: 8 additions & 1 deletion src/components/modals/interfacepanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
*/

import React from "react";
import { Checkbox, Grid, NumberInput, Textarea } from "@mantine/core";
import { Checkbox, Grid, NumberInput, Switch, Textarea } from "@mantine/core";
import type { UseFormReturnType } from "@mantine/form";

export interface InterfaceFormValues {
Expand All @@ -28,13 +28,20 @@ export interface InterfaceFormValues {
},
}

const bigSwitchStyles = { track: { flexGrow: 1 } };

export function InterfaceSettigsPanel<V extends InterfaceFormValues>(props: { form: UseFormReturnType<V> }) {
return (
<Grid>
<Grid.Col>
<Checkbox label="Skip add torrent dialog"
{...props.form.getInputProps("interface.skipAddDialog", { type: "checkbox" })} />
</Grid.Col>
<Grid.Col span={8}>Colorful progress bars</Grid.Col>
<Grid.Col span={2}>
<Switch onLabel="ON" offLabel="OFF" size="xl" styles={bigSwitchStyles}
{...props.form.getInputProps("interface.colorfulProgressBars", { type: "checkbox" })} />
</Grid.Col>
<Grid.Col span={8}>Max number of saved download directories</Grid.Col>
<Grid.Col span={2}>
<NumberInput
Expand Down
31 changes: 28 additions & 3 deletions src/components/progressbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,46 @@
*/

import "../css/progressbar.css";
import React from "react";
import React, { useContext } from "react";
import { ConfigContext } from "../config";
import { Status } from "../rpc/transmission";

interface ProgressBarProps {
now: number,
max?: number,
label?: string,
animate?: boolean,
status?: number,
Copy link
Member

Choose a reason for hiding this comment

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

Status logic should not be in ProgressBar component because it is generic. It should have an enum variant prop that gives it correct color, determine the variant in parent component that knows what status means, let progressbar handle the color based on variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kind of confused on this one. Do you mean the ProgressBar component should have an enum for the possible colors, and everything that calls ProgressBar should manually convert e.g. the torrent status into a ProgressBarColor? Which would mean repeating the code between torrenttable.tsx and details.tsx?

Copy link
Member

Choose a reason for hiding this comment

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

No need to repeat the code, extract it into a helper function, you can put it in torrent.ts.

className?: string,
}

export function ProgressBar(props: ProgressBarProps) {
const max = props.max ?? 100;
const percent = Math.floor(1000 * props.now / max) / 10;
const label = props.label ?? `${percent}%`;
const className = `progressbar ${props.animate === true ? "animate" : ""} ${props.className ?? ""}`;
const label = props.label || `${percent}%`;

Check failure on line 36 in src/components/progressbar.tsx

View workflow job for this annotation

GitHub Actions / eslint 

[eslint] reported by reviewdog 🐶
Unexpected nullable string value in conditional. Please handle the nullish/empty cases explicitly.

Raw Output:
{"ruleId":"@typescript-eslint/strict-boolean-expressions","severity":2,"message":"Unexpected nullable string value in conditional. Please handle the nullish/empty cases explicitly.","line":36,"column":19,"nodeType":"MemberExpression","messageId":"conditionErrorNullableString","endLine":36,"endColumn":30,"suggestions":[{"messageId":"conditionFixCompareNullish","fix":{"range":[1283,1294],"text":"(props.label != null)"},"desc":"Change condition to check for null/undefined (`value != null`)"},{"messageId":"conditionFixDefaultEmptyString","fix":{"range":[1283,1294],"text":"(props.label ?? \"\")"},"desc":"Explicitly treat nullish value the same as an empty string (`value ?? \"\"`)"},{"messageId":"conditionFixCastBoolean","fix":{"range":[1283,1294],"text":"(Boolean(props.label))"},"desc":"Explicitly cast value to a boolean (`Boolean(value)`)"}]}

Check failure on line 36 in src/components/progressbar.tsx

View workflow job for this annotation

GitHub Actions / eslint 

[eslint] reported by reviewdog 🐶
Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.

Raw Output:
{"ruleId":"@typescript-eslint/prefer-nullish-coalescing","severity":2,"message":"Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.","line":36,"column":31,"nodeType":"Punctuator","messageId":"preferNullishOverOr","endLine":36,"endColumn":33,"suggestions":[{"messageId":"suggestNullish","fix":{"range":[1295,1297],"text":"??"},"desc":"Fix to nullish coalescing operator (`??`)."}]}
const animate = (props.animate && props.status !== Status.queuedToVerify &&

Check failure on line 37 in src/components/progressbar.tsx

View workflow job for this annotation

GitHub Actions / eslint 

[eslint] reported by reviewdog 🐶
Unexpected nullable boolean value in conditional. Please handle the nullish case explicitly.

Raw Output:
{"ruleId":"@typescript-eslint/strict-boolean-expressions","severity":2,"message":"Unexpected nullable boolean value in conditional. Please handle the nullish case explicitly.","line":37,"column":22,"nodeType":"MemberExpression","messageId":"conditionErrorNullableBoolean","endLine":37,"endColumn":35,"suggestions":[{"messageId":"conditionFixDefaultFalse","fix":{"range":[1334,1347],"text":"(props.animate ?? false)"},"desc":"Explicitly treat nullish value the same as false (`value ?? false`)"},{"messageId":"conditionFixCompareTrue","fix":{"range":[1334,1347],"text":"(props.animate === true)"},"desc":"Change condition to check if true (`value === true`)"}]}
Copy link
Member

Choose a reason for hiding this comment

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

Animate prop should also be taken as is. Determine whether progress bar should be animated in the parent.

props.status !== Status.queuedToDownload && props.status !== Status.queuedToSeed) ||

Check failure on line 38 in src/components/progressbar.tsx

View workflow job for this annotation

GitHub Actions / eslint 

[eslint] reported by reviewdog 🐶
Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.

Raw Output:
{"ruleId":"@typescript-eslint/prefer-nullish-coalescing","severity":2,"message":"Prefer using nullish coalescing operator (`??`) instead of a logical or (`||`), as it is a safer operator.","line":38,"column":91,"nodeType":"Punctuator","messageId":"preferNullishOverOr","endLine":38,"endColumn":93,"suggestions":[{"messageId":"suggestNullish","fix":{"range":[1445,1528],"text":"(props.status !== Status.queuedToSeed) ??\n        props.status == Status.magnetizing)"},"desc":"Fix to nullish coalescing operator (`??`)."}]}
props.status == Status.magnetizing || props.status == Status.verifying;

Check failure on line 39 in src/components/progressbar.tsx

View workflow job for this annotation

GitHub Actions / eslint 

[eslint] reported by reviewdog 🐶
Expected '===' and instead saw '=='.

Raw Output:
{"ruleId":"eqeqeq","severity":2,"message":"Expected '===' and instead saw '=='.","line":39,"column":22,"nodeType":"BinaryExpression","messageId":"unexpected","endLine":39,"endColumn":24}

Check failure on line 39 in src/components/progressbar.tsx

View workflow job for this annotation

GitHub Actions / eslint 

[eslint] reported by reviewdog 🐶
Expected '===' and instead saw '=='.

Raw Output:
{"ruleId":"eqeqeq","severity":2,"message":"Expected '===' and instead saw '=='.","line":39,"column":60,"nodeType":"BinaryExpression","messageId":"unexpected","endLine":39,"endColumn":62}
let color = "blue";

const config = useContext(ConfigContext);
Copy link
Member

Choose a reason for hiding this comment

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

Move the config checks outside as well.

Basically this should be a dumb generic reusable component that just draws what it's told.

const colorize = config.values.interface.colorfulProgressBars;
if (colorize) {
if (props.status == Status.error) {

Check failure on line 45 in src/components/progressbar.tsx

View workflow job for this annotation

GitHub Actions / eslint 

[eslint] reported by reviewdog 🐶
Expected '===' and instead saw '=='.

Raw Output:
{"ruleId":"eqeqeq","severity":2,"message":"Expected '===' and instead saw '=='.","line":45,"column":26,"nodeType":"BinaryExpression","messageId":"unexpected","endLine":45,"endColumn":28}
color = "dark-red";
} else if (props.status == Status.magnetizing) {

Check failure on line 47 in src/components/progressbar.tsx

View workflow job for this annotation

GitHub Actions / eslint 

[eslint] reported by reviewdog 🐶
Expected '===' and instead saw '=='.

Raw Output:
{"ruleId":"eqeqeq","severity":2,"message":"Expected '===' and instead saw '=='.","line":47,"column":33,"nodeType":"BinaryExpression","messageId":"unexpected","endLine":47,"endColumn":35}
color = "red";
} else if (props.status == Status.stopped) {

Check failure on line 49 in src/components/progressbar.tsx

View workflow job for this annotation

GitHub Actions / eslint 

[eslint] reported by reviewdog 🐶
Expected '===' and instead saw '=='.

Raw Output:
{"ruleId":"eqeqeq","severity":2,"message":"Expected '===' and instead saw '=='.","line":49,"column":33,"nodeType":"BinaryExpression","messageId":"unexpected","endLine":49,"endColumn":35}
color = "dark-green";
} else if (props.status == Status.seeding || props.status == Status.downloading && percent == 100) {
color = "green";
} else if (props.status == Status.queuedToVerify || props.status == Status.queuedToDownload ||
props.status == Status.queuedToSeed) {
color = "dark-blue";
} // Waiting and Downloading @ <=99% will default to plain blue
}

const className = `progressbar ${animate === true ? "animate" : ""} ${color} ${props.className ?? ""}`;
return (
<div className={className}>
<div>{label}</div>
Expand Down
4 changes: 2 additions & 2 deletions src/components/tables/filetreetable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import type { Row, ColumnDef, CellContext } from "@tanstack/react-table";
import type { CachedFileTree, FileDirEntry } from "../../cachedfiletree";
import { isDirEntry } from "../../cachedfiletree";
import { ConfigContext, ServerConfigContext } from "../../config";
import { PriorityColors, PriorityStrings } from "../../rpc/transmission";
import { Status, PriorityColors, PriorityStrings } from "../../rpc/transmission";
import { bytesToHumanReadableStr, pathMapFromServer } from "../../trutil";
import { ProgressBar } from "../progressbar";
import * as Icon from "react-bootstrap-icons";
Expand Down Expand Up @@ -136,7 +136,7 @@ function ByteSizeField(props: TableFieldProps) {
function PercentBarField(props: TableFieldProps) {
const now = props.entry.percent ?? 0;

return <ProgressBar now={now} className="white-outline" />;
return <ProgressBar now={now} status={Status.downloading} className="white-outline" />;
Copy link
Member

Choose a reason for hiding this comment

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

Keep this blue.
Or, if you want to get colorful here too then make skipped files grey, downloading ones blue and finished ones green to be consistent with torrent table.

}

function PriorityField(props: TableFieldProps) {
Expand Down
4 changes: 3 additions & 1 deletion src/components/tables/peerstable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import type { AccessorFn, CellContext, ColumnDef } from "@tanstack/react-table";
import React, { useMemo, useCallback } from "react";
import type { Torrent, PeerStats } from "rpc/torrent";
import { Status } from "../../rpc/transmission";
import { bytesToHumanReadableStr } from "trutil";
import { TrguiTable, useStandardSelect } from "./common";
import { ProgressBar } from "components/progressbar";
Expand Down Expand Up @@ -80,7 +81,8 @@ function PercentField(props: TableFieldProps) {
return <ProgressBar
now={now}
className="white-outline"
animate={active} />;
animate={active}
status={Status.downloading} />;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, either keep this unchanged or keep consistent logic.

}

const Columns = AllFields.map((field): ColumnDef<PeerStats> => {
Expand Down
15 changes: 13 additions & 2 deletions src/components/tables/torrenttable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,13 +299,24 @@ function ByteRateField(props: TableFieldProps) {
}

function PercentBarField(props: TableFieldProps) {
const now = props.torrent[props.fieldName] * 100;
let now: number = props.torrent[props.fieldName] * 100;
let label: string = '';
let status: number = props.torrent.status;
if ((props.torrent.error !== undefined && props.torrent.error > 0) || props.torrent.cachedError !== "") {
status = Status.error;
} else if (props.torrent.status === Status.downloading && props.torrent.pieceCount === 0) {
now = 100;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is leftover from testing or you intend to always show full progress bar. I'm inclined to not show it, even if it makes the color of the progress bar rarely visible because it usually goes from 0 to downloading in a single update.

Copy link
Contributor Author

@melyux melyux Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The problem with the Magnetizing one is that I think you said the Magnetization progress is not grabbed at the full table level, only at the per-torrent level. So the Magnetization process will never be shown, and so the colors will never be shown. I felt it's better to have the bar be full all the time to at least show that this torrent is being Magnetized.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, showing full animated bar with no percentage text will be fine.

label = "🧲";
Copy link
Member

Choose a reason for hiding this comment

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

No emojis and no custom labels here, the icon in name field is enough indication.

status = Status.magnetizing;
}
const active = props.torrent.rateDownload > 0 || props.torrent.rateUpload > 0;

return <ProgressBar
now={now}
className="white-outline"
animate={active} />;
label={label}
animate={active}
status={status} />;
}

const Columns = AllFields.map((f): ColumnDef<Torrent> => {
Expand Down
2 changes: 2 additions & 0 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ interface Settings {
showFilesSearchBox: boolean,
mainSplit: SplitType,
skipAddDialog: boolean,
colorfulProgressBars: boolean,
numLastSaveDirs: number,
defaultTrackers: string[],
},
Expand Down Expand Up @@ -210,6 +211,7 @@ const DefaultSettings: Settings = {
showFilesSearchBox: false,
mainSplit: "vertical",
skipAddDialog: false,
colorfulProgressBars: true,
melyux marked this conversation as resolved.
Show resolved Hide resolved
numLastSaveDirs: 20,
defaultTrackers: [...DefaultTrackerList],
},
Expand Down
21 changes: 21 additions & 0 deletions src/css/progressbar.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,27 @@
transition: clip-path 0.5s ease;
}

.progressbar.dark-blue>:first-child {
background: #0054ae;
}

.progressbar.green>:first-child {
background: #36B24D;
}

.progressbar.dark-green>:first-child {
background: #1d8931;
}

.progressbar.red>:first-child {
background: #FA5352;
}

.progressbar.dark-red>:first-child {
background: #C92B2A;
}
Comment on lines +36 to +54
Copy link
Member

Choose a reason for hiding this comment

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

The colors are fine but they are not from the default palette and stand out.
This and the default blue should be moved from css file into inline css in jsx and use the MantineTheme colors.
If you are not sure how to do that I can move current styles there and you can build on top of it.



.progressbar.animate>:first-child {
background-image: linear-gradient(45deg, rgba(255, 255, 255, .15) 25%, transparent 25%, transparent 50%, rgba(255, 255, 255, .15) 50%, rgba(255, 255, 255, .15) 75%, transparent 75%, transparent);
background-size: 1rem 1rem;
Expand Down
4 changes: 4 additions & 0 deletions src/rpc/transmission.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ export const Status = {
downloading: 4,
queuedToSeed: 5,
seeding: 6,
magnetizing: -1,
error: -2,
Comment on lines +32 to +33
Copy link
Member

Choose a reason for hiding this comment

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

This file should not be changed at all. It is meant to directly map to transmission API.

Once you move the status out of ProgressBar you will likely find that you don't need these fake statuses here.

} as const;

export const StatusStrings = [
Expand All @@ -39,6 +41,8 @@ export const StatusStrings = [
"Downloading",
"Waiting",
"Seeding",
"Magnetizing",
"Error",
] as const;

const PriorityNumbers = [-1, 0, 1] as const;
Expand Down
Loading