From 2b56e91d676d5f5b17d06666e471101c60680992 Mon Sep 17 00:00:00 2001 From: mxtmx <142958831+mxtmx@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:17:46 -0700 Subject: [PATCH 1/2] Add per-line colors and layer ordering to Graph View --- client/src/assets/icons/drag_indicator.svg | 4 + .../views/GraphView/ColorPicker.tsx | 174 ++++++++++++ .../src/components/views/GraphView/Graph.ts | 106 +++++--- .../views/GraphView/GraphCanvas.jsx | 2 + .../views/GraphView/GraphSeriesList.tsx | 161 +++++++++++ .../components/views/GraphView/GraphView.tsx | 249 ++++++++++++++---- .../views/GraphView/MultipleCheckbox.jsx | 75 ------ .../views/GraphView/MultipleCheckbox.tsx | 53 ++++ .../src/components/views/GraphView/colors.ts | 73 +++++ 9 files changed, 743 insertions(+), 154 deletions(-) create mode 100644 client/src/assets/icons/drag_indicator.svg create mode 100644 client/src/components/views/GraphView/ColorPicker.tsx create mode 100644 client/src/components/views/GraphView/GraphSeriesList.tsx delete mode 100644 client/src/components/views/GraphView/MultipleCheckbox.jsx create mode 100644 client/src/components/views/GraphView/MultipleCheckbox.tsx create mode 100644 client/src/components/views/GraphView/colors.ts diff --git a/client/src/assets/icons/drag_indicator.svg b/client/src/assets/icons/drag_indicator.svg new file mode 100644 index 000000000..f42659c54 --- /dev/null +++ b/client/src/assets/icons/drag_indicator.svg @@ -0,0 +1,4 @@ + + + + diff --git a/client/src/components/views/GraphView/ColorPicker.tsx b/client/src/components/views/GraphView/ColorPicker.tsx new file mode 100644 index 000000000..ca63dcb14 --- /dev/null +++ b/client/src/components/views/GraphView/ColorPicker.tsx @@ -0,0 +1,174 @@ +import { useEffect, useId, useRef, useState } from 'react'; +import clsx from 'clsx'; + +import { normalizeHexColor, sameColor, SWATCH_COLORS } from './colors'; + +type ColorSwatchButtonProps = { + color: string; + title: string; + expanded: boolean; + onClick: () => void; +}; + +export const ColorSwatchButton = ({ + color, + title, + expanded, + onClick, +}: ColorSwatchButtonProps) => ( + +); + +type ColorPaletteProps = { + color: string; + onChange: (color: string) => void; + onReset: () => void; + onClose: () => void; +}; + +export const ColorPalette = ({ + color, + onChange, + onReset, + onClose, +}: ColorPaletteProps) => { + const id = useId(); + + const [hexInput, setHexInput] = useState(color); + const hexValid = normalizeHexColor(hexInput) !== null; + + // Leave the field alone while it already spells the current color, or + // normalizing '#abc' under the cursor makes shorthand impossible to type. + useEffect( + () => + setHexInput((current) => + normalizeHexColor(current) !== null && sameColor(current, color) + ? current + : color, + ), + [color], + ); + + const containerRef = useRef(null); + + // Focus the palette so Escape works, and hand focus back on close so the + // view's shortcuts still reach. + useEffect(() => { + const previous = document.activeElement; + containerRef.current?.focus(); + + return () => { + // Only when closing dropped focus on the floor; switching rows leaves + // the other row's swatch focused. + const active = document.activeElement; + if (active !== null && active !== document.body) return; + + if (previous instanceof HTMLElement && previous.isConnected) + previous.focus(); + }; + }, []); + + const handleHexChange = (raw: string) => { + setHexInput(raw); + + const normalized = normalizeHexColor(raw); + if (normalized !== null) onChange(normalized); + }; + + return ( +
{ + if (evt.key === 'Escape') { + evt.stopPropagation(); + onClose(); + } + }} + > +
+ {SWATCH_COLORS.map((swatch) => { + const selected = sameColor(swatch, color); + + return ( +
+ +
+ + handleHexChange(evt.target.value)} + /> + handleHexChange(evt.target.value)} + /> + +
+
+ ); +}; diff --git a/client/src/components/views/GraphView/Graph.ts b/client/src/components/views/GraphView/Graph.ts index 125b6aae1..6e60a7530 100644 --- a/client/src/components/views/GraphView/Graph.ts +++ b/client/src/components/views/GraphView/Graph.ts @@ -3,6 +3,11 @@ import { cloneDeep } from 'lodash'; type Options = { windowMs: number; colors: string[]; + // Layer order; the first is drawn in front and heads the key. Unlisted names + // keep arrival order, after the listed ones. + seriesOrder: string[]; + // per-series color overrides; series without one fall back to `colors` + seriesColors: { [name: string]: string }; lineWidth: number; padding: number; keySpacing: number; @@ -14,19 +19,14 @@ type Options = { maxTicks: number; }; -import twColors from 'tailwindcss/colors'; +import { DEFAULT_SERIES_COLORS } from './colors'; // all dimensions in this file are *CSS* pixels unless otherwise stated export const DEFAULT_OPTIONS: Options = { windowMs: 5000, - colors: [ - twColors['blue']['600'], - twColors['red']['600'], - twColors['green']['600'], - twColors['purple']['600'], - twColors['orange']['600'], - twColors['pink']['600'], - ], + colors: [...DEFAULT_SERIES_COLORS], + seriesOrder: [], + seriesColors: {}, lineWidth: 2, padding: 15, keySpacing: 4, @@ -208,7 +208,7 @@ export default class Graph { ctx: CanvasRenderingContext2D; options: Options; - data: { [key: string]: { ts: number[]; vs: number[]; color: string } }; + data: { [key: string]: { ts: number[]; vs: number[] } }; beginGraphNowMs = Number.NaN; // in telemetry time beginRenderTimeMs = Number.NaN; // in browser time @@ -243,9 +243,39 @@ export default class Graph { this.beginRenderTimeMs = Number.NaN; // in browser time } - add(time: number, samples: Sample[][]) { + // The first name is the topmost layer. + orderedNames() { + const { seriesOrder } = this.options; + + const names = Object.keys(this.data); + if (seriesOrder.length === 0) return names; + + const rank = (name: string) => { + const i = seriesOrder.indexOf(name); + return i === -1 ? seriesOrder.length : i; + }; + + // the insertion index keeps unlisted series in a stable relative order + return names + .map((name, i) => ({ name, i })) + .sort((a, b) => rank(a.name) - rank(b.name) || a.i - b.i) + .map(({ name }) => name); + } + + colorFor(name: string) { const o = this.options; + const override = o.seriesColors[name]; + if (override) return override; + + const orderIndex = o.seriesOrder.indexOf(name); + const index = + orderIndex === -1 ? Object.keys(this.data).indexOf(name) : orderIndex; + + return o.colors[Math.max(index, 0) % o.colors.length]; + } + + add(time: number, samples: Sample[][]) { for (const sample of samples) { const t = sample.reduce( (acc, { name, value }) => (name === 'time' ? value : acc), @@ -263,7 +293,6 @@ export default class Graph { this.data[name] = { ts: [], vs: [], - color: o.colors[Object.keys(this.data).length % o.colors.length], }; } @@ -352,13 +381,13 @@ export default class Graph { this.ctx.save(); - const names = Object.keys(this.data); + const names = this.orderedNames(); const numSets = names.length; const height = numSets * o.fontSize + (numSets - 1) * o.keySpacing; for (let i = 0; i < numSets; i++) { const lineY = y + i * (o.fontSize + o.keySpacing) + o.fontSize / 2; const name = names[i]; - const { color } = this.data[name]; + const color = this.colorFor(name); const lineWidth = this.ctx.measureText(name).width + o.keyLineLength + o.keySpacing; const lineX = x + (width - lineWidth) / 2; @@ -499,32 +528,33 @@ export default class Graph { // draw data lines // scaling is used instead of transform because of the non-uniform stretching warps the plot line + // drawn back to front so that the head of the layer order ends up on top this.ctx.beginPath(); - Object.keys(this.data).forEach((k, i) => { - const { ts, vs } = this.data[k]; - - if (ts.length === 0) return; + this.orderedNames() + .reverse() + .forEach((k) => { + const { ts, vs } = this.data[k]; - const color = o.colors[i % o.colors.length]; + if (ts.length === 0) return; - this.ctx.beginPath(); - this.ctx.strokeStyle = color; - fineMoveTo( - this.ctx, - this.scaling, - scale(ts[0] - graphNowMs + o.windowMs, 0, o.windowMs, 0, width), - scale(vs[0], axis.min, axis.max, height, 0), - ); - for (let j = 1; j < ts.length; j++) { - fineLineTo( + this.ctx.beginPath(); + this.ctx.strokeStyle = this.colorFor(k); + fineMoveTo( this.ctx, this.scaling, - scale(ts[j] - graphNowMs + o.windowMs, 0, o.windowMs, 0, width), - scale(vs[j], axis.min, axis.max, height, 0), + scale(ts[0] - graphNowMs + o.windowMs, 0, o.windowMs, 0, width), + scale(vs[0], axis.min, axis.max, height, 0), ); - } - this.ctx.stroke(); - }); + for (let j = 1; j < ts.length; j++) { + fineLineTo( + this.ctx, + this.scaling, + scale(ts[j] - graphNowMs + o.windowMs, 0, o.windowMs, 0, width), + scale(vs[j], axis.min, axis.max, height, 0), + ); + } + this.ctx.stroke(); + }); this.ctx.restore(); } @@ -535,5 +565,13 @@ export default class Graph { setOptions(options: Options) { Object.assign(this.options, options); + + // Dropped from the layer order means no longer graphed, so forget it. + const { seriesOrder } = this.options; + if (seriesOrder.length === 0) return; + + for (const name of Object.keys(this.data)) { + if (!seriesOrder.includes(name)) delete this.data[name]; + } } } diff --git a/client/src/components/views/GraphView/GraphCanvas.jsx b/client/src/components/views/GraphView/GraphCanvas.jsx index af0738df5..3c2784080 100644 --- a/client/src/components/views/GraphView/GraphCanvas.jsx +++ b/client/src/components/views/GraphView/GraphCanvas.jsx @@ -60,6 +60,8 @@ class GraphCanvas extends React.Component { renderGraph() { if (this.props.paused) { + // Option changes made while paused are visible without resuming. + this.graph.render(this.props.pausedTime); this.requestId = 0; } else { this.setState(() => ({ diff --git a/client/src/components/views/GraphView/GraphSeriesList.tsx b/client/src/components/views/GraphView/GraphSeriesList.tsx new file mode 100644 index 000000000..1a7118e7e --- /dev/null +++ b/client/src/components/views/GraphView/GraphSeriesList.tsx @@ -0,0 +1,161 @@ +import { useState } from 'react'; +import clsx from 'clsx'; + +import { ReactComponent as ExpandMoreIcon } from '@/assets/icons/expand_more.svg'; +import { ReactComponent as DragIndicatorIcon } from '@/assets/icons/drag_indicator.svg'; + +import { ColorPalette, ColorSwatchButton } from './ColorPicker'; + +// moves the item at `from` to `to`, leaving the rest in order +export function moveItem(items: T[], from: number, to: number) { + if (from === to || from < 0 || from >= items.length) return items; + + const clamped = Math.max(0, Math.min(to, items.length - 1)); + + const reordered = [...items]; + const [item] = reordered.splice(from, 1); + reordered.splice(clamped, 0, item); + + return reordered; +} + +type GraphSeriesListProps = { + // series names in layer order; the first is drawn in front of the rest + seriesKeys: string[]; + colors: { [key: string]: string }; + onReorder: (seriesKeys: string[]) => void; + onColorChange: (key: string, color: string) => void; + onColorReset: (key: string) => void; +}; + +const GraphSeriesList = ({ + seriesKeys, + colors, + onReorder, + onColorChange, + onColorReset, +}: GraphSeriesListProps) => { + // only one palette is open at a time to keep the list compact + const [openKey, setOpenKey] = useState(null); + const [dragIndex, setDragIndex] = useState(null); + const [dropIndex, setDropIndex] = useState(null); + + const reorderable = seriesKeys.length > 1; + + const move = (from: number, to: number) => { + const reordered = moveItem(seriesKeys, from, to); + if (reordered !== seriesKeys) onReorder(reordered); + }; + + const endDrag = () => { + setDragIndex(null); + setDropIndex(null); + }; + + return ( +
    { + if (evt.currentTarget.contains(evt.relatedTarget as Node | null)) + return; + + setDropIndex(null); + }} + > + {seriesKeys.map((key, i) => ( +
  • { + if (dragIndex === null) return; + + evt.preventDefault(); + evt.dataTransfer.dropEffect = 'move'; + setDropIndex(i); + }} + onDrop={(evt) => { + if (dragIndex === null) return; + + evt.preventDefault(); + move(dragIndex, i); + endDrag(); + }} + > + {/* only the row starts a drag: keeping the palette out of the + draggable subtree leaves its text field selectable */} +
    { + setDragIndex(i); + evt.dataTransfer.effectAllowed = 'move'; + // Firefox ignores drags that carry no data + evt.dataTransfer.setData('text/plain', key); + }} + onDragEnd={endDrag} + className={clsx( + 'flex items-center gap-1 rounded py-0.5 transition', + reorderable && 'cursor-grab', + dragIndex === i && 'opacity-40', + dropIndex === i && + dragIndex !== null && + dragIndex !== i && + 'bg-primary-100 dark:bg-slate-700', + )} + > + {reorderable && ( + <> +
    + + {openKey === key && ( + onColorChange(key, color)} + onReset={() => onColorReset(key)} + onClose={() => setOpenKey(null)} + /> + )} +
    • + ))} +
+ ); +}; + +export default GraphSeriesList; diff --git a/client/src/components/views/GraphView/GraphView.tsx b/client/src/components/views/GraphView/GraphView.tsx index 9a9af7180..8397973ba 100644 --- a/client/src/components/views/GraphView/GraphView.tsx +++ b/client/src/components/views/GraphView/GraphView.tsx @@ -11,18 +11,21 @@ import BaseView, { } from '@/components/views/BaseView'; import MultipleCheckbox from './MultipleCheckbox'; import GraphCanvas from './GraphCanvas'; +import GraphSeriesList from './GraphSeriesList'; import TextInput from '@/components/views/ConfigView/inputs/TextInput'; import { ReactComponent as ChartIcon } from '@/assets/icons/chart.svg'; import { ReactComponent as CloseIcon } from '@/assets/icons/close.svg'; import { ReactComponent as PlayIcon } from '@/assets/icons/play_arrow.svg'; import { ReactComponent as PauseIcon } from '@/assets/icons/pause.svg'; +import { ReactComponent as PaletteIcon } from '@/assets/icons/palette.svg'; import { RootState } from '@/store/reducers'; import { STOP_OP_MODE_TAG } from '@/store/types'; import { OpModeStatus } from '@/enums/OpModeStatus'; import { colors, ThemeConsumer } from '@/hooks/useTheme'; import { DEFAULT_OPTIONS } from './Graph'; +import { pickDefaultColor, sameColor } from './colors'; import { validateInt, ValResult } from '@/components/inputs/validation'; type GraphViewState = { @@ -31,7 +34,12 @@ type GraphViewState = { userPaused: boolean; pausedTime: number; availableKeys: string[]; + // selection doubles as the layer order: the first key is drawn in front selectedKeys: string[]; + // colors are remembered per key, including for keys that are unchecked and + // later re-checked + keyColors: { [key: string]: string }; + showSeriesSettings: boolean; windowMs: ValResult; }; @@ -59,6 +67,8 @@ class GraphView extends Component { pausedTime: 0, availableKeys: [], selectedKeys: [], + keyColors: {}, + showSeriesSettings: false, windowMs: { value: DEFAULT_OPTIONS.windowMs, valid: true, @@ -73,9 +83,22 @@ class GraphView extends Component { this.userPlay = this.userPlay.bind(this); this.userPause = this.userPause.bind(this); + this.handleSelectionChange = this.handleSelectionChange.bind(this); + this.handleReorder = this.handleReorder.bind(this); + this.handleColorChange = this.handleColorChange.bind(this); + this.handleColorReset = this.handleColorReset.bind(this); + + this.keepFocusOnView = this.keepFocusOnView.bind(this); this.handleDocumentKeydown = this.handleDocumentKeydown.bind(this); } + // Keep focus on the view, or Space after clicking "Start Graphing" would + // activate that button and stop graphing instead of toggling pause. + keepFocusOnView(evt: React.MouseEvent) { + evt.preventDefault(); + this.containerRef.current?.focus(); + } + componentDidMount() { if (this.containerRef.current) { this.containerRef.current.addEventListener( @@ -127,13 +150,74 @@ class GraphView extends Component { }); } + handleSelectionChange(selectedKeys: string[]) { + this.setState((state) => { + const keyColors = { ...state.keyColors }; + + // Only arriving keys need a color picked; only theirs can collide. + const staying = selectedKeys.filter((key) => + state.selectedKeys.includes(key), + ); + const arriving = selectedKeys.filter( + (key) => !state.selectedKeys.includes(key), + ); + + const used = staying + .map((key) => keyColors[key]) + .filter((color): color is string => color !== undefined); + + for (const key of arriving) { + // A returning key keeps its old color unless another line took it. + const remembered = keyColors[key]; + const color = + remembered !== undefined && + !used.some((taken) => sameColor(taken, remembered)) + ? remembered + : pickDefaultColor(used); + + keyColors[key] = color; + used.push(color); + } + + return { selectedKeys, keyColors }; + }); + } + + handleReorder(selectedKeys: string[]) { + this.setState({ selectedKeys }); + } + + handleColorChange(key: string, color: string) { + this.setState((state) => ({ + keyColors: { ...state.keyColors, [key]: color }, + })); + } + + handleColorReset(key: string) { + this.setState((state) => { + const used = state.selectedKeys + .filter((k) => k !== key) + .map((k) => state.keyColors[k]) + .filter((color): color is string => color !== undefined); + + return { + keyColors: { ...state.keyColors, [key]: pickDefaultColor(used) }, + }; + }); + } + handleDocumentKeydown(evt: KeyboardEvent) { + // Leave keystrokes aimed at fields and buttons alone. + const target = evt.target as HTMLElement | null; + if (target?.closest?.('input, textarea, select, [contenteditable="true"]')) + return; + if (evt.code === 'Space' && target?.closest?.('button')) return; + if (evt.code === 'Space' || evt.key === 'k') { - this.setState({ - ...this.state, - userPaused: !this.state.userPaused, - pausedTime: Date.now(), - }); + // The pause button's handlers leave pausedTime alone when already frozen, + // so the frame does not jump on the next repaint. + if (this.state.userPaused) this.userPlay(); + else this.userPause(); } } @@ -196,6 +280,32 @@ class GraphView extends Component { }); } + renderSeriesList(seriesColors: { [key: string]: string }) { + return ( + <> + + {this.state.selectedKeys.length > 1 && ( +

+ The first line is drawn in front of the others. +

+ )} + + ); + } + + // colors of the graphed keys only; the rest are remembered but unused + seriesColors() { + return Object.fromEntries( + this.state.selectedKeys.map((key) => [key, this.state.keyColors[key]]), + ); + } + render() { const showNoNumeric = !this.state.graphing && this.state.availableKeys.length === 0; @@ -218,6 +328,8 @@ class GraphView extends Component { }), ]); + const seriesColors = this.seriesColors(); + return ( { {this.state.graphing && this.state.selectedKeys.length !== 0 && ( - - {this.state.userPaused ? ( - - ) : ( - - )} - + <> + + this.setState((state) => ({ + showSeriesSettings: !state.showSeriesSettings, + })) + } + > + + + + + {this.state.userPaused ? ( + + ) : ( + + )} + + )} {this.state.graphing ? ( - + ) : ( - + )} @@ -275,12 +414,18 @@ class GraphView extends Component {
- this.setState({ selectedKeys }) - } + onChange={this.handleSelectionChange} selected={this.state.selectedKeys} />
+ {this.state.selectedKeys.length !== 0 && ( +
+

Lines:

+
+ {this.renderSeriesList(seriesColors)} +
+
+ )}

Options:

@@ -314,26 +459,40 @@ class GraphView extends Component { No telemetry selected to graph

) : ( - - {({ isDarkMode }) => ( - +
+ + {({ isDarkMode }) => ( + + )} + + {/* anchored to the top rather than full-bleed so the lines it + restyles stay visible underneath */} + {this.state.showSeriesSettings && ( +
+

Lines:

+
+ {this.renderSeriesList(seriesColors)} +
+
)} - +
)} diff --git a/client/src/components/views/GraphView/MultipleCheckbox.jsx b/client/src/components/views/GraphView/MultipleCheckbox.jsx deleted file mode 100644 index 590599e8f..000000000 --- a/client/src/components/views/GraphView/MultipleCheckbox.jsx +++ /dev/null @@ -1,75 +0,0 @@ -import React from 'react'; -import PropTypes from 'prop-types'; -import { v4 as uuid4 } from 'uuid'; - -class MultipleCheckbox extends React.Component { - constructor(props) { - super(props); - - this.state = { - selected: this.props.selected || [], - uuid: uuid4(), - }; - } - - handleChange(evt, val) { - if (evt.target.checked) { - this.setState( - { - selected: [...this.state.selected, val], - }, - () => this.props.onChange(this.state.selected), - ); - } else { - this.setState( - { - selected: this.state.selected.filter((el) => val !== el), - }, - () => this.props.onChange(this.state.selected), - ); - } - } - - render() { - return ( - - - {this.props.arr - .filter( - (val) => - !this.props.exclude || this.props.exclude.indexOf(val) === -1, - ) - .map((val) => ( - - - - - ))} - -
- this.handleChange(evt, val)} - checked={this.state.selected.indexOf(val) !== -1} - /> - - -
- ); - } -} - -MultipleCheckbox.propTypes = { - arr: PropTypes.arrayOf(PropTypes.string).isRequired, - selected: PropTypes.arrayOf(PropTypes.string), - exclude: PropTypes.arrayOf(PropTypes.string), - onChange: PropTypes.func, // TODO: fix! -}; - -export default MultipleCheckbox; diff --git a/client/src/components/views/GraphView/MultipleCheckbox.tsx b/client/src/components/views/GraphView/MultipleCheckbox.tsx new file mode 100644 index 000000000..7175ceafe --- /dev/null +++ b/client/src/components/views/GraphView/MultipleCheckbox.tsx @@ -0,0 +1,53 @@ +import { useId } from 'react'; + +type MultipleCheckboxProps = { + arr: string[]; + // fully controlled: the order of `selected` is preserved, and newly checked + // values are appended, so the caller stays in charge of the ordering + selected: string[]; + exclude?: string[]; + onChange: (selected: string[]) => void; +}; + +const MultipleCheckbox = ({ + arr, + selected, + exclude, + onChange, +}: MultipleCheckboxProps) => { + const id = useId(); + + const handleChange = (val: string, checked: boolean) => + onChange( + checked + ? [...selected.filter((el) => el !== val), val] + : selected.filter((el) => el !== val), + ); + + return ( + + + {arr + .filter((val) => !exclude || exclude.indexOf(val) === -1) + .map((val) => ( + + + + + ))} + +
+ handleChange(val, evt.target.checked)} + checked={selected.indexOf(val) !== -1} + /> + + +
+ ); +}; + +export default MultipleCheckbox; diff --git a/client/src/components/views/GraphView/colors.ts b/client/src/components/views/GraphView/colors.ts new file mode 100644 index 000000000..7997b35a7 --- /dev/null +++ b/client/src/components/views/GraphView/colors.ts @@ -0,0 +1,73 @@ +import twColors from 'tailwindcss/colors'; + +// Handed out to new lines in order. Short and high-contrast, so a handful of +// lines reads without manual picking. +export const DEFAULT_SERIES_COLORS = [ + twColors['blue']['600'], + twColors['red']['600'], + twColors['green']['600'], + twColors['purple']['600'], + twColors['orange']['600'], + twColors['pink']['600'], +]; + +const SWATCH_HUES = [ + 'red', + 'orange', + 'amber', + 'yellow', + 'lime', + 'green', + 'emerald', + 'teal', + 'cyan', + 'sky', + 'blue', + 'indigo', + 'violet', + 'purple', + 'fuchsia', + 'pink', +] as const; + +// The lighter shade reads better on the dark theme, the darker on light. +const SWATCH_SHADES = ['400', '600'] as const; + +// Shade-major, so the picker renders as two bands rather than alternating. +export const SWATCH_COLORS = SWATCH_SHADES.flatMap((shade) => + SWATCH_HUES.map((hue) => twColors[hue][shade]), +); + +const HEX_COLOR_REGEX = /^#?([0-9a-f]{3}|[0-9a-f]{6})$/i; + +// Hex only, so manual entry stays interchangeable with . +export function normalizeHexColor(raw: string): string | null { + const match = HEX_COLOR_REGEX.exec(raw.trim()); + if (match === null) return null; + + const digits = match[1].toLowerCase(); + const expanded = + digits.length === 3 + ? digits + .split('') + .map((c) => c + c) + .join('') + : digits; + + return `#${expanded}`; +} + +export function sameColor(a: string, b: string) { + return (normalizeHexColor(a) ?? a) === (normalizeHexColor(b) ?? b); +} + +export function pickDefaultColor(usedColors: string[]) { + const unused = DEFAULT_SERIES_COLORS.find( + (color) => !usedColors.some((used) => sameColor(used, color)), + ); + + return ( + unused ?? + DEFAULT_SERIES_COLORS[usedColors.length % DEFAULT_SERIES_COLORS.length] + ); +} From f37370de0d185a70422aed171d8d835b015179b7 Mon Sep 17 00:00:00 2001 From: mxtmx <142958831+mxtmx@users.noreply.github.com> Date: Wed, 23 Sep 2026 11:29:45 -0700 Subject: [PATCH 2/2] Fix Graph View line color and ordering edge cases --- .../views/GraphView/ColorPicker.tsx | 33 +++- .../src/components/views/GraphView/Graph.ts | 15 +- .../views/GraphView/GraphCanvas.jsx | 4 + .../views/GraphView/GraphSeriesList.tsx | 27 +++- .../components/views/GraphView/GraphView.tsx | 148 ++++++++++-------- 5 files changed, 142 insertions(+), 85 deletions(-) diff --git a/client/src/components/views/GraphView/ColorPicker.tsx b/client/src/components/views/GraphView/ColorPicker.tsx index ca63dcb14..d51202459 100644 --- a/client/src/components/views/GraphView/ColorPicker.tsx +++ b/client/src/components/views/GraphView/ColorPicker.tsx @@ -46,17 +46,21 @@ export const ColorPalette = ({ }: ColorPaletteProps) => { const id = useId(); - const [hexInput, setHexInput] = useState(color); - const hexValid = normalizeHexColor(hexInput) !== null; + // null shows `color` itself + const [hexInput, setHexInput] = useState(null); + const hexText = hexInput ?? color; + const hexValid = normalizeHexColor(hexText) !== null; // Leave the field alone while it already spells the current color, or // normalizing '#abc' under the cursor makes shorthand impossible to type. useEffect( () => setHexInput((current) => - normalizeHexColor(current) !== null && sameColor(current, color) + current !== null && + normalizeHexColor(current) !== null && + sameColor(current, color) ? current - : color, + : null, ), [color], ); @@ -87,6 +91,18 @@ export const ColorPalette = ({ if (normalized !== null) onChange(normalized); }; + // Drop typed text here rather than in the effect above, which runs a frame + // late and not at all when the chosen color is the current one. + const choose = (swatch: string) => { + setHexInput(null); + onChange(swatch); + }; + + const reset = () => { + setHexInput(null); + onReset(); + }; + return (
onChange(swatch)} + onClick={() => choose(swatch)} /> ); })} @@ -149,7 +165,7 @@ export const ColorPalette = ({ !hexValid && 'border-red-500 focus:border-red-500 focus:ring-red-500', )} - value={hexInput} + value={hexText} spellCheck={false} placeholder="#rrggbb" onChange={(evt) => handleHexChange(evt.target.value)} @@ -160,11 +176,12 @@ export const ColorPalette = ({ // override the resting border back to transparent className={clsx( 'shrink-0 rounded border border-gray-300 px-2 py-1 transition-colors', - 'hover:bg-gray-200 focus:outline-none focus:ring-2 focus:ring-primary-500 focus:ring-opacity-30', + 'hover:bg-gray-200 focus:outline-none', + 'focus-visible:ring-2 focus-visible:ring-primary-500 focus-visible:ring-opacity-30', 'dark:border-slate-500 dark:hover:bg-slate-700', )} title="Restore the automatically assigned color" - onClick={onReset} + onClick={reset} > Reset diff --git a/client/src/components/views/GraphView/Graph.ts b/client/src/components/views/GraphView/Graph.ts index 6e60a7530..99684dcdb 100644 --- a/client/src/components/views/GraphView/Graph.ts +++ b/client/src/components/views/GraphView/Graph.ts @@ -237,7 +237,8 @@ export default class Graph { } reset() { - this.data = {}; + // no prototype, so a series named '__proto__' is stored like any other + this.data = Object.create(null); this.beginGraphNowMs = Number.NaN; // in telemetry time this.beginRenderTimeMs = Number.NaN; // in browser time @@ -265,8 +266,8 @@ export default class Graph { colorFor(name: string) { const o = this.options; - const override = o.seriesColors[name]; - if (override) return override; + if (Object.prototype.hasOwnProperty.call(o.seriesColors, name)) + return o.seriesColors[name]; const orderIndex = o.seriesOrder.indexOf(name); const index = @@ -565,13 +566,5 @@ export default class Graph { setOptions(options: Options) { Object.assign(this.options, options); - - // Dropped from the layer order means no longer graphed, so forget it. - const { seriesOrder } = this.options; - if (seriesOrder.length === 0) return; - - for (const name of Object.keys(this.data)) { - if (!seriesOrder.includes(name)) delete this.data[name]; - } } } diff --git a/client/src/components/views/GraphView/GraphCanvas.jsx b/client/src/components/views/GraphView/GraphCanvas.jsx index 3c2784080..8d7f50392 100644 --- a/client/src/components/views/GraphView/GraphCanvas.jsx +++ b/client/src/components/views/GraphView/GraphCanvas.jsx @@ -59,6 +59,10 @@ class GraphCanvas extends React.Component { } renderGraph() { + // Option changes call this while a frame is already queued; without the + // cancel each one would leave another loop running. + if (this.requestId) cancelAnimationFrame(this.requestId); + if (this.props.paused) { // Option changes made while paused are visible without resuming. this.graph.render(this.props.pausedTime); diff --git a/client/src/components/views/GraphView/GraphSeriesList.tsx b/client/src/components/views/GraphView/GraphSeriesList.tsx index 1a7118e7e..866c9287e 100644 --- a/client/src/components/views/GraphView/GraphSeriesList.tsx +++ b/client/src/components/views/GraphView/GraphSeriesList.tsx @@ -1,4 +1,5 @@ import { useState } from 'react'; +import { flushSync } from 'react-dom'; import clsx from 'clsx'; import { ReactComponent as ExpandMoreIcon } from '@/assets/icons/expand_more.svg'; @@ -40,6 +41,9 @@ const GraphSeriesList = ({ const [dragIndex, setDragIndex] = useState(null); const [dropIndex, setDropIndex] = useState(null); + // Otherwise the palette reopens, and takes focus, when the key comes back. + if (openKey !== null && !seriesKeys.includes(openKey)) setOpenKey(null); + const reorderable = seriesKeys.length > 1; const move = (from: number, to: number) => { @@ -47,6 +51,20 @@ const GraphSeriesList = ({ if (reordered !== seriesKeys) onReorder(reordered); }; + const moveWithButton = ( + button: HTMLButtonElement, + from: number, + to: number, + ) => { + const hadFocus = button === document.activeElement; + flushSync(() => move(from, to)); + + // Landing at either end disables the button just used, dropping its focus. + const row = button.closest('li'); + if (hadFocus && button.disabled) + row?.querySelector('button:enabled')?.focus(); + }; + const endDrag = () => { setDragIndex(null); setDropIndex(null); @@ -89,8 +107,9 @@ const GraphSeriesList = ({ onDragStart={(evt) => { setDragIndex(i); evt.dataTransfer.effectAllowed = 'move'; - // Firefox ignores drags that carry no data - evt.dataTransfer.setData('text/plain', key); + // Firefox ignores drags that carry no data; a private type keeps + // text fields from taking a missed drop as typed text + evt.dataTransfer.setData('application/x-ftc-graph-series', key); }} onDragEnd={endDrag} className={clsx( @@ -115,7 +134,7 @@ const GraphSeriesList = ({ title={`Move ${key} one layer forward`} aria-label={`Move ${key} one layer forward`} disabled={i === 0} - onClick={() => move(i, i - 1)} + onClick={(evt) => moveWithButton(evt.currentTarget, i, i - 1)} > @@ -125,7 +144,7 @@ const GraphSeriesList = ({ title={`Move ${key} one layer back`} aria-label={`Move ${key} one layer back`} disabled={i === seriesKeys.length - 1} - onClick={() => move(i, i + 1)} + onClick={(evt) => moveWithButton(evt.currentTarget, i, i + 1)} > diff --git a/client/src/components/views/GraphView/GraphView.tsx b/client/src/components/views/GraphView/GraphView.tsx index 8397973ba..af216ff28 100644 --- a/client/src/components/views/GraphView/GraphView.tsx +++ b/client/src/components/views/GraphView/GraphView.tsx @@ -37,8 +37,8 @@ type GraphViewState = { // selection doubles as the layer order: the first key is drawn in front selectedKeys: string[]; // colors are remembered per key, including for keys that are unchecked and - // later re-checked - keyColors: { [key: string]: string }; + // later re-checked; a Map because keys like 'constructor' come from the robot + keyColors: ReadonlyMap; showSeriesSettings: boolean; windowMs: ValResult; }; @@ -67,7 +67,7 @@ class GraphView extends Component { pausedTime: 0, availableKeys: [], selectedKeys: [], - keyColors: {}, + keyColors: new Map(), showSeriesSettings: false, windowMs: { value: DEFAULT_OPTIONS.windowMs, @@ -96,7 +96,9 @@ class GraphView extends Component { // activate that button and stop graphing instead of toggling pause. keepFocusOnView(evt: React.MouseEvent) { evt.preventDefault(); - this.containerRef.current?.focus(); + // Scrolling the tile into view would move the button from under the + // pointer and lose the click. + this.containerRef.current?.focus({ preventScroll: true }); } componentDidMount() { @@ -152,7 +154,7 @@ class GraphView extends Component { handleSelectionChange(selectedKeys: string[]) { this.setState((state) => { - const keyColors = { ...state.keyColors }; + const keyColors = new Map(state.keyColors); // Only arriving keys need a color picked; only theirs can collide. const staying = selectedKeys.filter((key) => @@ -163,19 +165,19 @@ class GraphView extends Component { ); const used = staying - .map((key) => keyColors[key]) + .map((key) => keyColors.get(key)) .filter((color): color is string => color !== undefined); for (const key of arriving) { // A returning key keeps its old color unless another line took it. - const remembered = keyColors[key]; + const remembered = keyColors.get(key); const color = remembered !== undefined && !used.some((taken) => sameColor(taken, remembered)) ? remembered : pickDefaultColor(used); - keyColors[key] = color; + keyColors.set(key, color); used.push(color); } @@ -183,13 +185,21 @@ class GraphView extends Component { }); } - handleReorder(selectedKeys: string[]) { - this.setState({ selectedKeys }); + // Only the shown keys move; absent ones keep their slots in the selection. + handleReorder(shownKeys: string[]) { + this.setState((state) => { + let next = 0; + return { + selectedKeys: state.selectedKeys.map((key) => + shownKeys.includes(key) ? shownKeys[next++] : key, + ), + }; + }); } handleColorChange(key: string, color: string) { this.setState((state) => ({ - keyColors: { ...state.keyColors, [key]: color }, + keyColors: new Map(state.keyColors).set(key, color), })); } @@ -197,11 +207,11 @@ class GraphView extends Component { this.setState((state) => { const used = state.selectedKeys .filter((k) => k !== key) - .map((k) => state.keyColors[k]) + .map((k) => state.keyColors.get(k)) .filter((color): color is string => color !== undefined); return { - keyColors: { ...state.keyColors, [key]: pickDefaultColor(used) }, + keyColors: new Map(state.keyColors).set(key, pickDefaultColor(used)), }; }); } @@ -280,17 +290,30 @@ class GraphView extends Component { }); } - renderSeriesList(seriesColors: { [key: string]: string }) { + // Keys the current op mode is not sending stay selected, so they come back + // checked when it sends them again, but have no line to edit until then. + // Every op mode init and telemetry.clear() empties availableKeys, so until + // the next packet the selection stands in for it. + shownKeys() { + const { selectedKeys, availableKeys } = this.state; + if (availableKeys.length === 0) return selectedKeys; + return selectedKeys.filter((key) => availableKeys.includes(key)); + } + + renderSeriesList( + shownKeys: string[], + seriesColors: { [key: string]: string }, + ) { return ( <> - {this.state.selectedKeys.length > 1 && ( + {shownKeys.length > 1 && (

The first line is drawn in front of the others.

@@ -302,7 +325,9 @@ class GraphView extends Component { // colors of the graphed keys only; the rest are remembered but unused seriesColors() { return Object.fromEntries( - this.state.selectedKeys.map((key) => [key, this.state.keyColors[key]]), + [...this.state.keyColors].filter(([key]) => + this.state.selectedKeys.includes(key), + ), ); } @@ -328,6 +353,7 @@ class GraphView extends Component { }), ]); + const shownKeys = this.shownKeys(); const seriesColors = this.seriesColors(); return ( @@ -342,48 +368,46 @@ class GraphView extends Component { Graph + {this.state.graphing && shownKeys.length !== 0 && ( + + this.setState((state) => ({ + showSeriesSettings: !state.showSeriesSettings, + })) + } + > + + + )} + {this.state.graphing && this.state.selectedKeys.length !== 0 && ( - <> - - this.setState((state) => ({ - showSeriesSettings: !state.showSeriesSettings, - })) - } - > - - - - - {this.state.userPaused ? ( - - ) : ( - - )} - - + + {this.state.userPaused ? ( + + ) : ( + + )} + )} { selected={this.state.selectedKeys} />
- {this.state.selectedKeys.length !== 0 && ( + {shownKeys.length !== 0 && (

Lines:

- {this.renderSeriesList(seriesColors)} + {this.renderSeriesList(shownKeys, seriesColors)}
)} @@ -484,11 +508,11 @@ class GraphView extends Component { {/* anchored to the top rather than full-bleed so the lines it restyles stay visible underneath */} - {this.state.showSeriesSettings && ( + {this.state.showSeriesSettings && shownKeys.length !== 0 && (

Lines:

- {this.renderSeriesList(seriesColors)} + {this.renderSeriesList(shownKeys, seriesColors)}
)}