From 4d07946c740ea2afcddf523d89372f31e935221b Mon Sep 17 00:00:00 2001 From: root Date: Thu, 10 Sep 2026 10:58:40 +0200 Subject: [PATCH] Lay out a scroll container's content when the content changes A short list opened after a long one drew all its rows stacked on one point. The fault was not in the list tab: rebuild() handed the row stack to setContent() while it was still empty, so the content was sized to max(rows, viewport) = the viewport. The rows arrived after that, and for a short list the same formula yields the same height, so resize() saw no change and skipped layout(). A long list changes the height and never showed it. relayoutContent() now runs whenever the content changed, rather than leaning on resize()'s change detection - that detection is the wrong instrument for "the children moved". The list tab was the only affected caller: the panels use ListView, which positions its rows itself, and the inspector fills its body before setContent. Also drops the comma line under the list. Every section that can carry a list supplies openList, so the comma fallback in fieldFactory was unreachable; listToText/listFromText go with it. The list-value test now covers coerceListItem through coerceValue instead, so the boolean spelling stays guarded. Co-Authored-By: Claude Opus 5 --- README.md | 10 +++--- src/core/graph/nodes/data/ListLiteral.ts | 2 +- src/core/properties/PropertySchema.ts | 13 -------- src/editor/inspector/fieldFactory.ts | 26 ++++----------- src/editor/list/ListEditor.ts | 42 ++++-------------------- src/ui/widgets/ScrollContainer.ts | 7 +++- tests/core/list-value.test.ts | 22 ++++++++----- tests/ui/scroll-container.test.ts | 38 +++++++++++++++++++++ tools/e2e/21-list.mjs | 29 ++++++++++++++-- 9 files changed, 102 insertions(+), 87 deletions(-) create mode 100644 tests/ui/scroll-container.test.ts diff --git a/README.md b/README.md index d35f33dc..ef0bee5a 100644 --- a/README.md +++ b/README.md @@ -99,10 +99,10 @@ shown on the node and editable in the inspector. handle that no reload survives. A set of objects is a list of their names, walked with `Repeat` and `Object by name`. A list property is shown as a button saying how many items it holds; clicking it opens the **List** tab, which gives each item a row with the same widget the - inspector would use for that kind — plus the comma-separated line, kept for pasting a whole - list at once. The inspector stays visible beside the tab, so what the list belongs to is still - on screen. Which list the tab shows is a `ListSlot` (asset property, object override or node - setting) on the editor state, because no tab may reach into the `Editor` to switch itself on. + inspector would use for that kind. The inspector stays visible beside the tab, so what the list + belongs to is still on screen. Which list the tab shows is a `ListSlot` (asset property, object + override or node setting) on the editor state, because no tab may reach into the `Editor` to + switch itself on. - **Owner** = parent: a placed behavior is usually parented to the object it controls (inspector → Parent, or drag it onto that object in the hierarchy; dashed line in the viewport). The `Owner` node returns it; `Self` returns the behavior itself; `Parent` walks up @@ -569,7 +569,7 @@ npm run format # Prettier over src, tests and tools npm run check # typecheck + lint + format:check + tests, what CI would run npm run build # writes dist/ and regenerates index.html export GM_REMOTE=user@host # optional: run the browser elsewhere; see below -node tools/seed-e2e.mjs && node tools/e2e.mjs # 436 editor checks, screenshots in /tmp/gm-e2e-*.png +node tools/seed-e2e.mjs && node tools/e2e.mjs # 438 editor checks, screenshots in /tmp/gm-e2e-*.png GM_STEPS=20 node tools/e2e.mjs # or 1-12, or 6,20 – a subset while working on one step node tools/seed-jumprun.mjs && node tools/smoke-jumprun.mjs # 24 checks: the example game, played end to end node tools/seed-tetris.mjs && node tools/smoke-tetris.mjs # 19 checks: the Tetris example, played end to end diff --git a/src/core/graph/nodes/data/ListLiteral.ts b/src/core/graph/nodes/data/ListLiteral.ts index 5b022b20..c4845007 100644 --- a/src/core/graph/nodes/data/ListLiteral.ts +++ b/src/core/graph/nodes/data/ListLiteral.ts @@ -6,7 +6,7 @@ export const ListLiteral: NodeDef = { type: 'List', label: 'List', category: 'data', - description: 'A constant list, typed as one comma-separated line.', + description: 'A constant list, edited item by item in the List tab.', inputs: [], outputs: [list('value')], params: [{ key: 'value', label: 'Value', kind: 'list', default: [], items: 'number' }], diff --git a/src/core/properties/PropertySchema.ts b/src/core/properties/PropertySchema.ts index a76f0bc3..42f8f3e4 100644 --- a/src/core/properties/PropertySchema.ts +++ b/src/core/properties/PropertySchema.ts @@ -225,19 +225,6 @@ export function coerceValue(def: PropertyDef, value: PropertyValue): FieldValue } } -/** Colors read back as `#rrggbb` so what the inspector shows can be typed in again unchanged. */ -export function listToText(list: ListValue, items: ListItemKind): string { - return list - .map((v) => (items === 'color' ? `#${Number(v).toString(16).padStart(6, '0')}` : String(v))) - .join(', '); -} - -export function listFromText(text: string, items: ListItemKind): ListValue { - const trimmed = text.trim(); - if (trimmed === '') return []; - return trimmed.split(',').map((part) => coerceListItem(part.trim(), items)); -} - export function coerceListItem(value: PropertyValue, items: ListItemKind): number | string | boolean { if (items === 'text') return isList(value) ? value.join(', ') : textOf(value); if (items === 'boolean') { diff --git a/src/editor/inspector/fieldFactory.ts b/src/editor/inspector/fieldFactory.ts index 73c28cb4..ed854754 100644 --- a/src/editor/inspector/fieldFactory.ts +++ b/src/editor/inspector/fieldFactory.ts @@ -6,7 +6,7 @@ import { assetRefLabel, NO_ASSET_LABEL, } from '../../core/properties/assetRef'; -import { listFromText, listToText, type PropertyDef } from '../../core/properties/PropertySchema'; +import type { PropertyDef } from '../../core/properties/PropertySchema'; import { NO_TEXTURE_LABEL, textureRefChoices, textureRefLabel } from '../../core/properties/textureRef'; import { Button, @@ -39,10 +39,7 @@ export interface FieldHandlers { onInput: (value: PropertyValue) => void; /** Final value – commit as a command. */ onChange: (value: PropertyValue) => void; - /** - * Sends a list to the List tab. Only the caller knows where the value it edits lives, so a - * list row falls back to the comma-separated text field wherever this is not given. - */ + /** Sends a list to the List tab; only the caller knows where the value it edits lives. */ openList?: () => void; } @@ -200,20 +197,11 @@ export function createField( return { widget: field, grow: true, setValue: (v) => field.setValue(Number(v)) }; } case 'list': { - const asList = (v: PropertyValue) => (isList(v) ? v : []); - const { openList } = handlers; - if (openList) { - // A board is hundreds of entries: the row says how many and hands the editing to the - // List tab, where a row per item has the space for it. - const count = (v: PropertyValue) => `${asList(v).length} items`; - const button = new Button(count(value), { variant: 'ghost', onClick: openList }); - return { widget: button, grow: true, setValue: (v) => button.setText(count(v)) }; - } - const asText = (v: PropertyValue) => listToText(asList(v), def.items); - const field = new TextField(asText(value), { - onChange: (text) => handlers.onChange(listFromText(String(text), def.items)), - }); - return { widget: field, grow: true, setValue: (v) => field.setValue(asText(v)) }; + // A board is hundreds of entries: the row says how many and hands the editing to the + // List tab, where a row per item has the space for it. + const count = (v: PropertyValue) => `${(isList(v) ? v : []).length} items`; + const button = new Button(count(value), { variant: 'ghost', onClick: handlers.openList }); + return { widget: button, grow: true, setValue: (v) => button.setText(count(v)) }; } case 'boolean': { const field = new Checkbox(Boolean(value)); diff --git a/src/editor/list/ListEditor.ts b/src/editor/list/ListEditor.ts index 73dbcf79..932880de 100644 --- a/src/editor/list/ListEditor.ts +++ b/src/editor/list/ListEditor.ts @@ -1,25 +1,10 @@ /** - * Edits one list, a row per item, with the same field widgets the inspector builds. A list row in - * the inspector has one line for a value that can be hundreds long; the inspector stays visible - * beside this tab, so what the list belongs to is still on screen while it is edited. + * Edits one list, a row per item, with the same field widgets the inspector builds -- a value that + * can be hundreds long has no room in an inspector row. The inspector stays visible beside this + * tab, so what the list belongs to is still on screen while it is edited. */ -import { - listFromText, - type ListItemKind, - listToText, - type PropertyDef, -} from '../../core/properties/PropertySchema'; -import { - Button, - HStack, - Label, - ScrollContainer, - Spacer, - TextField, - Theme, - VStack, - type Widget, -} from '../../ui'; +import type { ListItemKind, PropertyDef } from '../../core/properties/PropertySchema'; +import { Button, HStack, Label, ScrollContainer, Spacer, Theme, VStack, type Widget } from '../../ui'; import { createField, type FieldContext } from '../inspector/fieldFactory'; import type { EditorState } from '../state'; import { type ListAccess, resolveList } from './slot'; @@ -34,7 +19,6 @@ export class ListEditor extends VStack { private readonly count = new Label('', { style: 'small', align: 'right' }); private readonly scroll = new ScrollContainer(); private readonly addButton: Button; - private readonly bulk: TextField; private rows = new VStack({ padding: Theme.padding, gap: Theme.gap }); /** The structure the rows were built for; only a change here may destroy a row being typed in. */ private signature = ''; @@ -54,16 +38,10 @@ export class ListEditor extends VStack { this.scroll.setContent(this.rows); this.add(this.scroll, 1); - // The comma text is what the inspector used to show. It stays, as the way to paste a whole - // list at once – per-item rows do not replace that. const footer = new HStack({ gap: Theme.gap, padding: Theme.padding, align: 'center' }); this.addButton = new Button('+ Item', { variant: 'ghost', onClick: () => this.addItem() }); footer.add(this.addButton); - this.bulk = new TextField('', { - placeholder: 'comma-separated', - onChange: (text) => this.setAll(text), - }); - footer.add(this.bulk, 1); + footer.add(new Spacer(), 1); this.add(footer); state.on('list', () => this.rebuild()); @@ -109,7 +87,6 @@ export class ListEditor extends VStack { } if (!access) return; this.setters.forEach((set, i) => set(access.value[i] ?? '')); - this.bulk.setValue(listToText(access.value, access.def.items)); } private rebuild(): void { @@ -124,12 +101,10 @@ export class ListEditor extends VStack { this.title.setText(access ? access.title : 'No list open'); this.count.setText(access ? `${access.value.length} items` : ''); this.addButton.visible = access !== null; - this.bulk.visible = access !== null; if (!access) { this.rows.add(new Label('Open a list from its row in the inspector.', { style: 'muted' })); } else { - this.bulk.setValue(listToText(access.value, access.def.items)); access.value.forEach((item, index) => this.rows.add(this.itemRow(access, item, index))); if (access.value.length === 0) this.rows.add(new Label('The list is empty.', { style: 'muted' })); } @@ -162,11 +137,6 @@ export class ListEditor extends VStack { const access = this.access(); if (access) access.replace([...access.value, itemDef(access.def.items, 0).default]); } - - private setAll(text: string): void { - const access = this.access(); - if (access) access.replace(listFromText(text, access.def.items)); - } } /** One item as a property definition, so the inspector's own widget renders it. */ diff --git a/src/ui/widgets/ScrollContainer.ts b/src/ui/widgets/ScrollContainer.ts index cfde123d..1f19f582 100644 --- a/src/ui/widgets/ScrollContainer.ts +++ b/src/ui/widgets/ScrollContainer.ts @@ -76,7 +76,12 @@ export class ScrollContainer extends Widget { relayoutContent(): void { if (!this.content) return; const size = this.content.measure(); - this.content.resize(this.w, Math.max(size.h, this.h)); + const height = Math.max(size.h, this.h); + // Content shorter than the viewport is pinned to the viewport height either way, so `resize` + // would see no change and skip the layout -- leaving rows added since stacked at the origin. + const unchanged = this.content.w === this.w && this.content.h === height; + this.content.resize(this.w, height); + if (unchanged) this.content.layout(); this.scrollY = clamp(this.scrollY, 0, this.maxScroll); this.applyScroll(); } diff --git a/tests/core/list-value.test.ts b/tests/core/list-value.test.ts index e505e92c..d5fb5b78 100644 --- a/tests/core/list-value.test.ts +++ b/tests/core/list-value.test.ts @@ -4,7 +4,7 @@ import { SetOverrideCommand } from '../../src/core/commands/sceneObjectCommands' import { sameValue, textOf } from '../../src/core/model/Properties'; import { findAsset, objectHolders, type Project } from '../../src/core/model/Project'; import { deserializeProject } from '../../src/core/store/serialize'; -import { coerceValue, listFromText, listToText, variableDef } from '../../src/core/properties/PropertySchema'; +import { coerceValue, variableDef } from '../../src/core/properties/PropertySchema'; import { deviatesFromAsset } from '../../src/core/properties/resolve'; import { asEntity, asStruct, writeTyped } from '../../src/core/graph'; import { fakeEntity, makeBehavior, makeSpriteScene, place, roundTrip } from '../helpers'; @@ -78,15 +78,19 @@ describe('a list as a value', () => { expect(deviatesFromAsset(placed, asset, 'row')).toBe(true); }); - it('reads and writes as one comma-separated line, item kind by item kind', () => { - expect(listFromText('1, 2, 3', 'number')).toEqual([1, 2, 3]); - expect(listFromText('', 'number')).toEqual([]); - expect(listFromText('a, b', 'text')).toEqual(['a', 'b']); + it('coerces a loaded item to its declared kind, kind by kind', () => { + expect(coerceValue(variableDef('n', 'list', [], 'number'), ['1', 2, 'x'])).toEqual([1, 2, 0]); + expect(coerceValue(variableDef('t', 'list', [], 'text'), [1, true])).toEqual(['1', 'true']); // Spelling has to count here: Boolean('false') would be true. - expect(listFromText('true, false, 0', 'boolean')).toEqual([true, false, false]); - expect(listFromText('#ff0000, #00ff00', 'color')).toEqual([0xff0000, 0x00ff00]); - expect(listToText([0xff0000], 'color')).toBe('#ff0000'); - expect(listToText([1, 2], 'number')).toBe('1, 2'); + expect(coerceValue(variableDef('b', 'list', [], 'boolean'), ['true', 'false', '0', ''])).toEqual([ + true, + false, + false, + false, + ]); + expect(coerceValue(variableDef('c', 'list', [], 'color'), ['#ff0000', 0x00ff00])).toEqual([ + 0xff0000, 0x00ff00, + ]); }); it('a value that is not a list falls back to the default rather than becoming one', () => { diff --git a/tests/ui/scroll-container.test.ts b/tests/ui/scroll-container.test.ts new file mode 100644 index 00000000..c229327c --- /dev/null +++ b/tests/ui/scroll-container.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, it } from 'vitest'; +import { ScrollContainer, Spacer, VStack } from '../../src/ui'; + +/** Row tops after a layout; an unlaid-out stack leaves every child at 0. */ +const tops = (stack: VStack) => stack.children.map((c) => c.y); + +describe('a scroll container told its content changed', () => { + it('lays out rows that leave the height pinned to the viewport', () => { + const scroll = new ScrollContainer(); + scroll.resize(200, 300); + const rows = new VStack(); + // The list tab hands over the stack before filling it, so the empty content is measured first + // and pinned to the viewport height. + scroll.setContent(rows); + rows.add(new Spacer(20)); + rows.add(new Spacer(20)); + rows.add(new Spacer(20)); + + // 60 < 300, so the content keeps the height it already had: `resize` alone would see no + // change and skip the layout, leaving all three rows stacked at the top. + scroll.relayoutContent(); + expect(tops(rows)).toEqual([0, 20, 40]); + }); + + it('still lays out rows that make the content taller than the viewport', () => { + const scroll = new ScrollContainer(); + scroll.resize(200, 50); + const rows = new VStack(); + scroll.setContent(rows); + rows.add(new Spacer(20)); + rows.add(new Spacer(20)); + rows.add(new Spacer(20)); + + scroll.relayoutContent(); + expect(tops(rows)).toEqual([0, 20, 40]); + expect(scroll.maxScroll).toBe(10); + }); +}); diff --git a/tools/e2e/21-list.mjs b/tools/e2e/21-list.mjs index 32b5e716..5a6fb7d5 100644 --- a/tools/e2e/21-list.mjs +++ b/tools/e2e/21-list.mjs @@ -52,7 +52,30 @@ export async function listTab(t) { info = await json('__gm.list.info()'); check('undo takes just that item back', JSON.stringify(info.items) === '["30","11","44"]', info.items[1]); - // 4. Adding and removing whole items. + // 4. A short list opened after a long one. Its rows leave the content height pinned to the + // viewport, so a layout skipped on "same size" would stack them all at the top. + await mutate(` + const a = __gm.state.project.assets.find((x) => x.id === 'listy'); + a.props.nums = Array.from({ length: 60 }, (_, i) => i); + __gm.state.preview(); + `); + await sleep(250); + const longRows = [await json('__gm.list.fieldAt(0)'), await json('__gm.list.fieldAt(1)')]; + check('a long list stacks its rows', longRows[0].y !== longRows[1].y, JSON.stringify(longRows)); + await mutate(` + const a = __gm.state.project.assets.find((x) => x.id === 'listy'); + a.props.nums = [30, 11, 44]; + __gm.state.preview(); + `); + await sleep(250); + const shortRows = [await json('__gm.list.fieldAt(0)'), await json('__gm.list.fieldAt(1)')]; + check( + 'and a short list after it still stacks its own', + shortRows[0].y !== shortRows[1].y, + JSON.stringify(shortRows), + ); + + // 5. Adding and removing whole items. await clickText('+ Item'); await sleep(200); info = await json('__gm.list.info()'); @@ -61,7 +84,7 @@ export async function listTab(t) { check('undo removes it again', (await json('__gm.list.info()')).items.length === 3); await shot('03-rows'); - // 5. Back on the scene tab the button carries the current count. + // 6. Back on the scene tab the button carries the current count. await mutate('__gm.state.openList(null)'); await sleep(200); check('closing the list leaves the tab', (await json('__gm.tab()')) === 'scene'); @@ -72,7 +95,7 @@ export async function listTab(t) { await sleep(200); check('and the count follows the value', (await find('2 items')) !== null); - // 6. A list whose target is gone empties the tab rather than showing a stale one. + // 7. A list whose target is gone empties the tab rather than showing a stale one. await mutate("__gm.state.openList({ kind: 'asset', assetId: 'listy', key: 'nums' })"); await sleep(200); check('a list can be opened from outside the inspector too', (await json('__gm.tab()')) === 'list');