From 3b039384c47d21896d296a5b093b46df8e794585 Mon Sep 17 00:00:00 2001 From: MelekTaus Date: Tue, 15 Sep 2026 10:21:20 +0200 Subject: [PATCH] A search that ignores what a thing is called is no search, so the name wins (#43) Typing "variable" into the node palette landed on `Run behavior`, because the filter weighed label, type and description alike and then sorted by category. `searchRank` in `src/ui/search.ts` now ranks a match: exact label or type, then prefix, then label, then type, and only last a hit that lives in the description alone. Category and alphabet order within a rank. The searchable `Menu` uses the same function, so searching means the same thing everywhere. The `+` menu grew to fifteen entries in declaration order. It is built in two alphabetical groups now -- the types that go into a scene, then the ones that stay in the library -- with a separator between them, the split `isPlaceableType` already draws. `ASSET_TYPES` itself stays untouched: it is also the serialisation and check order. A search flattens the groups and drops the lines. Popups were capped at fixed heights (320 menu, 260 palette, 340 pickers) while the canvas had room to spare. `Popup.roomFor()` measures its own chrome and takes the window height minus a tenth at each end. Placement clamps asymmetrically: the bottom margin is enforced, the top is only a budget, so a menu anchored near the top stays at its button instead of being pushed off it. Co-Authored-By: Claude Opus 5 --- src/editor/debugHook.ts | 10 ++- src/editor/font/FontPicker.ts | 5 +- src/editor/graph/NodePalette.ts | 55 ++++++++++------ src/editor/graph/paletteEntries.ts | 29 ++++++++ src/editor/panels/AssetPanel.ts | 32 ++++++--- src/editor/panels/assetMenu.ts | 17 +++++ src/editor/sound/SoundPicker.ts | 5 +- src/editor/texture/ImagePicker.ts | 5 +- src/ui/index.ts | 11 +++- src/ui/search.ts | 21 ++++++ src/ui/widgets/Menu.ts | 98 +++++++++++++++++++++++----- src/ui/widgets/Popup.ts | 46 ++++++++++++- src/ui/widgets/Separator.ts | 22 +++++++ tests/editor/asset-menu.test.ts | 36 ++++++++++ tests/editor/palette-ranking.test.ts | 52 +++++++++++++++ tests/ui/popup-placement.test.ts | 36 ++++++++++ tests/ui/search.test.ts | 37 +++++++++++ tools/e2e/04-assets.mjs | 55 +++++++++++++++- tools/e2e/06-graph.mjs | 44 +++++++++++++ tools/e2e/lib/scaffold.mjs | 9 +-- 20 files changed, 570 insertions(+), 55 deletions(-) create mode 100644 src/editor/panels/assetMenu.ts create mode 100644 src/ui/search.ts create mode 100644 src/ui/widgets/Separator.ts create mode 100644 tests/editor/asset-menu.test.ts create mode 100644 tests/editor/palette-ranking.test.ts create mode 100644 tests/ui/popup-placement.test.ts create mode 100644 tests/ui/search.test.ts diff --git a/src/editor/debugHook.ts b/src/editor/debugHook.ts index 809ecca8..67fe33c3 100644 --- a/src/editor/debugHook.ts +++ b/src/editor/debugHook.ts @@ -4,13 +4,14 @@ import { resolveObjects } from '../core/model/instance'; import { findObjectHolder } from '../core/model/Project'; import type { FrameRect } from '../core/model/texture'; import { effectiveScale, gameClock } from '../core/time'; -import { activeDrag, Button, isRendering, renderCount, requestRender, ui, Widget } from '../ui'; +import { activeDrag, Button, currentMenu, isRendering, renderCount, requestRender, ui, Widget } from '../ui'; import { particleLibrary } from '../viewport/ParticleLibrary'; import { shaderLibrary } from '../viewport/ShaderLibrary'; import { soundLibrary } from '../viewport/SoundLibrary'; import { textureLibrary } from '../viewport/TextureLibrary'; import type { CentreTab, Editor } from './Editor'; import { currentFontPicker } from './font/FontPicker'; +import { currentNodePalette } from './graph/NodePalette'; import { currentSoundPicker } from './sound/SoundPicker'; import { currentImagePicker } from './texture/ImagePicker'; import type { EditorState } from './state'; @@ -192,6 +193,8 @@ export function installDebugHook(app: Application, state: EditorState, editor: E textColor, overlayTexts, overlayRect, + /** Entries of the open menu in drawn order, `null` for a separator line (#43). */ + menuRows: () => currentMenu()?.rowsDrawn() ?? null, textOverflows, labelsDropped, shownLabel, @@ -266,6 +269,11 @@ export function installDebugHook(app: Application, state: EditorState, editor: E canvasRect: () => graphEditor.canvas.canvasRect(), /** Bare canvas below every node – where a step right-clicks to open the palette. */ emptySpot: () => graphEditor.canvas.emptySpotGlobal(), + /** + * What the open palette lists, in order. No colour check can see a ranking: the rows all + * look alike, only their order carries the answer (#43). + */ + paletteRows: () => currentNodePalette()?.rowsDrawn() ?? null, selected: () => state.selectedNodeIds, dropTarget: () => graphEditor.canvas.pendingDropTarget(), danglingEdge: () => graphEditor.canvas.danglingEdgeVisible(), diff --git a/src/editor/font/FontPicker.ts b/src/editor/font/FontPicker.ts index fac7e62c..8a192876 100644 --- a/src/editor/font/FontPicker.ts +++ b/src/editor/font/FontPicker.ts @@ -246,7 +246,10 @@ export class FontPicker extends Popup { if (asset.type === 'font') named.set(asset.file, asset.name); } this.list.setItems(stored.map((item) => ({ ...item, assetName: named.get(item.file) ?? null }))); - this.scroll.minHeight = Math.min(LIST_HEIGHT, Math.max(ROW_HEIGHT, heightFor(stored.length))); + this.scroll.minHeight = Math.min( + this.roomFor(this.scroll, ROW_HEIGHT), + Math.max(ROW_HEIGHT, heightFor(stored.length)), + ); this.status.setText( stored.length === 1 ? '1 font on the server' : `${stored.length} fonts on the server`, ); diff --git a/src/editor/graph/NodePalette.ts b/src/editor/graph/NodePalette.ts index 86879b53..8b0d6997 100644 --- a/src/editor/graph/NodePalette.ts +++ b/src/editor/graph/NodePalette.ts @@ -1,8 +1,20 @@ -import type { NodeCategory, NodeDef } from '../../core/graph'; +import type { NodeDef } from '../../core/graph'; import { ListView, Popup, ScrollContainer, TextField, Theme, ui } from '../../ui'; -import type { PaletteEntry } from './paletteEntries'; +import { type PaletteEntry, rankedEntries } from './paletteEntries'; -const CATEGORY_ORDER: NodeCategory[] = ['event', 'flow', 'variable', 'data', 'action', 'state']; +let current: NodePalette | null = null; + +function setCurrent(palette: NodePalette): void { + current = palette; +} + +/** The palette standing open, for the test hook. */ +export function currentNodePalette(): NodePalette | null { + return current; +} + +/** Below this the list would jump in height with every keystroke while typing a query. */ +const MIN_LIST_HEIGHT = 260; /** Searchable list of node types; right-click menu of the graph editor. */ export class NodePalette extends Popup { @@ -35,7 +47,7 @@ export class NodePalette extends Popup { }, }); this.content.add(this.search); - this.scroll.minHeight = 260; + this.scroll.minHeight = MIN_LIST_HEIGHT; this.scroll.setContent(this.list); this.content.add(this.scroll, 1); this.list.rowHeight = Theme.rowHeight - 2; @@ -49,27 +61,32 @@ export class NodePalette extends Popup { override open(globalX: number, globalY: number): void { super.open(globalX, globalY); + setCurrent(this); this.picked = false; ui().focus.focus(this.search); } + override close(): void { + if (current === this) current = null; + super.close(); + } + + /** The offered nodes in the order they are listed, best answer first (test hook). */ + rowsDrawn(): { label: string; type: string }[] { + return this.filtered.map((entry) => ({ label: entry.def.label, type: entry.def.type })); + } + private refresh(): void { - const query = this.search.value.trim().toLowerCase(); - this.filtered = this.entries - .filter((e) => this.accepts(e.def)) - .filter( - ({ def }) => - !query || - def.label.toLowerCase().includes(query) || - def.type.toLowerCase().includes(query) || - def.description.toLowerCase().includes(query), - ) - .sort( - (a, b) => - CATEGORY_ORDER.indexOf(a.def.category) - CATEGORY_ORDER.indexOf(b.def.category) || - a.def.label.localeCompare(b.def.label), - ); + this.filtered = rankedEntries( + this.entries.filter((e) => this.accepts(e.def)), + this.search.value, + ); this.list.setItems(this.filtered.map((e) => ({ id: e.id, label: e.def.label, hint: e.hint }))); + this.scroll.minHeight = Math.min( + Math.max(this.list.measure().h, MIN_LIST_HEIGHT), + this.roomFor(this.scroll), + ); + this.refreshLayout(); this.scroll.relayoutContent(); } diff --git a/src/editor/graph/paletteEntries.ts b/src/editor/graph/paletteEntries.ts index 25562b3d..bc1323d1 100644 --- a/src/editor/graph/paletteEntries.ts +++ b/src/editor/graph/paletteEntries.ts @@ -1,9 +1,11 @@ +import { searchRank } from '../../ui/search'; import { allNodeDefs, BEHAVIOR_PARAM, createEmptyGraph, isTerminalType, nextStateName, + type NodeCategory, type NodeDef, resolveDef, RUN_BEHAVIOR, @@ -36,6 +38,33 @@ export interface PaletteEntry { params: PropertyBag; } +const CATEGORY_ORDER: NodeCategory[] = ['event', 'flow', 'variable', 'data', 'action', 'state']; + +/** + * The entries a query answers, best first. How well the entry matches decides before anything else; + * category and alphabet only order what is equally good. A node named after the query therefore + * beats one that merely mentions it in its description – without that, "variable" + Enter adds + * `Run behavior`, whose description happens to contain the word. + * + * An empty query ranks everything alike, which leaves the plain category order of the palette. + */ +export function rankedEntries(entries: PaletteEntry[], query: string): PaletteEntry[] { + const needle = query.trim().toLowerCase(); + return entries + .map((entry) => ({ + entry, + rank: searchRank(needle, [entry.def.label, entry.def.type, entry.def.description]), + })) + .filter(({ rank }) => rank >= 0) + .sort( + (a, b) => + a.rank - b.rank || + CATEGORY_ORDER.indexOf(a.entry.def.category) - CATEGORY_ORDER.indexOf(b.entry.def.category) || + a.entry.def.label.localeCompare(b.entry.def.label), + ) + .map(({ entry }) => entry); +} + /** * The node types offerable in `asset`'s graph. A state machine holds states and nothing else, each * new one named by its number; a shader holds shader nodes, its one Output dropping out once diff --git a/src/editor/panels/AssetPanel.ts b/src/editor/panels/AssetPanel.ts index db60c922..098af121 100644 --- a/src/editor/panels/AssetPanel.ts +++ b/src/editor/panels/AssetPanel.ts @@ -4,7 +4,6 @@ import { oneStep } from '../../core/commands/CompoundCommand'; import { nextAssetName } from '../../core/commands/naming'; import { ASSET_TYPE_LABELS, - ASSET_TYPES, type AssetType, hasColorSwatch, isPlaceableType, @@ -18,7 +17,17 @@ import { assetDeviates } from '../../core/properties/deviation'; import type { FontStore } from '../../core/store/FontStore'; import type { SoundStore } from '../../core/store/SoundStore'; import type { TextureStore } from '../../core/store/TextureStore'; -import { Button, ListView, Menu, type MenuItem, Panel, ScrollContainer, startDrag } from '../../ui'; +import { + Button, + ListView, + Menu, + type MenuEntry, + type MenuItem, + Panel, + ScrollContainer, + startDrag, +} from '../../ui'; +import { assetMenuGroups } from './assetMenu'; import { confirmDeleteAsset } from '../actions'; import type { EditorState } from '../state'; import { openFontPicker } from '../font/FontPicker'; @@ -48,13 +57,7 @@ export class AssetPanel extends Panel { this.minWidth = 160; const add = new Button('+', { variant: 'ghost', width: 28 }); - add.onClick = () => - new Menu( - ASSET_TYPES.map((type) => ({ - label: ASSET_TYPE_LABELS[type], - onSelect: () => this.addAsset(type), - })), - ).openBelow(add); + add.onClick = () => new Menu(this.addMenuEntries()).openBelow(add); this.header.add(add); this.list.emptyText = 'No assets – press +'; @@ -71,6 +74,17 @@ export class AssetPanel extends Panel { this.refresh(); } + /** One entry per asset type, the groups of `assetMenuGroups` kept apart by a line. */ + private addMenuEntries(): MenuEntry[] { + const entry = (type: AssetType): MenuEntry => ({ + label: ASSET_TYPE_LABELS[type], + onSelect: () => this.addAsset(type), + }); + return assetMenuGroups().flatMap((group, index) => + index === 0 ? group.map(entry) : [{ separator: true }, ...group.map(entry)], + ); + } + private refresh(): void { const active = this.state.project.activeSceneId; this.list.setItems( diff --git a/src/editor/panels/assetMenu.ts b/src/editor/panels/assetMenu.ts new file mode 100644 index 00000000..d6b9481e --- /dev/null +++ b/src/editor/panels/assetMenu.ts @@ -0,0 +1,17 @@ +import { ASSET_TYPE_LABELS, ASSET_TYPES, type AssetType, isPlaceableType } from '../../core/model/AssetTypes'; + +/** + * The `+` menu in two alphabetical groups: what goes into a scene, then what only lives in the + * library. At fifteen types declaration order is nothing anyone can search by eye, and the group + * answers the question that comes right after "which type?" – whether the thing will be placed. + * + * Ordered here and not in `ASSET_TYPES`: that array is also the serialisation and validation order, + * and at least one check reads a list derived from it. + */ +export function assetMenuGroups(): AssetType[][] { + const byLabel = (a: AssetType, b: AssetType) => ASSET_TYPE_LABELS[a].localeCompare(ASSET_TYPE_LABELS[b]); + return [ + ASSET_TYPES.filter(isPlaceableType).sort(byLabel), + ASSET_TYPES.filter((type) => !isPlaceableType(type)).sort(byLabel), + ]; +} diff --git a/src/editor/sound/SoundPicker.ts b/src/editor/sound/SoundPicker.ts index 8eb50702..480d068d 100644 --- a/src/editor/sound/SoundPicker.ts +++ b/src/editor/sound/SoundPicker.ts @@ -263,7 +263,10 @@ export class SoundPicker extends Popup { if (asset.type === 'sound') named.set(asset.file, asset.name); } this.list.setItems(stored.map((item) => ({ ...item, assetName: named.get(item.file) ?? null }))); - this.scroll.minHeight = Math.min(LIST_HEIGHT, Math.max(ROW_HEIGHT, heightFor(stored.length))); + this.scroll.minHeight = Math.min( + this.roomFor(this.scroll, ROW_HEIGHT), + Math.max(ROW_HEIGHT, heightFor(stored.length)), + ); this.status.setText( stored.length === 1 ? '1 sound on the server' : `${stored.length} sounds on the server`, ); diff --git a/src/editor/texture/ImagePicker.ts b/src/editor/texture/ImagePicker.ts index c6494666..6f04f93c 100644 --- a/src/editor/texture/ImagePicker.ts +++ b/src/editor/texture/ImagePicker.ts @@ -252,7 +252,10 @@ export class ImagePicker extends Popup { // Only as tall as the tiles need, up to the cap: a handful of pictures should not leave // half a dialog of empty box under them. const metrics = gridMetrics(GRID_WIDTH, stored.length); - this.scroll.minHeight = Math.min(GRID_HEIGHT, Math.max(metrics.tile + CAPTION_HEIGHT, metrics.height)); + this.scroll.minHeight = Math.min( + this.roomFor(this.scroll, metrics.tile + CAPTION_HEIGHT), + Math.max(metrics.tile + CAPTION_HEIGHT, metrics.height), + ); this.status.setText( stored.length === 1 ? '1 image on the server' : `${stored.length} images on the server`, ); diff --git a/src/ui/index.ts b/src/ui/index.ts index 46f1dfcb..0293a1d2 100644 --- a/src/ui/index.ts +++ b/src/ui/index.ts @@ -16,7 +16,16 @@ export { Splitter } from './widgets/Splitter'; export { ScrollContainer } from './widgets/ScrollContainer'; export { type DropSpot, ListView, type ListItem, type PickModifiers } from './widgets/ListView'; export { Popup, type PopupOptions } from './widgets/Popup'; -export { Menu, type MenuItem, type MenuOptions } from './widgets/Menu'; +export { + currentMenu, + Menu, + type MenuEntry, + type MenuItem, + type MenuOptions, + type MenuSeparator, +} from './widgets/Menu'; +export { Separator } from './widgets/Separator'; +export { searchRank } from './search'; export { Dialog, type DialogButton } from './widgets/Dialog'; export { TextField, type TextFieldOptions } from './widgets/TextField'; export { NumberField } from './widgets/NumberField'; diff --git a/src/ui/search.ts b/src/ui/search.ts new file mode 100644 index 00000000..b09e26ae --- /dev/null +++ b/src/ui/search.ts @@ -0,0 +1,21 @@ +/** + * How well a thing answers a search query, smaller is better, -1 for "not at all". + * + * `fields` are ordered by how much they count: a node's label first, its type next, its description + * last. Inside one field an exact hit beats a prefix beats a substring, and an exact hit counts as + * the best possible answer whichever field it came from – someone typing a whole word means that + * word. Without this, a flat "contains" filter lets a long description outrank the node that is + * literally named after the query. + */ +export function searchRank(query: string, fields: readonly string[]): number { + if (!query) return 0; + let best = -1; + fields.forEach((field, index) => { + const value = field.toLowerCase(); + const kind = value === query ? 0 : value.startsWith(query) ? 1 : value.includes(query) ? 2 : -1; + if (kind < 0) return; + const rank = kind === 0 ? 0 : index * 3 + kind; + if (best < 0 || rank < best) best = rank; + }); + return best; +} diff --git a/src/ui/widgets/Menu.ts b/src/ui/widgets/Menu.ts index 7e3be86c..c95174e2 100644 --- a/src/ui/widgets/Menu.ts +++ b/src/ui/widgets/Menu.ts @@ -1,10 +1,13 @@ import { VStack } from '../layout/Stack'; +import { searchRank } from '../search'; import { Theme } from '../Theme'; import { ui } from '../UIContext'; +import type { Widget } from '../Widget'; import { Button } from './Button'; import { Label } from './Label'; import { Popup } from './Popup'; import { ScrollContainer } from './ScrollContainer'; +import { Separator } from './Separator'; import { TextField } from './TextField'; export interface MenuItem { @@ -14,22 +17,34 @@ export interface MenuItem { disabled?: boolean; } +/** A hairline between two groups of entries; it is not selectable and matches no search. */ +export interface MenuSeparator { + separator: true; +} + +export type MenuEntry = MenuItem | MenuSeparator; + export interface MenuOptions { /** Adds a search box above the entries that filters as you type; Enter picks the first match. */ searchable?: boolean; } -const MAX_LIST_HEIGHT = 320; +interface Row { + widget: Widget; + /** Null for a separator: it is decoration between groups, not an entry. */ + button: Button | null; + label: string; +} /** A vertical list of clickable actions. Picking one closes the menu before it runs. */ export class Menu extends Popup { - private readonly entries: { button: Button; haystack: string }[] = []; + private readonly rows: Row[] = []; private readonly list = new VStack({ gap: 2 }); private readonly scroll = new ScrollContainer(); private readonly empty = new Label('No match', { style: 'muted', padding: Theme.padding }); private readonly search: TextField | null; - constructor(items: MenuItem[], options: MenuOptions = {}) { + constructor(items: MenuEntry[], options: MenuOptions = {}) { super({ padding: 4, gap: 4 }); this.minWidth = 160; this.search = options.searchable @@ -44,16 +59,18 @@ export class Menu extends Popup { if (this.search) this.content.add(this.search); for (const item of items) { + if ('separator' in item) { + this.rows.push({ widget: new Separator(), button: null, label: '' }); + continue; + } const button = new Button(item.label, { variant: item.danger ? 'danger' : 'ghost', align: 'left', }); button.setEnabled(!item.disabled); button.onClick = this.closeThen(item.onSelect); - this.list.add(button); - this.entries.push({ button, haystack: item.label.toLowerCase() }); + this.rows.push({ widget: button, button, label: item.label.toLowerCase() }); } - this.list.add(this.empty); this.scroll.setContent(this.list); this.content.add(this.scroll); this.filter(); @@ -61,23 +78,39 @@ export class Menu extends Popup { override open(globalX: number, globalY: number): void { super.open(globalX, globalY); + setCurrent(this); // open() blurs whatever had focus, so this has to come after it. if (this.search) ui().focus.focus(this.search); } + override close(): void { + if (current === this) current = null; + super.close(); + } + + /** The entries as they stand, in the order they are drawn; `null` is a separator (test hook). */ + rowsDrawn(): (string | null)[] { + return this.list.children + .filter((child) => child !== this.empty && child.visible) + .map((child) => (child instanceof Button ? child.text : null)); + } + private filter(): void { const query = this.search?.value.trim().toLowerCase() ?? ''; - let matches = 0; - for (const entry of this.entries) { - entry.button.visible = entry.haystack.includes(query); - if (entry.button.visible) matches++; - } - this.empty.visible = matches === 0; + const shown = query ? this.ranked(query) : groupsOf(this.rows); + + // Searching reorders, so the rows are re-hung rather than only hidden. removeChildren detaches + // them; the widgets themselves live in `this.rows` and survive every filter. + this.list.removeChildren(); + for (const row of shown) this.list.addChild(row.widget); + this.empty.visible = shown.length === 0; + this.list.addChild(this.empty); const size = this.list.measure(); - const scrolls = size.h > MAX_LIST_HEIGHT; + const room = this.roomFor(this.scroll); + const scrolls = size.h > room; this.scroll.minWidth = size.w + (scrolls ? Theme.scrollbarWidth + 4 : 0); - this.scroll.minHeight = Math.min(size.h, MAX_LIST_HEIGHT); + this.scroll.minHeight = Math.min(size.h, room); this.scroll.scrollTo(0); this.refreshLayout(); // Both are needed: a resize re-lays out only on an actual size change, and two filters can @@ -86,8 +119,41 @@ export class Menu extends Popup { this.list.relayout(); } + /** Best answers first; a query flattens the groups, so separators drop out with them. */ + private ranked(query: string): Row[] { + return this.rows + .map((row, index) => ({ row, index, rank: row.button ? searchRank(query, [row.label]) : -1 })) + .filter((entry) => entry.rank >= 0) + .sort((a, b) => a.rank - b.rank || a.index - b.index) + .map((entry) => entry.row); + } + private pickFirstMatch(): void { - const first = this.entries.find((entry) => entry.button.visible && entry.button.enabled); - first?.button.onClick?.(); + const first = this.list.children.find( + (child): child is Button => child instanceof Button && child.visible && child.enabled, + ); + first?.onClick?.(); } } + +let current: Menu | null = null; + +function setCurrent(menu: Menu): void { + current = menu; +} + +/** The menu standing open, for the test hook. */ +export function currentMenu(): Menu | null { + return current; +} + +/** Drops separators that would open, close or double up the list – a line needs two sides. */ +function groupsOf(rows: Row[]): Row[] { + const out: Row[] = []; + for (const row of rows) { + if (row.button) out.push(row); + else if (out.length > 0 && out[out.length - 1]!.button) out.push(row); + } + if (out.length > 0 && !out[out.length - 1]!.button) out.pop(); + return out; +} diff --git a/src/ui/widgets/Popup.ts b/src/ui/widgets/Popup.ts index c726ce9d..92e003c3 100644 --- a/src/ui/widgets/Popup.ts +++ b/src/ui/widgets/Popup.ts @@ -13,6 +13,35 @@ export interface PopupOptions { modal?: boolean; } +/** + * Left free around a popup, so it never touches the edge of the canvas. A share of the canvas + * rather than a constant: the same gap that frames a popup on a laptop looks like a hairline on a + * large screen, and the height cap is measured against the canvas anyway. + */ +const SCREEN_MARGIN = 0.1; + +/** The gap the layout reserves, in pixels on the current canvas. */ +export function screenMargin(screenHeight: number): number { + return Math.round(screenHeight * SCREEN_MARGIN); +} + +/** + * Where a popup of `height` ends up when it was asked to open at `wanted`. The margin has to be + * enforced here, not only in `roomFor`: a popup anchored low is *shifted* up to fit, and shifting + * it against the raw canvas height parks it on the bottom edge – which is why the gap used to be + * visible above and missing below. + * + * Downwards only, though. Above the popup the margin is what the height cap spends, not a distance + * to keep: a menu hanging off a button near the top of the screen belongs under its button, and + * pushing it down to the tenth would tear it away from what opened it. So the top edge of the + * canvas is the only limit upwards, and a popup taller than the canvas starts there instead of + * being centred out of view. + */ +export function clampPopupY(wanted: number, height: number, screenHeight: number): number { + const margin = screenMargin(screenHeight); + return clamp(wanted, 0, Math.max(0, screenHeight - margin - height)); +} + /** A floating box on the overlay layer with a click-away backdrop. */ export class Popup extends Widget { private readonly backdrop = new Graphics(); @@ -68,7 +97,7 @@ export class Popup extends Widget { const size = this.measure(); this.resize(size.w, size.h); const x = clamp(this.openX, 0, Math.max(0, screen.width - size.w)); - const y = clamp(this.openY, 0, Math.max(0, screen.height - size.h)); + const y = clampPopupY(this.openY, size.h, screen.height); this.position.set(Math.round(x), Math.round(y)); } @@ -96,6 +125,21 @@ export class Popup extends Widget { this.onClose?.(); } + /** + * How tall the scrolling part of this popup may become: the canvas, minus a margin, minus the + * chrome around it. The chrome is *measured* – everything the content holds besides `scroll` – + * rather than repeated as a constant, so a search field or a row of buttons cannot be forgotten. + * + * A fixed cap would either waste a tall screen or overflow a short one; the canvas is the only + * number that knows which of the two we are on. + */ + protected roomFor(scroll: Widget, minimum = Theme.rowHeight * 3): number { + const chrome = this.content.measure().h - scroll.measure().h; + const screenHeight = ui().app.screen.height; + const room = screenHeight - screenMargin(screenHeight) * 2 - chrome; + return Math.max(minimum, room); + } + /** Closes first, so `action` runs with the overlay already cleared. */ protected closeThen(action?: () => void): () => void { return () => { diff --git a/src/ui/widgets/Separator.ts b/src/ui/widgets/Separator.ts new file mode 100644 index 00000000..2e8330c8 --- /dev/null +++ b/src/ui/widgets/Separator.ts @@ -0,0 +1,22 @@ +import { Graphics } from 'pixi.js'; +import { Theme } from '../Theme'; +import { Widget } from '../Widget'; + +/** A hairline between two groups of entries. Takes no pointer events and answers no search. */ +export class Separator extends Widget { + private readonly line = new Graphics(); + + constructor() { + super(); + this.addChild(this.line); + this.minHeight = 5; + this.eventMode = 'none'; + } + + override redraw(): void { + this.line + .clear() + .rect(Theme.padding, Math.floor(this.h / 2), Math.max(0, this.w - Theme.padding * 2), 1) + .fill({ color: Theme.colors.border }); + } +} diff --git a/tests/editor/asset-menu.test.ts b/tests/editor/asset-menu.test.ts new file mode 100644 index 00000000..e75de820 --- /dev/null +++ b/tests/editor/asset-menu.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, it } from 'vitest'; +import { + ASSET_TYPE_LABELS, + ASSET_TYPES, + type AssetType, + isPlaceableType, +} from '../../src/core/model/AssetTypes'; +import { assetMenuGroups } from '../../src/editor/panels/assetMenu'; + +const labels = (types: AssetType[]) => types.map((type) => ASSET_TYPE_LABELS[type]); + +describe('assetMenuGroups', () => { + it('offers every type exactly once', () => { + const flat = assetMenuGroups().flat(); + expect([...flat].sort()).toEqual([...ASSET_TYPES].sort()); + }); + + it('splits into placeable first, library second (#43)', () => { + const [placeable, library] = assetMenuGroups(); + expect(placeable!.every(isPlaceableType)).toBe(true); + expect(library!.some(isPlaceableType)).toBe(false); + }); + + it('sorts each group by its label, not by declaration order', () => { + for (const group of assetMenuGroups()) { + expect(labels(group)).toEqual([...labels(group)].sort()); + } + // Declaration order starts with Scene, which is not placeable and no longer leads the menu. + expect(assetMenuGroups()[0]![0]).not.toBe('scene'); + }); + + it('leaves ASSET_TYPES alone – it is also the serialisation order', () => { + assetMenuGroups(); + expect(ASSET_TYPES[0]).toBe('scene'); + }); +}); diff --git a/tests/editor/palette-ranking.test.ts b/tests/editor/palette-ranking.test.ts new file mode 100644 index 00000000..e1537cb0 --- /dev/null +++ b/tests/editor/palette-ranking.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from 'vitest'; +import { CreateAssetCommand } from '../../src/core/commands/assetCommands'; +import { findAssetOf } from '../../src/core/model/Project'; +import { paletteEntries, rankedEntries } from '../../src/editor/graph/paletteEntries'; +import { makeProject } from '../helpers'; + +/** The palette of a fresh behavior: the real catalog, not a hand-made list. */ +function behaviorPalette() { + const f = makeProject(); + const create = new CreateAssetCommand('behavior', 'B'); + f.stack.execute(create); + const asset = findAssetOf(f.project, create.asset.id, 'behavior')!; + return paletteEntries(f.project, asset); +} + +const labels = (query: string) => rankedEntries(behaviorPalette(), query).map((e) => e.def.label); + +describe('rankedEntries', () => { + it('falls back to category and alphabet while nothing is typed', () => { + const entries = rankedEntries(behaviorPalette(), ''); + expect(entries.length).toBe(behaviorPalette().length); + const events = entries.filter((e) => e.def.category === 'event'); + expect(entries.slice(0, events.length)).toEqual(events); + expect(events.map((e) => e.def.label)).toEqual([...events.map((e) => e.def.label)].sort()); + }); + + it('puts the node named after the query first (#43)', () => { + // What the issue reported: "variable" + Enter took the first row, which was `Run behavior` + // because its description mentions variables. The entry that is *called* it comes first now. + const first = labels('variable')[0]!; + expect(first.toLowerCase()).toContain('variable'); + expect(first).not.toBe('Run behavior'); + }); + + it('ranks every label hit ahead of every description-only hit', () => { + const ranked = rankedEntries(behaviorPalette(), 'variable'); + const named = ranked.filter((e) => e.def.label.toLowerCase().includes('variable')); + const mentioned = ranked.filter((e) => !e.def.label.toLowerCase().includes('variable')); + expect(named.length).toBeGreaterThan(0); + expect(mentioned.length).toBeGreaterThan(0); + expect(ranked.indexOf(named[named.length - 1]!)).toBeLessThan(ranked.indexOf(mentioned[0]!)); + }); + + it('finds an exact label before anything that merely starts with it', () => { + expect(labels('log')[0]).toBe('Log'); + }); + + it('still matches type and description, so nothing becomes unfindable', () => { + expect(labels('Vsprchtnich')).toEqual([]); + expect(labels('onstart')).toContain('On start'); + }); +}); diff --git a/tests/ui/popup-placement.test.ts b/tests/ui/popup-placement.test.ts new file mode 100644 index 00000000..d2a2a8f8 --- /dev/null +++ b/tests/ui/popup-placement.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, it } from 'vitest'; +import { clampPopupY, screenMargin } from '../../src/ui/widgets/Popup'; + +describe('screenMargin', () => { + it('is a tenth of the canvas, so it grows with the screen', () => { + expect(screenMargin(800)).toBe(80); + expect(screenMargin(1440)).toBe(144); + }); +}); + +describe('clampPopupY', () => { + it('leaves a popup that already sits inside the band where it is', () => { + expect(clampPopupY(300, 200, 1000)).toBe(300); + }); + + it('keeps the same gap below as above when the anchor sits low', () => { + // 1000 tall canvas, 100 margin: a 700 tall popup asked for y=500 ends at 200, not at 300. + const y = clampPopupY(500, 700, 1000); + expect(y).toBe(200); + expect(1000 - (y + 700)).toBe(100); // the gap below matches the 100 above + }); + + it('leaves a popup anchored high where it is, the gap above is no minimum', () => { + // The + menu hangs off a button near the top; pushing it down to the margin would tear it away. + expect(clampPopupY(10, 200, 1000)).toBe(10); + expect(clampPopupY(0, 200, 1000)).toBe(0); + }); + + it('still keeps the gap below when a tall popup has to be shifted up', () => { + expect(clampPopupY(400, 900, 1000)).toBe(0); + }); + + it('gives up rather than shifting a popup taller than the canvas off screen', () => { + expect(clampPopupY(400, 1200, 1000)).toBe(0); + }); +}); diff --git a/tests/ui/search.test.ts b/tests/ui/search.test.ts new file mode 100644 index 00000000..46020164 --- /dev/null +++ b/tests/ui/search.test.ts @@ -0,0 +1,37 @@ +import { describe, expect, it } from 'vitest'; +import { searchRank } from '../../src/ui/search'; + +describe('searchRank', () => { + const fields = ['Set variable', 'setvariable', 'Writes a value into a variable']; + + it('ranks everything alike when nothing was typed', () => { + expect(searchRank('', fields)).toBe(0); + expect(searchRank('', ['anything at all'])).toBe(0); + }); + + it('answers -1 when no field contains the query', () => { + expect(searchRank('collider', fields)).toBe(-1); + }); + + it('puts an exact hit first, whichever field it came from', () => { + expect(searchRank('set variable', fields)).toBe(0); + expect(searchRank('setvariable', fields)).toBe(0); + }); + + it('prefers a prefix over a substring, and the label over the type', () => { + const prefix = searchRank('set', fields); + const substring = searchRank('variable', fields); + expect(prefix).toBeGreaterThan(0); + expect(prefix).toBeLessThan(substring); + }); + + it('ranks a description-only hit behind every hit on the label', () => { + const description = searchRank('writes', fields); + expect(description).toBeGreaterThan(searchRank('variable', fields)); + }); + + it('takes the best field, not the first that matches', () => { + // "value" sits in the description only, "Set" in the label -- a query in both takes the label. + expect(searchRank('set', ['Set value', 'setvalue', 'Set a value'])).toBe(1); + }); +}); diff --git a/tools/e2e/04-assets.mjs b/tools/e2e/04-assets.mjs index e398da56..c13ffae0 100644 --- a/tools/e2e/04-assets.mjs +++ b/tools/e2e/04-assets.mjs @@ -73,8 +73,61 @@ export async function assets(t) { await sleep(150); check('redo puts it back', (await objects()).length === 8); - // 4c. Create a new asset through the + menu. + // 4c. Create a new asset through the + menu. Since #43 the menu is two alphabetical groups – + // what can be placed in a scene, a line, then what only lives in the library – and it is + // tall enough to show all fifteen types without scrolling. await clickText('+'); + await sleep(150); + const rows = await json('__gm.menuRows()'); + const cut = rows.indexOf(null); + const placeable = rows.slice(0, cut); + const library = rows.slice(cut + 1); + check( + 'the + menu is split by exactly one line', + rows.filter((r) => r === null).length === 1 && cut > 0, + JSON.stringify(rows), + ); + check( + 'every asset type is offered once, each group sorted by label', + placeable.length + library.length === 15 && + String(placeable) === String([...placeable].sort()) && + String(library) === String([...library].sort()), + JSON.stringify({ placeable, library }), + ); + check( + 'placeable types come first, the library ones after the line', + placeable.includes('Sprite') && + !placeable.includes('Scene') && + library.includes('Scene') && + library.includes('Texture'), + JSON.stringify({ placeable, library }), + ); + // The height cap is the canvas, not a constant: fifteen entries used to run under the mask edge, + // where `locate` still finds them but a click lands on nothing (that is what took step 30 down). + await shot('tmp-plus-menu'); + const menuBox = await json('__gm.overlayRect()'); + const lastRow = await evaluate(`__gm.locate(${JSON.stringify(library.at(-1))}, true)`); + const screenH = await json('__gm.app.screen.height'); + check( + 'the whole menu stands on screen, last entry clickable', + Boolean(lastRow) && + lastRow.y < menuBox.y + menuBox.h && + menuBox.y + menuBox.h <= screenH && + menuBox.y >= 0, + JSON.stringify({ menuBox, lastRow, screenH }), + ); + // A tenth of the canvas stays free below every popup. The margin used to live only in the height + // cap, never in the placement, so a popup that had to be shifted up ended flush on the bottom + // edge with the whole gap collected above it. Above it is a budget, not a distance: this menu + // hangs off a button near the top and stays there. + const above = menuBox.y; + const below = screenH - (menuBox.y + menuBox.h); + const margin = Math.round(screenH * 0.1); + check( + 'the menu keeps a tenth of the canvas free below, and stays under its button', + below >= margin - 1 && above >= 0 && above < margin, + JSON.stringify({ above, below, margin, screenH }), + ); await clickText('Sprite'); await sleep(100); const names = await json('__gm.state.project.assets.map(a => a.name)'); diff --git a/tools/e2e/06-graph.mjs b/tools/e2e/06-graph.mjs index 9caeeafa..aa1775b7 100644 --- a/tools/e2e/06-graph.mjs +++ b/tools/e2e/06-graph.mjs @@ -101,6 +101,20 @@ export async function graph(t) { await click(bare.x, bare.y, 'right'); await sleep(100); check('node palette opens', Boolean(await evaluate(`__gm.locate('Search nodes…')`))); + // Unfiltered the palette holds far more nodes than fit, so it grows to the full height it may + // have – and that height is the canvas minus a tenth at each end. Shifting it up to fit used to + // put the entire gap above it and leave it flush on the bottom edge. Opened this far down it ends + // up with a tenth at both ends; the gap above is what is left over, not a rule of its own. + const paletteBox = await json('__gm.overlayRect()'); + const paletteScreenH = await json('__gm.app.screen.height'); + const paletteMargin = Math.round(paletteScreenH * 0.1); + const gapAbove = paletteBox.y; + const gapBelow = paletteScreenH - (paletteBox.y + paletteBox.h); + check( + 'a palette shifted up to fit keeps the tenth of the canvas below it', + Math.abs(gapBelow - paletteMargin) <= 1 && gapAbove >= 0, + JSON.stringify({ gapAbove, gapBelow, paletteMargin, paletteScreenH }), + ); await typeText('log'); await sleep(100); await press('Enter'); @@ -115,6 +129,36 @@ export async function graph(t) { ); await shot('17-node-added'); + // 6d2. The palette ranks by how well an entry answers the query (#43): a node *named* after it + // comes before one that merely mentions it in its description. "variable" used to hand + // Enter the first row alphabetically, which was `Run behavior`. + const beforeRank = await graphCounts(); + await click(bare.x, bare.y, 'right'); + await sleep(100); + await typeText('variable'); + await sleep(150); + const ranked = await json('__gm.graph.paletteRows()'); + const hits = (row) => `${row.label} ${row.type}`.toLowerCase().includes('variable'); + const lastNamed = ranked.map(hits).lastIndexOf(true); + const firstOther = ranked.map(hits).indexOf(false); + check( + 'the palette lists the nodes named after the query first', + lastNamed >= 0 && (firstOther === -1 || lastNamed < firstOther) && hits(ranked[0]), + JSON.stringify(ranked), + ); + await press('Enter'); + await sleep(250); + s = await state(); + const rankPick = s.selection?.kind === 'node' ? await nodeOf(s.selection.nodeId) : null; + check( + 'Enter takes that node, not the one whose description mentions it', + rankPick !== null && rankPick.type.toLowerCase().includes('variable'), + JSON.stringify(rankPick), + ); + await undo(); + await sleep(150); + check('undo removes it again', (await graphCounts()).nodes === beforeRank.nodes); + // 6e. Connect: drag from the Move node's exec output onto the Log node's exec input. An // exec output must not connect to a data input: dragging onto "message" does nothing. const fromPort = await json(`__gm.graph.portAt('n-move', 'exec', true)`); diff --git a/tools/e2e/lib/scaffold.mjs b/tools/e2e/lib/scaffold.mjs index 7929186b..afed95cf 100644 --- a/tools/e2e/lib/scaffold.mjs +++ b/tools/e2e/lib/scaffold.mjs @@ -132,10 +132,11 @@ export async function openTab(t, tab) { } /** - * Picks an asset type from the already-open `+` menu. The menu has one entry per type and scrolls - * past 320 px, so the last of them sit under the mask: `locate` still finds them, but a click there - * lands on nothing. The fourteenth type (`Variables`, #91) pushed `Font` under that edge and took - * step 30 down with it, because only step 31 had scrolled first. + * Picks an asset type from the already-open `+` menu. Since #43 the menu may grow to the height of + * the canvas, so on this window every type is usually on screen at once; the scroll stays in as + * insurance for a shorter window, where entries below the mask edge are still found by `locate` + * but a click there lands on nothing. That is what the fourteenth type (`Variables`, #91) did to + * `Font` and to step 30. */ export async function pickAssetType(t, label) { const box = await t.json('__gm.overlayRect()');