diff --git a/DESIGN.md b/DESIGN.md index 562fd27e..59513fac 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -498,6 +498,14 @@ market segment and the domain does not hold one. The single map is quietly out of a list; headings are non-selectable `SelectLabel`s and a category with nothing under it renders none. +**An option that is an identifier plus the facts about it takes two lines** (#472, +2026-08-09) — the identifier at the label role, the facts beneath it at the meta role in +`muted-foreground`. It is `SelectItem`'s `meta` prop, so the closed trigger shows the same +two lines the open list does; the trigger is `min-h-9` rather than `h-9` and grows to fit, +which leaves every one-line select on the contract's 36px. **Nothing truncates**: an +identifier cut off in the middle is not an identifier, so a long one wraps. The specimen is +on the styleguide page. + ### Lists and filtering Any list that can exceed ~20 rows carries a filter input. Filtering is client-side and diff --git a/docs/ui.md b/docs/ui.md index 93547bb9..51c9aa92 100644 --- a/docs/ui.md +++ b/docs/ui.md @@ -434,7 +434,18 @@ because the model reads the whole image once; refining after it is quick. **The proposal is not an annotation until it is accepted.** It is drawn faintly with a dashed outline, carries its class and the model's confidence beside it, and is in neither the document nor the undo history. `Esc` is its undo. Switching -tools, switching frames or leaving the page discards it, and nothing is written. +class, switching frames or leaving the page discards it, and nothing is written. + +**The tool stays armed while you change class.** Arming it is a decision about how +to work, and picking the class to work on is the next thing you do — so a class +switch ends the proposal on screen and not the tool. The next click asks under the +new class, in its geometry and its colour. Only pressing the button again, or +moving to another frame, puts the tool away. + +Land on a class that can hold no proposal — a tag, a lane — and the tool **parks** +rather than switching itself off: the button dims and says why, the panel says what +to pick, and the canvas goes back to drawing that class normally. Choose a box or a +polygon class again and the tool carries on, with nothing to press. Accepting creates one ordinary annotation, in one undo step, carrying `provenance: model`, the `model_ref` the answer named and its `confidence` — the @@ -444,7 +455,10 @@ frame settles the same way. **The tool is offered only for a class that can hold the answer.** The proposal comes back as a polygon for a polygon class and as the shape's bounding box for a box class; a schema whose classes are tag-only or lane-only gets no button at all, -because there is no kind the answer could be expressed in. +because there is no kind the answer could be expressed in. That is the *project* +answered; the parked state above is the same question asked of the class you are +holding, and it dims the button rather than removing it because the answer changes +again the moment you pick another class. Arming it with no usable connection shows an in-editor panel saying what is missing — none configured, or configured with its weights not yet downloaded — and diff --git a/frontend/annotator/src/adapters/react/paint.test.ts b/frontend/annotator/src/adapters/react/paint.test.ts index 42021e6d..8e072c96 100644 --- a/frontend/annotator/src/adapters/react/paint.test.ts +++ b/frontend/annotator/src/adapters/react/paint.test.ts @@ -303,6 +303,14 @@ describe("a pending suggestion, drawn as a proposal (#424)", () => { expect(paintSuggestion(cleared(shown()), SIGN)).toBeNull(); }); + it("draws nothing for a parked session, which has no class to draw it in (#472)", () => { + // Constructed, because a parked session cannot reach `shown` — the same + // belt-and-braces as the kind check below, for a function exported from the + // package root that a caller could hand anything the type permits. + const parked: SuggestionState = { ...shown(), labelClass: null }; + expect(paintSuggestion(parked, SIGN)).toBeNull(); + }); + it("draws nothing for a kind that is not one of the two suggestible ones", () => { // Unreachable through the route, which narrows to `allowed_geometries` — and // still refused here, because this function is exported from the package root diff --git a/frontend/annotator/src/adapters/react/paint.ts b/frontend/annotator/src/adapters/react/paint.ts index f2ded7b9..74d38342 100644 --- a/frontend/annotator/src/adapters/react/paint.ts +++ b/frontend/annotator/src/adapters/react/paint.ts @@ -291,12 +291,17 @@ export function paintSuggestion( ): PaintedSuggestion | null { const suggestion = state.suggestion; if (state.status !== "shown" || suggestion === null) return null; + // A parked session (#472) has no class, so there is no colour to draw it in and + // no label to write on it. It also cannot be `shown`, so this is the same kind + // of guard as the one above: what the type allows, not what the machine does. + const labelClass = state.labelClass; + if (labelClass === null) return null; const geometry = suggestion.geometry; if (geometry.type !== "bbox" && geometry.type !== "polygon") return null; return { geometry, - color: classColor(declared, state.labelClass), - label: confidenceLabel(state.labelClass, suggestion.confidence), + color: classColor(declared, labelClass), + label: confidenceLabel(labelClass, suggestion.confidence), points: state.points, }; } diff --git a/frontend/annotator/src/core/interaction/suggestion.test.ts b/frontend/annotator/src/core/interaction/suggestion.test.ts index a7e63521..d3e96a27 100644 --- a/frontend/annotator/src/core/interaction/suggestion.test.ts +++ b/frontend/annotator/src/core/interaction/suggestion.test.ts @@ -25,11 +25,14 @@ import { cleared, hasPending, isAcceptable, + isParked, isSuggestibleClass, promptOf, refused, schemaCanSuggest, suggestClassFor, + suggestibleClassIn, + withClass, withPoint, } from "./suggestion"; import type { Suggestion, SuggestionState } from "./suggestion"; @@ -118,6 +121,100 @@ describe("which classes the tool is offered for", () => { expect(suggestClassFor(schemaOf(LANE, ROAD, CAR), "lane")).toBe("road"); expect(suggestClassFor(schemaOf(LANE, ROAD, CAR), null)).toBe("road"); }); + + it("reads the held class without the fallback, which is the parked question (#472)", () => { + const schema = schemaOf(LANE, ROAD, CAR); + expect(suggestibleClassIn(schema, "car")).toBe("car"); + // The very case `suggestClassFor` answers `road` for. Moving somebody off the + // class they just picked is not this function's business. + expect(suggestibleClassIn(schema, "lane")).toBe(null); + expect(suggestibleClassIn(schema, null)).toBe(null); + expect(suggestibleClassIn(schema, "not-a-class")).toBe(null); + }); +}); + +/** + * The class moving under an armed session (#472) — the behaviour #451 shipped, and + * the direction it was deliberately turned around in. + */ +describe("the active class moves and the session goes with it", () => { + it("returns the state by identity when the class did not really move", () => { + const state = showing(); + // Not merely equal: the host folds this through on every render of the class + // it is already on, so a fresh object here would be a re-render per keystroke + // and, worse, a discarded preview. + expect(withClass(state, "car")).toBe(state); + }); + + it("stays armed on the new class, with nothing pending carried over", () => { + const state = withClass(armed("car"), "road"); + expect(state.labelClass).toBe("road"); + expect(state.status).toBe("idle"); + expect(isParked(state)).toBe(false); + expect(hasPending(state)).toBe(false); + }); + + it("discards a preview the new class may not be able to hold", () => { + const shown = showing(); + expect(shown.suggestion).not.toBe(null); + + const moved = withClass(shown, "road"); + + // The shape was answered under `car`'s `allowed_geometries`; accepting it + // under `road` could write a kind that class does not admit. + expect(moved.suggestion).toBe(null); + expect(moved.points).toEqual([]); + expect(moved.status).toBe("idle"); + expect(isAcceptable(moved)).toBe(false); + // And the tool is still armed, which is the whole of the change. + expect(moved.labelClass).toBe("road"); + }); + + it("keeps the serial counting, so the answer in flight cannot repaint", () => { + const asked = withPoint(armed("car"), [10, 10], "positive"); + const moved = withClass(asked, "road"); + + expect(moved.serial).toBe(asked.serial); + // The ask that was in flight lands under the old serial and is dropped whole. + expect(answered(moved, asked.serial - 1, proposal())).toBe(moved); + + // And the next click on the new class cannot be answered by it either: the + // serial moves on rather than being handed back out. + const again = withPoint(moved, [20, 20], "positive"); + expect(again.serial).toBe(asked.serial + 1); + }); + + it("parks on a class that can hold nothing, rather than ending", () => { + const state = withClass(showing(), null); + expect(isParked(state)).toBe(true); + expect(state.labelClass).toBe(null); + expect(state.suggestion).toBe(null); + expect(state.points).toEqual([]); + }); + + it("re-arms from parked on the class that unparked it, with no second press", () => { + const parked = withClass(armed("car"), null); + const back = withClass(parked, "road"); + + expect(isParked(back)).toBe(false); + // `road`, not `car`: a parked session resumes on the class the person has just + // picked, and nothing here remembers the one they left. + expect(back.labelClass).toBe("road"); + expect(back.status).toBe("idle"); + }); + + it("stays parked while the active class moves between classes that hold nothing", () => { + const parked = withClass(armed("car"), null); + expect(withClass(parked, null)).toBe(parked); + }); + + it("writes nothing while parked, whatever a caller believes about the status", () => { + const document = documentOf(); + // Constructed, not reachable: a parked session cannot be `shown`. The guard is + // the guarantee — no class, no annotation — and not a formality. + const impossible: SuggestionState = { ...showing(), labelClass: null }; + expect(acceptedAnnotation(document, impossible, () => "id-1")).toBe(null); + }); }); describe("the preview lifecycle", () => { diff --git a/frontend/annotator/src/core/interaction/suggestion.ts b/frontend/annotator/src/core/interaction/suggestion.ts index f37ffbc5..948212d1 100644 --- a/frontend/annotator/src/core/interaction/suggestion.ts +++ b/frontend/annotator/src/core/interaction/suggestion.ts @@ -61,6 +61,33 @@ * that can be true. `promptOf` is the projection, and it is here rather than in * the host so that the ordering rule — positives and negatives in the order they * were placed, each list on its own — has one owner. + * + * ## The class can move under a session, and the session survives it (#472) + * + * #451 held the mode *beside* the active class and discarded it whenever that + * class moved, on the reading that moving the active class is how this build + * spells switching tools. In use that is wrong: arming the tool is a decision + * about how to work, and picking the class to work on is the next thing somebody + * does. Directed (Armando, 2026-08-09): a session ends when the user puts the + * tool away or the asset changes, and a class switch is neither. + * + * {@link withClass} is that whole rule, and it is one function because the three + * readings are one transition: + * + * - **Nothing pending.** The class is swapped in; the next click asks under it. + * - **A preview showing.** It is discarded with the clicks that produced it. It + * was answered under the old class's `allowed_geometries`, so accepting it + * under the new one could write a shape that class does not admit — the same + * argument D3 makes about which classes may be asked in the first place. + * - **A class that can hold nothing** — a tag, a path. The session **parks**: + * `labelClass` is `null`, no click is diverted, and the armed intent is kept so + * that returning to a class which can hold an answer picks up where it left + * off. Parking rather than disarming is what stops a capability disappearing + * without a sentence (principle 9); the sentence itself is the host's. + * + * The serial keeps counting across all three, for `cleared`'s reason: an answer + * to the ask that was in flight when the class moved must not repaint a preview + * that belongs to a class nobody is on any more. */ import { classNamed } from "../state/document"; @@ -133,6 +160,28 @@ export function schemaCanSuggest(schema: AnnotationSchema): boolean { return schema.classes.some(isSuggestibleClass); } +/** + * The active class if a suggestion could be labelled with it, and `null` if not. + * + * What {@link withClass} is fed when the active class moves, so `null` is the + * parked reading rather than an error: a schema that can suggest can still be + * sitting on a class that cannot, and that is a state to describe rather than one + * to prevent. + * + * Deliberately **not** {@link suggestClassFor}, which falls back to the schema's + * first suggestible class. That fallback is right when somebody presses the tool + * — they asked for it, so take them somewhere it works — and wrong here, where + * they asked for a *class*: moving the active class out from under a person who + * just picked one is the consequence `ToolPalette` refuses to have. + */ +export function suggestibleClassIn( + schema: AnnotationSchema, + activeClass: string | null, +): string | null { + const held = schema.classes.find((declared) => declared.name === activeClass); + return held !== undefined && isSuggestibleClass(held) ? held.name : null; +} + /** What a click told the model: this is the thing, or this is not the thing. */ export type Polarity = "positive" | "negative"; @@ -168,8 +217,14 @@ export type SuggestionStatus = "idle" | "asking" | "shown" | "none" | "refused"; /** The whole of a suggest session. `null`, in a host, is a tool that is not armed. */ export interface SuggestionState { - /** The class the accepted annotation will carry. Fixed for the session. */ - readonly labelClass: string; + /** + * The class the accepted annotation will carry. + * + * `null` is a **parked** session: armed, but sitting on a class that can hold + * nothing a segmenter proposes, so there is nothing to ask about. It is not an + * absent value to be defaulted — see {@link withClass} and {@link isParked}. + */ + readonly labelClass: string | null; /** Every click so far, in the order they were placed. */ readonly points: readonly PromptPoint[]; readonly status: SuggestionStatus; @@ -283,6 +338,37 @@ export function cleared(state: SuggestionState): SuggestionState { return { ...state, points: [], status: "idle", suggestion: null, refusal: null }; } +/** + * The active class moved. The tool stays armed; what was pending does not. + * + * One function for the three readings the module note sets out — swap, discard, + * park — because they are one transition over a different argument. A class that + * did not actually move returns the state **by identity**, so a host can fold + * this through unconditionally without a render or a discarded preview for a + * re-selection of the class already held. + * + * `null` parks. Re-arming is this same call with a class that can hold an answer, + * which is why nothing here remembers *which* class was parked from: the class a + * parked session resumes on is the one the user has just picked, not the one they + * left. + */ +export function withClass(state: SuggestionState, labelClass: string | null): SuggestionState { + if (labelClass === state.labelClass) return state; + return { ...cleared(state), labelClass }; +} + +/** + * Whether the session is armed over a class that can hold nothing. + * + * The one thing a host must not do with a parked session is divert a press into + * it: the class the user is on may still be drawable — a lane is a `polyline` — + * and a tool that swallowed those presses would have stopped being parked and + * started being broken. + */ +export function isParked(state: SuggestionState): boolean { + return state.labelClass === null; +} + /** Whether there is anything for Escape to take back. */ export function hasPending(state: SuggestionState): boolean { return state.points.length > 0 || state.status !== "idle"; @@ -318,6 +404,10 @@ export function acceptedAnnotation( mint: IdFactory, ): Annotation | null { if (!isAcceptable(state) || state.suggestion === null) return null; + // A parked session cannot reach `shown`, so this is unreachable by the state + // machine — and it is the guarantee, not a formality: nothing may be written + // without a class, whatever a caller believes about how it got here. + if (state.labelClass === null) return null; if (classNamed(document, state.labelClass) === undefined) return null; const drawn = draftAnnotation(document, state.labelClass, state.suggestion.geometry, mint); return { diff --git a/frontend/annotator/src/index.ts b/frontend/annotator/src/index.ts index 04e449c9..1740c5ab 100644 --- a/frontend/annotator/src/index.ts +++ b/frontend/annotator/src/index.ts @@ -156,11 +156,14 @@ export { cleared, hasPending, isAcceptable, + isParked, isSuggestibleClass, promptOf, refused, schemaCanSuggest, suggestClassFor, + suggestibleClassIn, + withClass, withPoint, type Polarity, type Prompt, diff --git a/frontend/app/e2e/styleguide.spec.ts b/frontend/app/e2e/styleguide.spec.ts index eadc51d5..b99dc8a9 100644 --- a/frontend/app/e2e/styleguide.spec.ts +++ b/frontend/app/e2e/styleguide.spec.ts @@ -177,3 +177,30 @@ test("the tab bar sits one rhythm step above its content", async ({ page }) => { expect(panel).not.toBeNull(); expect(panel!.y - (list!.y + list!.height)).toBeCloseTo(12, 0); }); + +/** + * #472's claim, which is a claim about pixels. + * + * `primitives.test.tsx` asserts the *structure* — two elements, the meta in the + * muted role, the id keeping its own line — and jsdom computes no layout, so the + * one thing it cannot see is the thing that was reported: a two-line value inside + * a control measured for one line. A revert to `h-9` leaves every unit test green + * and fails here. + */ +test("a two-line option grows its trigger, and a one-line one stays on the contract's 36px", async ({ + page, +}) => { + const plain = await page.getByLabel("Geometry").boundingBox(); + const stacked = await page.getByLabel("Model").boundingBox(); + expect(plain).not.toBeNull(); + expect(stacked).not.toBeNull(); + + // Unmoved: every select that shipped before the variant is still exactly 36px. + expect(plain!.height).toBeCloseTo(36, 0); + // Grown, not squashed: the second line is inside the box rather than over it. + expect(stacked!.height).toBeGreaterThan(plain!.height); + + // And the identifier is whole — no ellipsis, no clipped end. + const id = page.getByLabel("Model").locator("span", { hasText: "facebook/sam2.1-hiera-base-plus" }).first(); + await expect(id).toHaveCSS("text-overflow", "clip"); +}); diff --git a/frontend/app/src/styleguide/Styleguide.tsx b/frontend/app/src/styleguide/Styleguide.tsx index 9348d140..0e27afd4 100644 --- a/frontend/app/src/styleguide/Styleguide.tsx +++ b/frontend/app/src/styleguide/Styleguide.tsx @@ -235,6 +235,44 @@ export function Styleguide(): JSX.Element { Singular per class — picking a class picks a tool. + {/* + The two-line option (#472). Here because it is a primitive variant + rather than one screen's styling: an option that is an identifier + plus the facts about it stacks them, and the trigger shows the same + two lines the list does because Radix renders the selected item's + own text into it. + */} +
+ + + + The id at the label role, what it costs beneath it. Nothing truncates. + +