diff --git a/DESIGN.md b/DESIGN.md index ddb76f20..28e396dc 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -335,6 +335,15 @@ base**, line-height 1.6. One scale — reuse it, don't invent sizes: not qualify, however often it is visited. - **Page widths**: lists/dashboards/detail `max-w-7xl`; forms/settings `max-w-3xl`; centered, `px-4 md:px-6 py-6`. +- **Dialog widths**: the primitive's default `max-w-lg` for a confirmation or a form of + stacked single fields; **`max-w-2xl`** for a form whose fields sit side by side, because + `ClassFields`' grid splits on `md:` — a *viewport* breakpoint — so the box must be wide + enough for a split it cannot prevent; `max-w-3xl` for a dialog whose content is a grid + rather than a form (`FrameGallery`). Three sizes, and a fourth needs a reason written here + rather than picked by eye. **Any dialog whose content grows with the data carries + `max-h-[85vh] overflow-y-auto`** — `DialogContent` is centred with `-translate-y-1/2`, so + one taller than the viewport overflows off both edges and takes its own footer with it, + which is not a state a person can recover from. - **Page header**: title + subtitle left, actions right, `border-b` below, `mb-8`. - **Grids**: cards at `gap-6`, 2/3 columns by breakpoint; 16px is the default layout unit, 24px separates page sections. Detail two-column: `1fr / 320px`, stacking below @@ -916,7 +925,7 @@ The page the reference design shows (#56), with measurements verified in v1's so class selection lives. It was the side panel's Labels tab, then a `Combobox` in the centre of the top bar, and it is a **list** now — because what is being chosen between is the ontology, and a picker keeps all of it one click away, so the answer to *what can - I draw here* was never on screen. Rows carry swatch · name · geometry · hotkey badge, in + I draw here* was never on screen. Rows carry swatch · name · geometries · hotkey badge, in the **schema's authored order and only that**: a persistent list that reordered itself by recency would move rows under the cursor, and the digits are schema positions, so a recency-ordered list would print `3` against the row sitting first. `c` focuses its @@ -935,6 +944,25 @@ The page the reference design shows (#56), with measurements verified in v1's so a promise the page can keep. It stops at the job's edge, the same scope the clipboard has (#123) and for the same reason — a paste and a drawing class both belong to one pinned schema. + + **On the armed row, the geometry words are the shape picker** (#584). A class accepts a + *set* of geometries, so arming one no longer picks the shape — and until this the only + place that answer lived was the tool strip at the far left of the canvas while the class + was chosen on the right: one decision split across the width of the picture, in a loop + repeated hundreds of times a job. The active shape is lit; pressing another switches the + tool and **never the class**, which is the same retarget rule the strip holds. Only the + armed row carries it, and the accessible answer and the density answer agree: a row + ` )} diff --git a/frontend/ui-core/src/annotator/AnnotationPage.tsx b/frontend/ui-core/src/annotator/AnnotationPage.tsx index f652eba9..14d7f271 100644 --- a/frontend/ui-core/src/annotator/AnnotationPage.tsx +++ b/frontend/ui-core/src/annotator/AnnotationPage.tsx @@ -125,6 +125,7 @@ import { type Polarity, type Suggestion, type SuggestionState, + type Tool, type Viewport, } from "@visionset/annotator"; import { AnnotatorStore as Store } from "@visionset/annotator"; @@ -543,6 +544,22 @@ function JobScreen({ */ const [activeClass, setActiveClass] = useState(null); + /** + * Which of the held class's shapes to draw, when it accepts more than one. + * + * Beside `activeClass` and at the same scope, deliberately: they are one + * decision read two ways, and a preference kept at a scope those query keys + * could move would be lost by a mutation with nothing on screen to say so — + * the `ui-capabilities` rule the drawing class already lives under. + * + * A *preference*, never the answer. `toolFor` resolves it against what the + * class actually accepts and falls back when it cannot be honoured, so this + * being stale is harmless and an active tool the class forbids is + * unrepresentable. `null` means no preference, which is the state a schema of + * one-shape classes never leaves. + */ + const [activeTool, setActiveTool] = useState(null); + /** * The suggest tool's vertex density, held here for `activeClass`'s reason. * @@ -635,9 +652,11 @@ function JobScreen({ counts={progress.data ?? null} clipboard={clipboard} activeClass={activeClass} + activeTool={activeTool} detail={detail} onDetail={setDetail} onActivateClass={activateClass} + onActivateTool={setActiveTool} onNavigate={setChosen} {...(onConfigureInference === undefined ? {} : { onConfigureInference })} {...(onOpenGallery === undefined @@ -714,7 +733,10 @@ interface WorkspaceProps { readonly onDetail: (detail: Detail) => void; /** Also `JobScreen`'s, and for a sharper reason — see the note where it is declared. */ readonly activeClass: string | null; + /** `JobScreen`'s too, and at that scope for the same reason the class is. */ + readonly activeTool: Tool | null; readonly onActivateClass: (labelClass: string | null) => void; + readonly onActivateTool: (tool: Tool | null) => void; readonly onNavigate: (index: number) => void; readonly onOpenGallery?: () => void; /** Where to set up a model connection, if the host has such a screen. */ @@ -761,7 +783,9 @@ function Workspace({ detail, onDetail: setDetail, activeClass, + activeTool, onActivateClass: armClass, + onActivateTool, onNavigate, onOpenGallery, onConfigureInference, @@ -2425,6 +2449,7 @@ function Workspace({ // greyed-out toolbar does not stop a drag from drawing a box. readOnly={readOnly} activeClass={readOnly ? null : activeClass} + activeTool={activeTool} onActivateClass={activateClass} onViewChange={setView} hiddenIds={hiddenIds} @@ -2480,10 +2505,12 @@ function Workspace({ would mean the engine shipping chrome, and putting it outside the stage would mean it was not floating over the picture. - `toolFor` is read here rather than held: the tool is derived from the - active class and never stored (`core/interaction/tool.ts`), and a second - copy on this page would be the pair v1 spent two mechanisms keeping in - step. + `toolFor` is read here rather than held: the tool is *resolved* from + the active class and the preference beside it and never stored + (`core/interaction/tool.ts`), and a second copy of the answer on this + page would be the pair v1 spent two mechanisms keeping in step. What + this page does hold is the preference, which is an input to that + function rather than a second copy of its output. */} {/* A viewer gets the strip, carrying the hand and the shortcut sheet and @@ -2500,8 +2527,10 @@ function Workspace({ readOnly={readOnly} hand={{ active: handTool, onToggle: () => setHandTool((on) => !on) }} schema={store.document.schema} - tool={toolFor(store.document, activeClass)} + tool={toolFor(store.document, activeClass, activeTool)} + activeClass={activeClass} onActivateClass={activateClass} + onActivateTool={onActivateTool} onToggleHelp={() => setHelpOpen((open) => !open)} // Empty, unlike the class field's create row: `+` means "I want a // class", not a particular one, and carrying the previous @@ -2695,6 +2724,8 @@ function Workspace({ onHiddenChange={setHiddenIds} activeClass={activeClass} onActivateClass={activateClass} + activeTool={activeTool} + onActivateTool={onActivateTool} classFilterRef={classFilterRef} // The name comes from whoever asked: the no-match row hands over what // was typed (the WS4 prefill), and the header's `+` hands over "" — diff --git a/frontend/ui-core/src/annotator/AnnotatorPanel.tsx b/frontend/ui-core/src/annotator/AnnotatorPanel.tsx index dd73d104..059068de 100644 --- a/frontend/ui-core/src/annotator/AnnotatorPanel.tsx +++ b/frontend/ui-core/src/annotator/AnnotatorPanel.tsx @@ -85,6 +85,7 @@ import { type AnnotationSchema, type AnnotatorStore, type LabelClass, + type Tool, } from "@visionset/annotator"; import { Check, Eye, EyeOff, Sparkles, Tag, Trash2 } from "lucide-react"; import { useEffect, useRef, useState, type JSX, type RefObject } from "react"; @@ -128,6 +129,9 @@ export interface AnnotatorPanelProps { */ readonly activeClass: string | null; readonly onActivateClass: (labelClass: string) => void; + /** Which shape the armed class draws, and how to change it. See `ClassRegion`. */ + readonly activeTool?: Tool | null; + readonly onActivateTool?: (tool: Tool) => void; /** Focus target for `c`. See `ClassRegion`. */ readonly classFilterRef?: RefObject; /** Open the add-a-class dialog, or absent where there is nowhere to add one. */ @@ -141,6 +145,8 @@ export function AnnotatorPanel({ readOnly = false, activeClass, onActivateClass, + activeTool, + onActivateTool, classFilterRef, onAddClass, }: AnnotatorPanelProps): JSX.Element { @@ -195,7 +201,13 @@ export function AnnotatorPanel({ return (
@@ -214,6 +226,8 @@ export function AnnotatorPanel({ schema={schema} activeClass={activeClass} onActivateClass={onActivateClass} + activeTool={activeTool ?? null} + {...(onActivateTool === undefined ? {} : { onActivateTool })} {...(classFilterRef === undefined ? {} : { filterRef: classFilterRef })} {...(onAddClass === undefined ? {} : { onAddClass })} /> diff --git a/frontend/ui-core/src/annotator/ClassRegion.tsx b/frontend/ui-core/src/annotator/ClassRegion.tsx index d1c28d2d..4c7830e1 100644 --- a/frontend/ui-core/src/annotator/ClassRegion.tsx +++ b/frontend/ui-core/src/annotator/ClassRegion.tsx @@ -40,10 +40,18 @@ * the badge is on the row precisely so the mapping is read rather than memorised. */ -import { hotkeyForClass, type AnnotationSchema, type LabelClass } from "@visionset/annotator"; +import { + drawableGeometries, + hotkeyForClass, + toolForClass, + type AnnotationSchema, + type LabelClass, + type Tool, +} from "@visionset/annotator"; import { Plus } from "lucide-react"; import { useState, type JSX, type RefObject } from "react"; +import { formatGeometries, geometryLabel } from "../data/geometryCategory"; import { classColor } from "../palette"; import { Button } from "../primitives/Button"; import { Input } from "../primitives/Input"; @@ -71,6 +79,19 @@ export interface ClassRegionProps { /** The drawing class, or `null` for select mode — the tool palette's `V`. */ readonly activeClass: string | null; readonly onActivateClass: (labelClass: string) => void; + /** + * Which of the armed class's shapes the next drag produces, and how to change + * it — the same pair `ToolPalette` takes, held once by the page. + * + * Here because arming a class stopped answering it. A class accepting a set has + * no single implied tool, and until this the only place that answer lived was + * the strip at the **far left** of the canvas while the class was chosen on the + * right: one decision split across the width of the picture, in a loop repeated + * hundreds of times a job. Optional, so a host with no tool state renders the + * region as a pure list. + */ + readonly activeTool?: Tool | null; + readonly onActivateTool?: (tool: Tool) => void; /** * Focus target for `c`, held by the page because the keystroke arrives at the * canvas's own keyboard root and not at anything in this tree. @@ -92,6 +113,8 @@ export function ClassRegion({ schema, activeClass, onActivateClass, + activeTool = null, + onActivateTool, filterRef, onAddClass, }: ClassRegionProps): JSX.Element { @@ -211,6 +234,8 @@ export function ClassRegion({ schema={schema} selected={declared.name === activeClass} onSelect={() => onActivateClass(declared.name)} + activeTool={activeTool} + {...(onActivateTool === undefined ? {} : { onActivateTool })} /> )) )} @@ -236,17 +261,48 @@ function ClassRow({ schema, selected, onSelect, + activeTool, + onActivateTool, }: { readonly declared: LabelClass; readonly schema: AnnotationSchema; readonly selected: boolean; readonly onSelect: () => void; + readonly activeTool: Tool | null; + readonly onActivateTool?: (tool: Tool) => void; }): JSX.Element { + /** + * The shapes this row offers as a choice, or nothing. + * + * Three conditions, and each removes a control that would be noise. **Armed**, + * because an unarmed row has no live choice to make and fifty rows of pickers + * would be fifty controls for one decision. **More than one drawable**, because + * a class with a single shape has nothing to choose. **A host that takes the + * answer**, because a picker nothing listens to is worse than none. + * + * `drawableGeometries` rather than `geometries`: a class may accept a tag + * alongside a box, and a tag has no canvas gesture — offering it here would put + * a tool on the strip's vocabulary that the canvas cannot answer. + */ + const drawable = drawableGeometries(declared); + const picker = + selected && drawable.length > 1 && onActivateTool !== undefined + ? drawable.map((tool) => ({ + value: tool, + label: geometryLabel(tool), + // Through `toolFor`'s own resolution rather than a comparison with the + // raw preference: the held tool may be one this class forbids, and the + // lit segment must be the one that would actually be drawn. + active: toolForClass(declared, activeTool) === tool, + onPick: () => onActivateTool(tool), + })) + : undefined; return ( ): void { @@ -147,7 +152,11 @@ export function ReassignMenu({ ) ) : ( - needs a {declared.geometry} + {/* Named in full rather than "does not take a {geometry}": the + question somebody has is what this class *does* take, and a + refusal that only repeats what they already selected answers + nothing. */} + needs {formatGeometries(declared.geometries)} )} diff --git a/frontend/ui-core/src/annotator/ToolPalette.tsx b/frontend/ui-core/src/annotator/ToolPalette.tsx index 3e817cd6..cc32572c 100644 --- a/frontend/ui-core/src/annotator/ToolPalette.tsx +++ b/frontend/ui-core/src/annotator/ToolPalette.tsx @@ -85,12 +85,15 @@ */ import { - drawableGeometry, + drawableGeometries, hotkeyForClass, schemaCanSuggest, type AnnotationSchema, + type GeometryType, type Tool, } from "@visionset/annotator"; + +import { geometryLabel } from "../data/geometryCategory"; import { CircleHelp, Hand, @@ -117,7 +120,7 @@ import { Tooltip, TooltipContent, TooltipTrigger } from "../primitives/Menu"; * that is a property worth more than the twelve lines it costs. * * The record is typed on `string` rather than on the geometry union so an entry - * can name a geometry `drawableGeometry` has never heard of, which is the case it + * can name a geometry `drawableGeometries` never returns, which is the case it * exists for. */ const PENDING_TOOLS: Readonly> = {}; @@ -125,15 +128,16 @@ const PENDING_TOOLS: Readonly> = {}; /** * What each drawing tool is called on the strip. * - * Total over what `drawableGeometry` can answer, so a fourth geometry gaining a - * tool cannot reach the strip unnamed — which is what the ternary this replaced - * would have let it do, silently reading "Polygon". + * Read off the product's one geometry vocabulary rather than kept here. This used + * to be a private map saying `Box` while every other surface printed `bbox`, so + * the same tool had two names depending on which side of the canvas you read it + * from. `geometryLabel` is now the single source; this only capitalises, because + * a control label takes a capital and a word inside a sentence does not. */ -const TOOL_LABELS: Readonly> = { - bbox: "Box", - polygon: "Polygon", - polyline: "Polyline", -}; +function toolLabel(geometry: GeometryType): string { + const word = geometryLabel(geometry); + return word.charAt(0).toUpperCase() + word.slice(1); +} /** A schema's tools, in the order the strip lists them. */ interface ToolChoice { @@ -160,47 +164,99 @@ interface ToolChoice { * A geometry is represented by the **first** class declaring it, in authored * order, which is the same order `classHotkeys` binds the digit row in. Nothing * here dedupes by class: two bbox classes are one bbox tool. + * + * **`activeClass` narrows it.** With a class selected the strip offers only that + * class's own geometries, because those are the only shapes a gesture could + * produce — a bbox button that armed a different class the moment it was pressed + * would answer "what can I draw here?" with something about somewhere else. With + * none selected it is the union, which is what the strip has always shown and is + * still the right answer to "what does this project label?". + * + * A class is only narrowed *to* when it can be drawn: selecting a pure tag class + * leaves the full union rather than emptying the strip, since the tag lives in a + * panel and the strip would otherwise vanish for a reason nothing on it explains. */ -export function toolChoices(schema: AnnotationSchema): readonly ToolChoice[] { +export function toolChoices( + schema: AnnotationSchema, + activeClass: string | null = null, +): readonly ToolChoice[] { + const selected = schema.classes.find((declared) => declared.name === activeClass); + const narrowed = + selected !== undefined && drawableGeometries(selected).length > 0 ? selected : undefined; + const offered = narrowed === undefined ? schema.classes : [narrowed]; + const choices: ToolChoice[] = [ { tool: "select", label: "Select", labelClass: null, hotkey: "V", unavailable: null }, ]; - for (const declared of schema.classes) { - const geometry = drawableGeometry(declared); - if (geometry === null) continue; - if (choices.some((choice) => choice.tool === geometry)) continue; - choices.push({ - tool: geometry, - label: TOOL_LABELS[geometry], - labelClass: declared.name, - hotkey: hotkeyForClass(schema, declared.name) ?? "—", - unavailable: null, - }); + for (const declared of offered) { + for (const geometry of drawableGeometries(declared)) { + if (choices.some((choice) => choice.tool === geometry)) continue; + choices.push({ + tool: geometry, + label: toolLabel(geometry), + labelClass: declared.name, + hotkey: hotkeyForClass(schema, declared.name) ?? "—", + unavailable: null, + }); + } } // After the usable tools, never interleaved: the strip reads top to bottom as // "what you can do", and a disabled control in the middle of that list reads as // a broken one rather than as a coming one. + // + // Read off `schema.classes` rather than `offered`, deliberately: a geometry with + // no tool is a fact about the *project*, and hiding it while a class is selected + // would make the explanation come and go with the selection. for (const declared of schema.classes) { - const pending = PENDING_TOOLS[declared.geometry]; - if (pending === undefined) continue; - if (choices.some((choice) => choice.tool === declared.geometry)) continue; - choices.push({ - tool: declared.geometry, - label: declared.geometry, - // No class to activate, because there is no tool to activate it for. - labelClass: null, - hotkey: "—", - unavailable: pending, - }); + for (const geometry of declared.geometries) { + const pending = PENDING_TOOLS[geometry]; + if (pending === undefined) continue; + if (choices.some((choice) => choice.tool === geometry)) continue; + choices.push({ + tool: geometry, + label: geometry, + // No class to activate, because there is no tool to activate it for. + labelClass: null, + hotkey: "—", + unavailable: pending, + }); + } } return choices; } +/** Whether the held class can produce this shape. `false` when it holds none. */ +function accepts( + schema: AnnotationSchema, + activeClass: string | null, + tool: ToolChoice["tool"], +): boolean { + const declared = schema.classes.find((one) => one.name === activeClass); + return declared !== undefined && drawableGeometries(declared).some((one) => one === tool); +} + export interface ToolPaletteProps { readonly schema: AnnotationSchema; /** What `toolFor` currently answers. Reported, never stored here. */ readonly tool: Tool; + /** + * The class the strip is narrowed to, or `null` for the schema's whole union. + * + * The strip answers *what can I draw here*, and once a class accepts a set of + * geometries the honest answer depends on which class is held. + */ + readonly activeClass: string | null; readonly onActivateClass: (labelClass: string | null) => void; + /** + * Prefer this shape, among the ones the held class accepts. + * + * Separate from `onActivateClass` because pressing a tool now means two + * different things depending on the class: within a class that accepts the + * shape it is only a change of shape, and the class must **not** move — a strip + * that re-armed the geometry's first declaring class would silently retarget + * somebody's labels to a different class than the one they had selected. + */ + readonly onActivateTool: (tool: Tool | null) => void; readonly onToggleHelp: () => void; /** * The suggest tool, or absent where the host cannot serve one. @@ -307,7 +363,9 @@ export interface ToolPaletteProps { export function ToolPalette({ schema, tool, + activeClass, onActivateClass, + onActivateTool, onToggleHelp, onAddClass, history, @@ -334,7 +392,7 @@ export function ToolPalette({ className="absolute left-3 top-3 flex w-12 flex-col items-center gap-1 rounded-xl border border-border bg-muted p-2 shadow-lg" > {!readOnly && - toolChoices(schema).map((choice) => ( + toolChoices(schema, activeClass).map((choice) => ( { if (choice.unavailable !== null) return; - if (tool !== choice.tool) onActivateClass(choice.labelClass); - else if (hand.active) hand.onToggle(); + if (tool === choice.tool) { + if (hand.active) hand.onToggle(); + return; + } + // The shape always. The class only when the one being held cannot + // produce that shape — otherwise this is a change of tool inside + // one class, and moving the class would be the retarget the + // `onActivateTool` docstring warns about. + onActivateTool(choice.tool === "select" ? null : (choice.tool as Tool)); + if (!accepts(schema, activeClass, choice.tool)) { + onActivateClass(choice.labelClass); + } }} > diff --git a/frontend/ui-core/src/annotator/addClass.test.ts b/frontend/ui-core/src/annotator/addClass.test.ts index d9fae7e2..436399d5 100644 --- a/frontend/ui-core/src/annotator/addClass.test.ts +++ b/frontend/ui-core/src/annotator/addClass.test.ts @@ -11,12 +11,12 @@ import { describe, expect, it, vi } from "vitest"; -import { defaultNote, runAddClass } from "./AddClassDialog"; +import { composeVersion, defaultNote, runAddClass } from "./AddClassDialog"; import type { LabelClassBody } from "../screens/queries"; -const SIGN: LabelClassBody = { name: "sign", geometry: "bbox", color: null, attributes: [] }; -const LANE: LabelClassBody = { name: "lane", geometry: "polygon", color: null, attributes: [] }; -const NEW: LabelClassBody = { name: "crossing", geometry: "bbox", color: "#eb5a47", attributes: [] }; +const SIGN: LabelClassBody = { name: "sign", geometries: ["bbox"], color: null, attributes: [] }; +const LANE: LabelClassBody = { name: "lane", geometries: ["polygon"], color: null, attributes: [] }; +const NEW: LabelClassBody = { name: "crossing", geometries: ["bbox"], color: "#eb5a47", attributes: [] }; /** Two recorders writing into one list, so the order is a single assertion. */ function recorders(overrides: Partial Promise>> = {}) { @@ -202,3 +202,41 @@ describe("what the chain is given", () => { * true: a completed batch keeps its version, and somebody publishing from inside * one should be told that before they press. */ +describe("composing the version a sitting publishes", () => { + const WIDENED: LabelClassBody = { + name: "sign", + geometries: ["bbox", "polygon"], + color: null, + attributes: [], + }; + + it("appends a class the active version does not have", () => { + expect(composeVersion([SIGN, LANE], [NEW])).toEqual([SIGN, LANE, NEW]); + }); + + it("replaces a class of the same name rather than adding a second", () => { + // Two classes with one name is what `create_version` refuses outright, so an + // append here would turn the widening flow into a 422 naming nothing the user + // did. + expect(composeVersion([SIGN, LANE], [WIDENED])).toEqual([WIDENED, LANE]); + }); + + it("replaces in place, so the authored class order does not move", () => { + // Not cosmetic: the class list renders in this order and the digit hotkeys + // are positions in it, so appending the widened class would silently + // renumber somebody's keyboard. + expect(composeVersion([SIGN, LANE], [WIDENED]).map((one) => one.name)).toEqual([ + "sign", + "lane", + ]); + }); + + it("matches the name the way the API does, ignoring case", () => { + const shouted: LabelClassBody = { ...WIDENED, name: "SIGN" }; + expect(composeVersion([SIGN, LANE], [shouted])).toEqual([shouted, LANE]); + }); + + it("does both at once, for a sitting that widens one class and adds another", () => { + expect(composeVersion([SIGN, LANE], [WIDENED, NEW])).toEqual([WIDENED, LANE, NEW]); + }); +}); diff --git a/frontend/ui-core/src/annotator/addClassDialog.test.tsx b/frontend/ui-core/src/annotator/addClassDialog.test.tsx index aec32501..16c83dc2 100644 --- a/frontend/ui-core/src/annotator/addClassDialog.test.tsx +++ b/frontend/ui-core/src/annotator/addClassDialog.test.tsx @@ -24,8 +24,8 @@ const ACTIVE = { project_id: "11111111-1111-4111-8111-111111111111", version: 3, classes: [ - { name: "sign", geometry: "bbox", color: null, attributes: [] }, - { name: "lane", geometry: "polygon", color: "#f97316", attributes: [] }, + { name: "sign", geometries: ["bbox"], color: null, attributes: [] }, + { name: "lane", geometries: ["polygon"], color: "#f97316", attributes: [] }, ], description: null, created_at: null, @@ -58,16 +58,59 @@ describe("what the dialog refuses before it asks", () => { expect(screen.getByTestId("add-class-submit")).toHaveProperty("disabled", false); }); - it("will not submit a name the active version already declares, ignoring case", async () => { - // `create_version` refuses a collision case-insensitively, so this mirrors the - // API's rule rather than inventing a second one — and it explains beside the - // field instead of failing after a round trip. - render(mount()); + it("offers to widen a class the active version already declares, ignoring case", async () => { + // A name that exists is not a refusal. `create_version` compares names + // case-insensitively, so "SIGN" lands on "sign" — and what somebody typing it + // wants is almost always to draw that class as a shape it does not have yet. + const onSubmit = vi.fn(); + render(mount({ onSubmit })); await userEvent.type(screen.getByTestId("class-name-new"), "SIGN"); + // The form starts on `bbox`, which "sign" already accepts, so there is + // nothing to add yet and *that* is the refusal — with the remedy named. + expect(screen.getByTestId("add-class-submit")).toHaveProperty("disabled", true); + expect(screen.getByText(/adds nothing to it/)).toBeTruthy(); + + await userEvent.click(screen.getByTestId("class-geometry-new-polygon")); + + const offer = screen.getByTestId("widen-offer"); + expect(offer.textContent).toContain("“sign” already exists"); + expect(offer.textContent).toContain("declares it as box"); + const submit = screen.getByTestId("add-class-submit"); + expect(submit).toHaveProperty("disabled", false); + // The button says what it does, rather than "Add class". + expect(submit.textContent).toContain("Add polygon to “sign”"); + }); + + it("widens the existing class rather than writing a second one", async () => { + const onSubmit = vi.fn(); + render(mount({ onSubmit })); + + await userEvent.type(screen.getByTestId("class-name-new"), "SIGN"); + await userEvent.click(screen.getByTestId("class-geometry-new-polygon")); + await userEvent.click(screen.getByTestId("add-class-submit")); + + // The **existing** class's own name and colour, widened — not the form's. The + // form was opened to make a new class, so publishing its blank colour would + // quietly wipe what "sign" already declared. + expect(onSubmit).toHaveBeenCalledWith( + [{ name: "sign", geometries: ["bbox", "polygon"], color: null, attributes: [] }], + expect.anything(), + ); + }); + + it("still refuses a name typed twice in one sitting, which has nothing to offer", async () => { + // The other collision, and the one that stays a refusal: both entries are + // being written now, so merging them would be guessing which was meant. + render(mount()); + + await userEvent.type(screen.getByTestId("class-name-new"), "crossing"); + await userEvent.click(screen.getByTestId("add-another")); + await userEvent.type(screen.getByTestId("class-name-new"), "CROSSING"); expect(screen.getByTestId("add-class-submit")).toHaveProperty("disabled", true); - expect(screen.getByText(/already declares a class/)).toBeTruthy(); + expect(screen.getByText(/already added a class/)).toBeTruthy(); + expect(screen.queryByTestId("widen-offer")).toBeNull(); }); it("will not submit before the active version has loaded", async () => { @@ -97,7 +140,7 @@ describe("what it submits", () => { await userEvent.click(screen.getByTestId("add-class-submit")); expect(submit).toHaveBeenCalledWith( - [expect.objectContaining({ name: "crossing", geometry: "bbox" })], + [expect.objectContaining({ name: "crossing", geometries: ["bbox"] })], 'Added class "crossing" from the annotation view', ); }); @@ -144,16 +187,15 @@ describe("what it submits", () => { it("groups the geometries under their category, the same as the Schema tab", async () => { render(mount()); - await userEvent.click(screen.getByTestId("class-geometry-new")); - + // No press: the boxes are already on the page. See the Schema tab's twin. const basic = screen.getByTestId("geometry-category-Basic Computer Vision"); const robotics = screen.getByTestId("geometry-category-Robotics and AD"); const membersOf = (label: HTMLElement): string[] => - [...(label.parentElement?.querySelectorAll('[role="option"]') ?? [])].map( + [...(label.parentElement?.querySelectorAll("label") ?? [])].map( (option) => option.textContent ?? "", ); - expect(membersOf(basic)).toEqual(["bbox", "polygon", "classification_tag"]); + expect(membersOf(basic)).toEqual(["box", "polygon", "tag"]); expect(membersOf(robotics)).toEqual(["polyline"]); }); @@ -162,17 +204,35 @@ describe("what it submits", () => { * `toolFor` reads to decide which tool a hotkey arms, so a picker that grouped * its options and stopped writing one would break drawing rather than layout. */ - it("still writes the picked geometry onto the class it will publish", async () => { + it("writes every geometry ticked onto the class it will publish", async () => { const onSubmit = vi.fn(); render(mount({ onSubmit })); await userEvent.type(screen.getByTestId("class-name-new"), "centre-line"); - await userEvent.click(screen.getByTestId("class-geometry-new")); - await userEvent.click(screen.getByRole("option", { name: "polyline" })); + // Tick the second before clearing the first, which is also the only order + // the control allows: a class never passes through accepting nothing. + await userEvent.click(screen.getByTestId("class-geometry-new-polyline")); + await userEvent.click(screen.getByTestId("class-geometry-new-bbox")); + await userEvent.click(screen.getByTestId("add-class-submit")); + + expect(onSubmit).toHaveBeenCalledWith( + [expect.objectContaining({ name: "centre-line", geometries: ["polyline"] })], + expect.anything(), + ); + }); + + it("publishes a class accepting two shapes, which is what a set is for", async () => { + const onSubmit = vi.fn(); + render(mount({ onSubmit })); + + // A name the active version does not hold: "sign" would land on the widening + // path below, which is a different test. + await userEvent.type(screen.getByTestId("class-name-new"), "kerb"); + await userEvent.click(screen.getByTestId("class-geometry-new-polygon")); await userEvent.click(screen.getByTestId("add-class-submit")); expect(onSubmit).toHaveBeenCalledWith( - [expect.objectContaining({ name: "centre-line", geometry: "polyline" })], + [expect.objectContaining({ name: "kerb", geometries: ["bbox", "polygon"] })], expect.anything(), ); }); diff --git a/frontend/ui-core/src/annotator/addClassProvenance.test.tsx b/frontend/ui-core/src/annotator/addClassProvenance.test.tsx index 46432863..a2bdcb40 100644 --- a/frontend/ui-core/src/annotator/addClassProvenance.test.tsx +++ b/frontend/ui-core/src/annotator/addClassProvenance.test.tsx @@ -37,7 +37,7 @@ const ASSET = "44444444-4444-4444-8444-444444444444"; const SCHEMA = { project_id: PROJECT, version: 1, - classes: [{ name: "sign", geometry: "bbox", color: null, attributes: [] }], + classes: [{ name: "sign", geometries: ["bbox"], color: null, attributes: [] }], description: null, created_at: null, provenance: "curated", diff --git a/frontend/ui-core/src/annotator/canvasLabel.test.tsx b/frontend/ui-core/src/annotator/canvasLabel.test.tsx index 2c78617f..db4c2e49 100644 --- a/frontend/ui-core/src/annotator/canvasLabel.test.tsx +++ b/frontend/ui-core/src/annotator/canvasLabel.test.tsx @@ -31,8 +31,8 @@ const SCHEMA = { project_id: "11111111-1111-4111-8111-111111111111", version: 1, classes: [ - { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, - { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }, + { name: "pedestrian", geometries: ["bbox"], color: null, attributes: [] }, ], }; diff --git a/frontend/ui-core/src/annotator/canvasReassign.test.tsx b/frontend/ui-core/src/annotator/canvasReassign.test.tsx index 9eab027f..ec65d235 100644 --- a/frontend/ui-core/src/annotator/canvasReassign.test.tsx +++ b/frontend/ui-core/src/annotator/canvasReassign.test.tsx @@ -28,10 +28,10 @@ const SCHEMA = { project_id: "11111111-1111-4111-8111-111111111111", version: 1, classes: [ - { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, - { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, - { name: "lane", geometry: "polygon", color: "#f97316", attributes: [] }, - { name: "daytime", geometry: "classification_tag", color: "#a3e635", attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }, + { name: "pedestrian", geometries: ["bbox"], color: null, attributes: [] }, + { name: "lane", geometries: ["polygon"], color: "#f97316", attributes: [] }, + { name: "daytime", geometries: ["classification_tag"], color: "#a3e635", attributes: [] }, ], }; @@ -168,7 +168,7 @@ describe("what the picker does", () => { "true", ); } - expect(screen.getByTestId("canvas-reclass-lane").textContent).toContain("needs a polygon"); + expect(screen.getByTestId("canvas-reclass-lane").textContent).toContain("needs polygon"); }); it("checks the class the shape already carries", () => { diff --git a/frontend/ui-core/src/annotator/classRegion.test.tsx b/frontend/ui-core/src/annotator/classRegion.test.tsx index 91b7094a..8c188b6c 100644 --- a/frontend/ui-core/src/annotator/classRegion.test.tsx +++ b/frontend/ui-core/src/annotator/classRegion.test.tsx @@ -33,7 +33,7 @@ function schemaOf(n: number): AnnotationSchema { provenance: "curated", classes: Array.from({ length: n }, (_unused, index) => ({ name: `class-${index + 1}`, - geometry: index % 2 === 0 ? "bbox" : "polygon", + geometries: index % 2 === 0 ? ["bbox"] : ["polygon"], color: null, attributes: [], })), @@ -136,3 +136,107 @@ describe("the empty schema", () => { expect(screen.queryByTestId("class-list")).toBeNull(); }); }); + +describe("the shape picker on the armed row (#584)", () => { + /** One class taking two shapes, one taking a third, one taking only a tag. */ + const MIXED = { + project_id: "p", + version: 1, + description: null, + created_at: null, + provenance: "curated", + classes: [ + { name: "car", geometries: ["bbox", "polygon"], color: null, attributes: [] }, + { name: "lane", geometries: ["polyline"], color: null, attributes: [] }, + { name: "weather", geometries: ["classification_tag"], color: null, attributes: [] }, + ], + } as unknown as AnnotationSchema; + + function mountMixed(overrides: Partial[0]> = {}): JSX.Element { + return ( + + ); + } + + it("offers one control per drawable shape, with the active one pressed", () => { + render(mountMixed()); + + const box = screen.getByTestId("class-row-car-shape-bbox"); + const polygon = screen.getByTestId("class-row-car-shape-polygon"); + expect(box.getAttribute("aria-pressed")).toBe("true"); + expect(polygon.getAttribute("aria-pressed")).toBe("false"); + // The word, not the wire value — the row is where the vocabulary is read. + expect(box.textContent).toBe("box"); + }); + + it("changes the tool and not the class, which is the whole point of it being here", () => { + // The retarget guard, from the panel's side. `ToolPalette` already holds this + // rule and is tested in both directions; a second caller getting it wrong + // would silently move somebody's labels to a different class than the one + // they had armed — and with a two-shape class there is no visible tell. + const onActivateClass = vi.fn(); + const onActivateTool = vi.fn(); + render(mountMixed({ onActivateClass, onActivateTool })); + + screen.getByTestId("class-row-car-shape-polygon").click(); + + expect(onActivateTool).toHaveBeenCalledWith("polygon"); + expect(onActivateClass).not.toHaveBeenCalled(); + }); + + it("shows no picker on a row that is not armed", () => { + // An unarmed row has no live choice. Fifty classes would otherwise carry + // fifty controls for one decision. + render(mountMixed({ activeClass: "lane" })); + + expect(screen.queryByTestId("class-row-car-shape-bbox")).toBeNull(); + }); + + it("shows no picker on an armed class that accepts only one shape", () => { + render(mountMixed({ activeClass: "lane" })); + + expect(screen.queryByTestId("class-row-lane-shape-polyline")).toBeNull(); + }); + + it("counts drawable shapes, so a tag beside a box is not a second tool", () => { + // A class may accept a tag *and* a box. The tag has no canvas gesture, so it + // is not a choice the canvas could answer — one drawable shape means no + // picker, exactly as if the tag were not declared. + const tagAndBox = { + ...MIXED, + classes: [ + { name: "car", geometries: ["bbox", "classification_tag"], color: null, attributes: [] }, + ], + } as unknown as AnnotationSchema; + render(mountMixed({ schema: tagAndBox })); + + expect(screen.queryByTestId("class-row-car-shape-bbox")).toBeNull(); + expect(screen.queryByTestId("class-row-car-shape-classification_tag")).toBeNull(); + }); + + it("lights the shape that would actually be drawn, not the raw preference", () => { + // The held tool may be one this class forbids — arriving from a class that + // allowed it. `toolForClass` resolves that to the class's first drawable + // shape, and the lit segment has to be that, or the panel and the canvas + // disagree about what the next drag produces. + render(mountMixed({ activeTool: "polyline" })); + + expect(screen.getByTestId("class-row-car-shape-bbox").getAttribute("aria-pressed")).toBe( + "true", + ); + }); + + it("renders a plain row, one tab stop, when no host takes the answer", () => { + // A picker nothing listens to is worse than none. + render(mountMixed({ onActivateTool: undefined })); + + expect(screen.queryByTestId("class-row-car-shape-polygon")).toBeNull(); + }); +}); diff --git a/frontend/ui-core/src/annotator/drawingClass.test.tsx b/frontend/ui-core/src/annotator/drawingClass.test.tsx index f08e9264..68cc5ea5 100644 --- a/frontend/ui-core/src/annotator/drawingClass.test.tsx +++ b/frontend/ui-core/src/annotator/drawingClass.test.tsx @@ -39,8 +39,8 @@ const SCHEMA = { project_id: PROJECT, version: 1, classes: [ - { name: "sign", geometry: "bbox", color: null, attributes: [] }, - { name: "vehicle", geometry: "bbox", color: null, attributes: [] }, + { name: "sign", geometries: ["bbox"], color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: null, attributes: [] }, ], description: null, created_at: null, diff --git a/frontend/ui-core/src/annotator/editorNotice.test.tsx b/frontend/ui-core/src/annotator/editorNotice.test.tsx index ab4e5a74..713c6e6e 100644 --- a/frontend/ui-core/src/annotator/editorNotice.test.tsx +++ b/frontend/ui-core/src/annotator/editorNotice.test.tsx @@ -40,7 +40,7 @@ const SCHEMA = { description: null, created_at: null, provenance: "curated", - classes: [{ name: "vehicle", geometry: "bbox", color: "#3355ff", attributes: [] }], + classes: [{ name: "vehicle", geometries: ["bbox"], color: "#3355ff", attributes: [] }], }; /** The batch's state, which decides whether the page tries to open it. */ diff --git a/frontend/ui-core/src/annotator/frameGallery.test.tsx b/frontend/ui-core/src/annotator/frameGallery.test.tsx index 502aeb4a..302ee31a 100644 --- a/frontend/ui-core/src/annotator/frameGallery.test.tsx +++ b/frontend/ui-core/src/annotator/frameGallery.test.tsx @@ -43,7 +43,7 @@ const SCHEMA = { description: null, created_at: null, provenance: "curated", - classes: [{ name: "vehicle", geometry: "bbox", color: "#3355ff", attributes: [] }], + classes: [{ name: "vehicle", geometries: ["bbox"], color: "#3355ff", attributes: [] }], }; type Progress = "unannotated" | "annotated" | "skipped" | "review_pending" | "accepted"; diff --git a/frontend/ui-core/src/annotator/jobQueries.test.ts b/frontend/ui-core/src/annotator/jobQueries.test.ts index 76108902..5483fbd7 100644 --- a/frontend/ui-core/src/annotator/jobQueries.test.ts +++ b/frontend/ui-core/src/annotator/jobQueries.test.ts @@ -23,8 +23,8 @@ const SCHEMA = { project_id: "11111111-1111-4111-8111-111111111111", version: 2, classes: [ - { name: "vehicle", geometry: "bbox", color: null, attributes: [] }, - { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: null, attributes: [] }, + { name: "pedestrian", geometries: ["bbox"], color: null, attributes: [] }, ], }; diff --git a/frontend/ui-core/src/annotator/panel.test.tsx b/frontend/ui-core/src/annotator/panel.test.tsx index 3e9bd24c..a4dc5218 100644 --- a/frontend/ui-core/src/annotator/panel.test.tsx +++ b/frontend/ui-core/src/annotator/panel.test.tsx @@ -24,18 +24,20 @@ const SCHEMA = { project_id: "11111111-1111-4111-8111-111111111111", version: 1, classes: [ - { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, - { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, - { name: "lane", geometry: "polygon", color: "#f97316", attributes: [] }, - { name: "daytime", geometry: "classification_tag", color: "#a3e635", attributes: [] }, - { name: "centerline", geometry: "polyline", color: "#eb5a47", attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }, + { name: "pedestrian", geometries: ["bbox"], color: null, attributes: [] }, + { name: "lane", geometries: ["polygon"], color: "#f97316", attributes: [] }, + { name: "daytime", geometries: ["classification_tag"], color: "#a3e635", attributes: [] }, + { name: "centerline", geometries: ["polyline"], color: "#eb5a47", attributes: [] }, ], }; /** The same schema with its one tag class removed — the strip's absent case. */ const UNTAGGABLE_SCHEMA = { ...SCHEMA, - classes: SCHEMA.classes.filter((declared) => declared.geometry !== "classification_tag"), + classes: SCHEMA.classes.filter( + (declared) => !declared.geometries.includes("classification_tag"), + ), }; function annotation( @@ -313,8 +315,8 @@ describe("reassigning a class from a row", () => { for (const name of ["lane", "centerline", "daytime"]) { expect(screen.getByTestId(`reclass-0-${name}`).getAttribute("aria-disabled")).toBe("true"); } - expect(screen.getByTestId("reclass-0-lane").textContent).toContain("needs a polygon"); - expect(screen.getByTestId("reclass-0-centerline").textContent).toContain("needs a polyline"); + expect(screen.getByTestId("reclass-0-lane").textContent).toContain("needs polygon"); + expect(screen.getByTestId("reclass-0-centerline").textContent).toContain("needs polyline"); }); it("will not reassign to a class the kernel would refuse", async () => { diff --git a/frontend/ui-core/src/annotator/pinBadge.test.tsx b/frontend/ui-core/src/annotator/pinBadge.test.tsx index 08520de5..f4b987a8 100644 --- a/frontend/ui-core/src/annotator/pinBadge.test.tsx +++ b/frontend/ui-core/src/annotator/pinBadge.test.tsx @@ -35,7 +35,7 @@ const ASSET = "44444444-4444-4444-8444-444444444444"; const PINNED = { project_id: PROJECT, version: 1, - classes: [{ name: "sign", geometry: "bbox", color: null, attributes: [] }], + classes: [{ name: "sign", geometries: ["bbox"], color: null, attributes: [] }], description: null, created_at: null, provenance: "curated", diff --git a/frontend/ui-core/src/annotator/suggestFlow.test.tsx b/frontend/ui-core/src/annotator/suggestFlow.test.tsx index 28fb8e4d..aee0faf4 100644 --- a/frontend/ui-core/src/annotator/suggestFlow.test.tsx +++ b/frontend/ui-core/src/annotator/suggestFlow.test.tsx @@ -49,13 +49,13 @@ const SCHEMA = { created_at: null, provenance: "curated", classes: [ - { name: "vehicle", geometry: "bbox", color: "#3355ff", attributes: [] }, - { name: "lane-area", geometry: "polygon", color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#3355ff", attributes: [] }, + { name: "lane-area", geometries: ["polygon"], color: null, attributes: [] }, // Drawable, and not suggestible: a mask narrows to a region and a lane is an // open path. It is what parks the tool, and it is a `polyline` rather // than a tag on purpose — a class that can still be drawn on is the case where // a parked tool swallowing presses would be a bug rather than a nuisance. - { name: "lane", geometry: "polyline", color: null, attributes: [] }, + { name: "lane", geometries: ["polyline"], color: null, attributes: [] }, ], }; diff --git a/frontend/ui-core/src/annotator/toolPalette.test.tsx b/frontend/ui-core/src/annotator/toolPalette.test.tsx index 1e4d4d0f..0cc74968 100644 --- a/frontend/ui-core/src/annotator/toolPalette.test.tsx +++ b/frontend/ui-core/src/annotator/toolPalette.test.tsx @@ -24,11 +24,11 @@ const SCHEMA = { project_id: "11111111-1111-4111-8111-111111111111", version: 1, classes: [ - { name: "vehicle", geometry: "bbox", color: "#38bdf8", attributes: [] }, - { name: "pedestrian", geometry: "bbox", color: null, attributes: [] }, - { name: "lane", geometry: "polygon", color: "#f97316", attributes: [] }, - { name: "daytime", geometry: "classification_tag", color: "#a3e635", attributes: [] }, - { name: "kerb", geometry: "polyline", color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#38bdf8", attributes: [] }, + { name: "pedestrian", geometries: ["bbox"], color: null, attributes: [] }, + { name: "lane", geometries: ["polygon"], color: "#f97316", attributes: [] }, + { name: "daytime", geometries: ["classification_tag"], color: "#a3e635", attributes: [] }, + { name: "kerb", geometries: ["polyline"], color: null, attributes: [] }, ], } as unknown as Parameters[0]; @@ -40,7 +40,9 @@ function mount( { ]); }); + describe("a class that accepts more than one shape (#584)", () => { + /** One class, two shapes — the whole point of a geometry set. */ + const BOTH = { + ...SCHEMA, + classes: [ + { name: "sign", geometries: ["bbox", "polygon"], color: "#38bdf8", attributes: [] }, + { name: "kerb", geometries: ["polyline"], color: null, attributes: [] }, + ], + } as typeof SCHEMA; + + it("offers both of the held class's shapes and nothing else", () => { + render(mount({ schema: BOTH, activeClass: "sign", tool: "bbox" })); + + expect(screen.getByTestId("tool-bbox")).toBeTruthy(); + expect(screen.getByTestId("tool-polygon")).toBeTruthy(); + // `kerb`'s shape is not something this class could draw. + expect(screen.queryByTestId("tool-polyline")).toBeNull(); + }); + + it("changes only the tool when the held class already accepts the shape", () => { + // **The retarget guard.** Pressing polygon here means "draw this class as a + // polygon", not "switch to whatever class declares polygon first". A strip + // that re-armed the geometry's first declaring class would silently move + // somebody's labels to a different class than the one they had selected — + // and with a two-shape class there is no visible tell that it happened. + const onActivateClass = vi.fn(); + const onActivateTool = vi.fn(); + render( + mount({ schema: BOTH, activeClass: "sign", tool: "bbox", onActivateClass, onActivateTool }), + ); + + fireEvent.click(screen.getByTestId("tool-polygon")); + + expect(onActivateTool).toHaveBeenCalledWith("polygon"); + expect(onActivateClass).not.toHaveBeenCalled(); + }); + + it("moves the class when the held one cannot draw the shape pressed", () => { + // The other direction of the same site, which a single-direction mutation + // leaves green: with no class held, nothing accepts the tool, so the press + // has to arm the class that declares it. + const onActivateClass = vi.fn(); + const onActivateTool = vi.fn(); + render( + mount({ schema: BOTH, activeClass: null, tool: "select", onActivateClass, onActivateTool }), + ); + + fireEvent.click(screen.getByTestId("tool-polyline")); + + expect(onActivateTool).toHaveBeenCalledWith("polyline"); + expect(onActivateClass).toHaveBeenCalledWith("kerb"); + }); + }); + it("offers no polyline button at all when the schema declares no lane class", () => { // The affordance is about *this* schema. A strip advertising a geometry // nobody declared would be a roadmap, not a tool strip. const noLanes = { ...SCHEMA, - classes: SCHEMA.classes.filter((declared) => declared.geometry !== "polyline"), + classes: SCHEMA.classes.filter((declared) => !declared.geometries.includes("polyline")), } as typeof SCHEMA; render(mount({ schema: noLanes })); @@ -145,7 +201,7 @@ describe("the tools a schema can reach", () => { it("shows only select when no class draws anything", () => { const tagsOnly = { ...SCHEMA, - classes: [{ name: "daytime", geometry: "classification_tag", color: null, attributes: [] }], + classes: [{ name: "daytime", geometries: ["classification_tag"], color: null, attributes: [] }], } as unknown as typeof SCHEMA; render(mount({ schema: tagsOnly })); @@ -277,8 +333,8 @@ describe("the suggest tool (#424)", () => { const tagsOnly = { ...(SCHEMA as unknown as { classes: unknown[] }), classes: [ - { name: "daytime", geometry: "classification_tag", color: null, attributes: [] }, - { name: "kerb", geometry: "polyline", color: null, attributes: [] }, + { name: "daytime", geometries: ["classification_tag"], color: null, attributes: [] }, + { name: "kerb", geometries: ["polyline"], color: null, attributes: [] }, ], } as unknown as Parameters[0]; render(mount({ schema: tagsOnly, suggest: { active: false, onToggle: vi.fn() } })); diff --git a/frontend/ui-core/src/annotator/topBar.test.tsx b/frontend/ui-core/src/annotator/topBar.test.tsx index a1510272..03c5efb3 100644 --- a/frontend/ui-core/src/annotator/topBar.test.tsx +++ b/frontend/ui-core/src/annotator/topBar.test.tsx @@ -39,8 +39,8 @@ const SCHEMA = { created_at: null, provenance: "curated", classes: [ - { name: "vehicle", geometry: "bbox", color: "#3355ff", attributes: [] }, - { name: "lane-area", geometry: "polygon", color: null, attributes: [] }, + { name: "vehicle", geometries: ["bbox"], color: "#3355ff", attributes: [] }, + { name: "lane-area", geometries: ["polygon"], color: null, attributes: [] }, ], }; @@ -273,18 +273,25 @@ describe("the class list, now in the panel (#420)", () => { expect(screen.getByTestId("class-row-lane-area").textContent).toContain("2"); }); - it("changes the derived tool when the class picked declares another geometry", async () => { - // The tool is *derived* from the active class and never stored - // (`core/interaction/tool.ts`), so this asserts the derivation still runs - // through the panel — it does not re-derive anything itself. + it("changes the tool, and the strip, when the class picked accepts another geometry", async () => { + // The tool is *resolved* from the active class and the held preference and + // never stored (`core/interaction/tool.ts`), so this asserts the resolution + // still runs through the panel — it does not re-derive anything itself. + // + // The strip narrows with it, which is the visible half of #584: with a + // polygon-only class held, a box is not something that could be drawn here, + // and offering the button would answer "what can I draw?" with a lie. Both + // fixture classes accept exactly one shape, so each selection leaves exactly + // one drawing tool. await open(); await userEvent.click(screen.getByTestId("class-row-vehicle")); expect(screen.getByTestId("tool-bbox").getAttribute("data-active")).toBe("true"); + expect(screen.queryByTestId("tool-polygon")).toBeNull(); await userEvent.click(screen.getByTestId("class-row-lane-area")); expect(screen.getByTestId("tool-polygon").getAttribute("data-active")).toBe("true"); - expect(screen.getByTestId("tool-bbox").getAttribute("data-active")).toBe("false"); + expect(screen.queryByTestId("tool-bbox")).toBeNull(); }); it("focuses the panel's filter on `c`, which is the whole point of the host action", async () => { diff --git a/frontend/ui-core/src/data/geometryCategory.test.ts b/frontend/ui-core/src/data/geometryCategory.test.ts index 196d7938..a1f57fad 100644 --- a/frontend/ui-core/src/data/geometryCategory.test.ts +++ b/frontend/ui-core/src/data/geometryCategory.test.ts @@ -25,6 +25,9 @@ import { firstMismatch } from "./check"; import { GEOMETRY_CATEGORIES, GEOMETRY_CATEGORY, + GEOMETRY_LABELS, + formatGeometries, + geometryLabel, groupGeometries, type GeometryCategory, } from "./geometryCategory"; @@ -99,3 +102,61 @@ describe("grouping what a surface offers", () => { expect(groupGeometries([])).toEqual([]); }); }); + + +describe("what a geometry is called on screen", () => { + it("is total over the wire's geometry union", () => { + // Same shape as the category map's own claim above, and for the same reason: + // the `satisfies` is the proof, this is the copy of it that does not move + // when somebody edits the declaration. + const total: Record = GEOMETRY_LABELS; + expect(Object.keys(total).length).toBeGreaterThan(0); + }); + + it("names nothing the wire does not call a geometry", () => { + for (const geometry of Object.keys(GEOMETRY_LABELS)) { + expect(firstMismatch(checkGeometryType, geometry)).toBeNull(); + } + }); + + it("does not print the wire value where the two differ", () => { + // **The assertion that matters.** A map whose every entry equalled its key + // would type-check, satisfy totality, and be exactly the defect this exists + // to remove — the interface showing users identifiers. These are the two the + // kernel spells for itself rather than for a person, so they are the two that + // prove the map is doing work. + expect(geometryLabel("bbox")).toBe("box"); + expect(geometryLabel("classification_tag")).toBe("tag"); + }); + + it("never starts with a capital, because the same word goes in a sentence", () => { + // A capital reads fine as a chip and wrong mid-sentence ("Publishing adds + // Polygon to it"). The tool strip capitalises at its own control instead. + // + // **Starts** lowercase rather than *is* lowercase, and the difference is a + // real one this caught: `3D box` is an acronym, and a rule demanding the + // whole string be lowercase would have forced `3d box`, which is wrong in + // every position. Only the first letter is a sentence-position question. + for (const label of Object.values(GEOMETRY_LABELS)) { + expect(label).toBe(label.charAt(0).toLowerCase() + label.slice(1)); + } + }); +}); + +describe("a set of geometries, as one phrase", () => { + it("joins with a middot, in the order it was given", () => { + expect(formatGeometries(["bbox", "polygon"])).toBe("box · polygon"); + }); + + it("uses the display words, so a tag class does not print its enum member", () => { + // ~110px of a 248px row, before this. The single largest width saving + // available in the class list, larger than widening the panel. + expect(formatGeometries(["classification_tag"])).toBe("tag"); + }); + + it("says nothing for an empty set, rather than a stray separator", () => { + // The kernel cannot produce one, but a refusal renders `?? []` while a class + // is being typed, and " · " alone would read as damage. + expect(formatGeometries([])).toBe(""); + }); +}); diff --git a/frontend/ui-core/src/data/geometryCategory.ts b/frontend/ui-core/src/data/geometryCategory.ts index 05cfb532..04f7eb14 100644 --- a/frontend/ui-core/src/data/geometryCategory.ts +++ b/frontend/ui-core/src/data/geometryCategory.ts @@ -127,3 +127,66 @@ export function groupGeometries( geometries: offered.filter((geometry) => GEOMETRY_CATEGORY[geometry] === category), })).filter((group) => group.geometries.length > 0); } + +/** + * What each geometry is **called on screen**, which is not what it is called on + * the wire. + * + * `GeometryType`'s members are the kernel's identifiers — `bbox` because that is + * the discriminator every payload carries, `classification_tag` because that is + * what the variant is. Neither is a word to show somebody. Until this map existed + * the product had **two vocabularies**: the tool strip's private `TOOL_LABELS` + * said `Box`, and every other surface — class rows, the reassignment menu, the + * add-a-class dialog's checkboxes and prose, the schema editor's badges, the + * project summary — printed the enum. So one thing was `Box` on the left of the + * canvas and `bbox` on the right, and a tag class's row spent about 110px of a + * 248px row saying `classification_tag`. + * + * **Lowercase**, because the same word is used two ways and only lowercase reads + * correctly in both: as a chip in a dense row (`box · polygon`) and inside a + * sentence (*"Publishing adds polygon to it"*). A control that wants a capital + * — the tool strip's `Box (1)` — capitalises at the point of use, so there is one + * source and one transform rather than two lists free to drift apart again. + * + * Total over the union by `satisfies`, exactly as `GEOMETRY_CATEGORY` above, so a + * ninth member fails the build until somebody names it. The four with no + * implementation are named too: a schema may legally declare `mask`, and the + * surface that has to refuse it should refuse it in words. + */ +export const GEOMETRY_LABELS = { + bbox: "box", + polygon: "polygon", + polyline: "polyline", + classification_tag: "tag", + mask: "mask", + keypoints: "keypoints", + cuboid_3d: "3D box", + polyline_3d: "3D polyline", +} as const satisfies Record; + +/** What to call this geometry on screen. Never the wire value. */ +export function geometryLabel(geometry: GeometryType): string { + return GEOMETRY_LABELS[geometry]; +} + +/** + * A class's geometry set, as one phrase for a row, a badge or a refusal. + * + * One spelling, product-wide, for the reason `classColor` is one: a class list, a + * reassignment menu and a schema row all name the same set, and three joins would + * be three chances to render `box,polygon` beside `box, polygon` beside + * `box or polygon`. + * + * **A middot, not "or".** The set is a choice — an annotation carries one of them, + * never several — and a comma list would read as things a class has all of. "or" + * says that correctly and costs four characters in a row where the class *name* + * is what those characters come out of. `·` is what a set reads as at this + * density, and the row has no room to be polite. + * + * The order is the caller's, which for anything off the wire is the kernel's own + * sorted order. Nothing re-sorts here: a surface that grouped by category would + * hand them over grouped, and this would silently undo it. + */ +export function formatGeometries(geometries: readonly GeometryType[]): string { + return geometries.map(geometryLabel).join(" · "); +} diff --git a/frontend/ui-core/src/generated/api.ts b/frontend/ui-core/src/generated/api.ts index dd134b16..5355bdbf 100644 --- a/frontend/ui-core/src/generated/api.ts +++ b/frontend/ui-core/src/generated/api.ts @@ -3381,7 +3381,7 @@ export interface components { JsonValue: unknown; /** * LabelClassBody - * @description One labelable class, bound to a geometry. + * @description One labelable class, and the geometries an annotation of it may carry. */ LabelClassBody: { /** @@ -3391,7 +3391,8 @@ export interface components { attributes: components["schemas"]["AttributeBody"][]; /** Color */ color?: string | null; - geometry: components["schemas"]["GeometryType"]; + /** Geometries */ + geometries: components["schemas"]["GeometryType"][]; /** Name */ name: string; }; diff --git a/frontend/ui-core/src/generated/checks.ts b/frontend/ui-core/src/generated/checks.ts index f2d93a66..aae76068 100644 --- a/frontend/ui-core/src/generated/checks.ts +++ b/frontend/ui-core/src/generated/checks.ts @@ -264,7 +264,7 @@ export const checkAttributeBody: Check = /*#__PURE__*/ object({ "default": [false, either([isBoolean, isNumber, isString, isNull] as const)], "kind": [true, oneOf(["string", "number", "boolean", "select"] as const)], "name": [true, isString], "options": [false, either([arrayOf(isString), isNull] as const)], "required": [true, isBoolean] } as const); export const checkLabelClassBody: Check = - /*#__PURE__*/ object({ "attributes": [true, arrayOf(checkAttributeBody)], "color": [false, either([isString, isNull] as const)], "geometry": [true, checkGeometryType], "name": [true, isString] } as const); + /*#__PURE__*/ object({ "attributes": [true, arrayOf(checkAttributeBody)], "color": [false, either([isString, isNull] as const)], "geometries": [true, arrayOf(checkGeometryType)], "name": [true, isString] } as const); export const checkSchemaProvenance: Check = /*#__PURE__*/ oneOf(["curated", "annotation"] as const); diff --git a/frontend/ui-core/src/index.ts b/frontend/ui-core/src/index.ts index 20a6eba8..b10d45be 100644 --- a/frontend/ui-core/src/index.ts +++ b/frontend/ui-core/src/index.ts @@ -233,6 +233,9 @@ export { export { GEOMETRY_CATEGORIES, GEOMETRY_CATEGORY, + GEOMETRY_LABELS, + formatGeometries, + geometryLabel, groupGeometries, type GeometryCategory, type GeometryGroup, diff --git a/frontend/ui-core/src/palette.test.ts b/frontend/ui-core/src/palette.test.ts index 62b887b7..9e3d646f 100644 --- a/frontend/ui-core/src/palette.test.ts +++ b/frontend/ui-core/src/palette.test.ts @@ -19,14 +19,14 @@ import { CLASS_FILL_OPACITY, classColor, hexColor, type LabelClass } from "./pal const withColour: LabelClass = { name: "vehicle", - geometry: "bbox", + geometries: ["bbox"], color: "#38bdf8", attributes: [], }; const without: LabelClass = { name: "pedestrian", - geometry: "bbox", + geometries: ["bbox"], color: null, attributes: [], }; @@ -128,7 +128,7 @@ describe("hexColor", () => { // declared colour is convertible. The moment that stops holding, the editor // goes grey again. for (const name of ["lane", "vehicle", "pedestrian", "weather", "", "ünïcodé", "a".repeat(64)]) { - const derived = classColor({ name, geometry: "bbox", color: null, attributes: [] }, name); + const derived = classColor({ name, geometries: ["bbox"], color: null, attributes: [] }, name); expect(derived).toMatch(/^hsl\(/); expect(hexColor(derived)).toMatch(/^#[0-9a-f]{6}$/); } diff --git a/frontend/ui-core/src/patterns/ClassFields.tsx b/frontend/ui-core/src/patterns/ClassFields.tsx index 32505b0f..793bcdfd 100644 --- a/frontend/ui-core/src/patterns/ClassFields.tsx +++ b/frontend/ui-core/src/patterns/ClassFields.tsx @@ -22,16 +22,14 @@ import { Plus, Trash2 } from "lucide-react"; import type { JSX } from "react"; -import { groupGeometries } from "../data/geometryCategory"; +import { geometryLabel, groupGeometries } from "../data/geometryCategory"; import { classColor, hexColor } from "../palette"; import { Button } from "../primitives/Button"; import { FieldHint, Input, Label } from "../primitives/Input"; import { Select, SelectContent, - SelectGroup, SelectItem, - SelectLabel, SelectTrigger, SelectValue, } from "../primitives/Select"; @@ -58,6 +56,22 @@ const GEOMETRIES = [ const KINDS = ["string", "number", "boolean", "select"] as const; type Kind = (typeof KINDS)[number]; +/** + * What the chosen set means, said once under the group. + * + * The hint used to read *"Singular — picking a class picks a tool"*, which is no + * longer true: a class accepts a set, and picking one narrows the tool strip + * rather than deciding it. Naming the count rather than restating the rule keeps + * the sentence useful in the case somebody is most likely to have got wrong — + * having ticked one box and not realised a second was allowed. + */ +export function describeGeometries(geometries: readonly GeometryType[]): string { + if (geometries.length <= 1) { + return "One shape for now. Tick another and this class accepts both."; + } + return `An annotation of this class may be any of the ${geometries.length}.`; +} + export interface ClassFieldsProps { readonly declared: LabelClassBody; /** What this instance's `data-testid`s are built from. See the module docstring. */ @@ -88,39 +102,87 @@ export function ClassFields({ onChange={(event) => onChange({ ...declared, name: event.target.value })} /> -
- - - Singular — picking a class picks a tool. -
+ {/* Grouped, not flat, for the reason the dropdown was: a flat list of + every name the product can address says nothing about which ones + belong to the work somebody is actually doing, and the list only + grows. Native checkboxes rather than a new primitive — the + attribute `required` flag below is the same answer to the same + question, and a multi-select dropdown would hide the answer behind + a click on a control whose whole job is to show it. */} + {groupGeometries(GEOMETRIES).map((group) => ( +
+ + {group.category} + +
+ {group.geometries.map((geometry) => { + const checked = declared.geometries.includes(geometry); + // The last one standing does not come off. A class accepting + // nothing is refused by the kernel and by the wire, so the + // honest control is one that says why rather than one that + // lets you build a version the API will reject — and a bare + // disabled box would be principle 9's forbidden shape. + const last = checked && declared.geometries.length === 1; + return ( + + ); + })} +
+
+ ))} + + + {describeGeometries(declared.geometries)} + +
@@ -185,7 +247,7 @@ export function swatchOf(declared: LabelClassBody, index: number): string { return classColor( { name: declared.name, - geometry: declared.geometry, + geometries: declared.geometries, color: declared.color ?? null, attributes: [], }, diff --git a/frontend/ui-core/src/patterns/DataDisplay.tsx b/frontend/ui-core/src/patterns/DataDisplay.tsx index 66938d3f..324f049f 100644 --- a/frontend/ui-core/src/patterns/DataDisplay.tsx +++ b/frontend/ui-core/src/patterns/DataDisplay.tsx @@ -214,6 +214,31 @@ export interface ClassListRowProps { */ readonly testId?: string; readonly className?: string; + /** + * The shapes this class can be drawn as, when there is a choice between them. + * + * Absent is the ordinary row and the ordinary case: a class accepting one + * geometry has nothing to decide, and a list of fifty of them should carry + * fifty *names* rather than fifty controls. Present turns the geometry text + * into a segmented control — the active shape lit, pressing another switching + * the tool without moving the class. + * + * **Present also changes the row's markup**, and that is not an implementation + * detail worth hiding. This component's whole shape is "a real ` + {shapes.map((shape) => ( + + ))} + {hotkey != null && ( + + {hotkey} + + )} +
+ ); + } + return (