Keep edited list values when an item is deleted (#235)
The list tab read the list once when it built its rows. Editing a value does not rebuild the rows, so the delete button later filtered that old copy and wrote it back. Every edit since the last rebuild was lost. Adding an item only triggered it, because adding rebuilds the rows and that state became the old copy. - ListAccess.value in src/editor/list/slot.ts is now read on every access instead of once when the access is built. - The kind switch of a row and the struct field buttons (field type, rename, remove field, + Field) read the item when clicked, not when the row was built. They had the same stale copy. - New test hook __gm.list.removeAt(i) clicks the delete button of one row, because the tab shows many buttons with the same label. Not covered by a test of their own: the struct field buttons and the kind switch after an edit. The cause is the same, the fix is in code. Tests: - ./check.sh: 2435 tests, CHECK EXIT 0. - Unit test in tests/editor/list-slot.test.ts keeps one access, edits a value, adds and edits an item, deletes another. All edits stay. It fails without the fix. - e2e 21b-ii with real clicks: set row 2 to 66, delete row 0, expect ["11","66"], two undos take both back. e2e 21: 52 checks passed. - Counter probe: with the old snapshot the check fails with ["11","44"], the reported bug. - Check total in README is now 1400 (+2), counted, not measured. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
1d18862ed6
commit
085e6c8cce
6 files changed
+64
-9
No files matched your search
@@ -43,7 +43,7 @@ npm run build # writes dist/ and regenerates index.html; the folder is wh
|
|||||||
The browser suite and the three example games need a machine with a GPU:
|
The browser suite and the three example games need a machine with a GPU:
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
node tools/seed-e2e.mjs && node tools/e2e.mjs # the editor, 1398 checks (GM_STEPS=20 for one step)
|
node tools/seed-e2e.mjs && node tools/e2e.mjs # the editor, 1400 checks (GM_STEPS=20 for one step)
|
||||||
# every tool signs in first – see the Accounts page
|
# every tool signs in first – see the Accounts page
|
||||||
node tools/seed-jumprun.mjs && node tools/smoke-jumprun.mjs
|
node tools/seed-jumprun.mjs && node tools/smoke-jumprun.mjs
|
||||||
node tools/seed-tetris.mjs && node tools/smoke-tetris.mjs
|
node tools/seed-tetris.mjs && node tools/smoke-tetris.mjs
|
||||||
|
|||||||
@@ -88,6 +88,7 @@ export function documentHooks({ state, editor }: HookContext) {
|
|||||||
/** The kind mark of a row, or of one field of a struct item – both are pickers. */
|
/** The kind mark of a row, or of one field of a struct item – both are pickers. */
|
||||||
markAt: (index: number, field = '') => editor.listEditor.markAt(index, field),
|
markAt: (index: number, field = '') => editor.listEditor.markAt(index, field),
|
||||||
nameAt: (index: number, field: string) => editor.listEditor.nameAt(index, field),
|
nameAt: (index: number, field: string) => editor.listEditor.nameAt(index, field),
|
||||||
|
removeAt: (index: number) => editor.listEditor.removeAt(index),
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
@@ -54,6 +54,7 @@ const INDEX_WIDTH = 40;
|
|||||||
interface RowWidgets {
|
interface RowWidgets {
|
||||||
value: Widget;
|
value: Widget;
|
||||||
mark: Widget;
|
mark: Widget;
|
||||||
|
remove?: Widget;
|
||||||
parts: Map<string, Widget>;
|
parts: Map<string, Widget>;
|
||||||
marks: Map<string, Widget>;
|
marks: Map<string, Widget>;
|
||||||
names: Map<string, Widget>;
|
names: Map<string, Widget>;
|
||||||
@@ -127,6 +128,11 @@ export class ListEditor extends VStack {
|
|||||||
return centreOf(field ? row?.marks.get(field) : row?.mark);
|
return centreOf(field ? row?.marks.get(field) : row?.mark);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** For the check scripts: the button that deletes a row – every row has the same `×`. */
|
||||||
|
removeAt(index: number): { x: number; y: number } | null {
|
||||||
|
return centreOf(this.rowWidgets[index]?.remove);
|
||||||
|
}
|
||||||
|
|
||||||
/** For the check scripts: the name field of one field of a struct item. */
|
/** For the check scripts: the name field of one field of a struct item. */
|
||||||
nameAt(index: number, field: string): { x: number; y: number } | null {
|
nameAt(index: number, field: string): { x: number; y: number } | null {
|
||||||
return centreOf(this.rowWidgets[index]?.names.get(field));
|
return centreOf(this.rowWidgets[index]?.names.get(field));
|
||||||
@@ -211,7 +217,7 @@ export class ListEditor extends VStack {
|
|||||||
// Through `untag`, so a color keeps its number when it becomes one.
|
// Through `untag`, so a color keeps its number when it becomes one.
|
||||||
kindMenu(
|
kindMenu(
|
||||||
access.kinds.filter((next) => next !== kind),
|
access.kinds.filter((next) => next !== kind),
|
||||||
(next) => this.replaceItem(access, index, makeItem(next, untag(item))),
|
(next) => this.replaceItem(access, index, makeItem(next, untag(access.value[index]))),
|
||||||
),
|
),
|
||||||
);
|
);
|
||||||
if (tall) mark.minHeight = Theme.rowHeight;
|
if (tall) mark.minHeight = Theme.rowHeight;
|
||||||
@@ -228,7 +234,10 @@ export class ListEditor extends VStack {
|
|||||||
this.setters.push((next) => field.setValue(untag(next) ?? ''));
|
this.setters.push((next) => field.setValue(untag(next) ?? ''));
|
||||||
this.rowWidgets.push(rowWidgets(mark, field.widget));
|
this.rowWidgets.push(rowWidgets(mark, field.widget));
|
||||||
}
|
}
|
||||||
row.add(removeButton(() => access.replace(access.value.filter((_, i) => i !== index))));
|
const remove = removeButton(() => access.replace(access.value.filter((_, i) => i !== index)));
|
||||||
|
row.add(remove);
|
||||||
|
const widgets = this.rowWidgets[index];
|
||||||
|
if (widgets) widgets.remove = remove;
|
||||||
return row;
|
return row;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -242,6 +251,8 @@ export class ListEditor extends VStack {
|
|||||||
): void {
|
): void {
|
||||||
const bundle = structItemOf(item);
|
const bundle = structItemOf(item);
|
||||||
const fields = structItemFields(item);
|
const fields = structItemFields(item);
|
||||||
|
// A field value edit does not rebuild the row, so every button reads the item as it is now.
|
||||||
|
const now = () => structItemOf(access.value[index]);
|
||||||
const stack = new VStack({ gap: Theme.gap });
|
const stack = new VStack({ gap: Theme.gap });
|
||||||
const widgets = rowWidgets(mark, stack);
|
const widgets = rowWidgets(mark, stack);
|
||||||
const setters = new Map<string, (value: PropertyValue) => void>();
|
const setters = new Map<string, (value: PropertyValue) => void>();
|
||||||
@@ -255,13 +266,13 @@ export class ListEditor extends VStack {
|
|||||||
FIELD_KINDS.filter((next) => next !== field.kind),
|
FIELD_KINDS.filter((next) => next !== field.kind),
|
||||||
(next) =>
|
(next) =>
|
||||||
this.replaceItem(access, index, {
|
this.replaceItem(access, index, {
|
||||||
...bundle,
|
...now(),
|
||||||
[field.key]: makeFlatItem(next, untag(bundle[field.key])),
|
[field.key]: makeFlatItem(next, untag(now()[field.key])),
|
||||||
}),
|
}),
|
||||||
),
|
),
|
||||||
);
|
);
|
||||||
const name = nameField(field.key, (to) => {
|
const name = nameField(field.key, (to) => {
|
||||||
if (to && to !== field.key) this.replaceItem(access, index, renamedField(bundle, field.key, to));
|
if (to && to !== field.key) this.replaceItem(access, index, renamedField(now(), field.key, to));
|
||||||
});
|
});
|
||||||
line.add(fieldMark);
|
line.add(fieldMark);
|
||||||
line.add(name);
|
line.add(name);
|
||||||
@@ -275,12 +286,12 @@ export class ListEditor extends VStack {
|
|||||||
widgets.parts.set(field.key, sub.widget);
|
widgets.parts.set(field.key, sub.widget);
|
||||||
widgets.marks.set(field.key, fieldMark);
|
widgets.marks.set(field.key, fieldMark);
|
||||||
widgets.names.set(field.key, name);
|
widgets.names.set(field.key, name);
|
||||||
line.add(removeButton(() => this.replaceItem(access, index, withoutField(bundle, field.key))));
|
line.add(removeButton(() => this.replaceItem(access, index, withoutField(now(), field.key))));
|
||||||
stack.add(line);
|
stack.add(line);
|
||||||
}
|
}
|
||||||
const foot = new HStack({ gap: Theme.gap, align: 'center' });
|
const foot = new HStack({ gap: Theme.gap, align: 'center' });
|
||||||
foot.minHeight = Theme.rowHeight;
|
foot.minHeight = Theme.rowHeight;
|
||||||
foot.add(addFieldMenu((kind) => this.addField(access, index, bundle, kind)));
|
foot.add(addFieldMenu((kind) => this.addField(access, index, now(), kind)));
|
||||||
foot.add(new Spacer(), 1);
|
foot.add(new Spacer(), 1);
|
||||||
stack.add(foot);
|
stack.add(foot);
|
||||||
row.add(stack, 1);
|
row.add(stack, 1);
|
||||||
|
|||||||
@@ -30,6 +30,7 @@ export type ListDef = Extract<PropertyDef, { kind: 'list' }>;
|
|||||||
/** What the tab needs of a slot: what is in it, and the two ways of writing it back. */
|
/** What the tab needs of a slot: what is in it, and the two ways of writing it back. */
|
||||||
export interface ListAccess {
|
export interface ListAccess {
|
||||||
readonly def: ListDef;
|
readonly def: ListDef;
|
||||||
|
/** What is in the list now – re-read on every access. */
|
||||||
readonly value: ListValue;
|
readonly value: ListValue;
|
||||||
/**
|
/**
|
||||||
* The kinds this list may hold. A list in a struct field takes primitives only – `struct → list →
|
* The kinds this list may hold. A list in a struct field takes primitives only – `struct → list →
|
||||||
@@ -119,7 +120,11 @@ function access<Target>(
|
|||||||
def,
|
def,
|
||||||
kinds,
|
kinds,
|
||||||
title,
|
title,
|
||||||
value: current(),
|
// Re-read, not a snapshot: an item edit does not rebuild the rows, so a delete or kind change
|
||||||
|
// clicked later has to build on the list with that edit in it, not on the one the rows showed.
|
||||||
|
get value() {
|
||||||
|
return current();
|
||||||
|
},
|
||||||
itemHandlers: (index) => ({
|
itemHandlers: (index) => ({
|
||||||
...previewingHandlers<Target>(
|
...previewingHandlers<Target>(
|
||||||
{ state },
|
{ state },
|
||||||
|
|||||||
@@ -74,6 +74,20 @@ describe('the address a list is edited through', () => {
|
|||||||
expect(list().value).toEqual([10, 1, 20]);
|
expect(list().value).toEqual([10, 1, 20]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The tab resolves once per rebuild and keeps that access for its delete, add and kind buttons,
|
||||||
|
* and an item edit does not rebuild. A delete clicked after an edit has to keep that edit.
|
||||||
|
*/
|
||||||
|
it('deletes from the list as it is now, so an edit made after the rows were built stays', () => {
|
||||||
|
const held = list();
|
||||||
|
held.itemHandlers(0).onChange(10);
|
||||||
|
held.replace([...held.value, 5]);
|
||||||
|
held.itemHandlers(3).onChange(50);
|
||||||
|
held.replace(held.value.filter((_, i) => i !== 1));
|
||||||
|
expect(list().value).toEqual([10, 4, 50]);
|
||||||
|
expect(held.value).toEqual([10, 4, 50]);
|
||||||
|
});
|
||||||
|
|
||||||
/** The row keeps the kind of the item in it: a number field may only ever hand back a number. */
|
/** The row keeps the kind of the item in it: a number field may only ever hand back a number. */
|
||||||
it('writes what it is given as the kind the item already is', () => {
|
it('writes what it is given as the kind the item already is', () => {
|
||||||
list().itemHandlers(1).onChange('42');
|
list().itemHandlers(1).onChange('42');
|
||||||
|
|||||||
@@ -57,6 +57,30 @@ export async function listTab(t) {
|
|||||||
info = await json('__gm.list.info()');
|
info = await json('__gm.list.info()');
|
||||||
check('undo takes just that item back', JSON.stringify(info.items) === '["30","11","44"]', info.items[1]);
|
check('undo takes just that item back', JSON.stringify(info.items) === '["30","11","44"]', info.items[1]);
|
||||||
|
|
||||||
|
// 21b-ii. An item edit does not rebuild the rows, so a delete clicked after it must build on the
|
||||||
|
// edited list – with the one the rows were built from, the edit came back undone (#235).
|
||||||
|
const edited = await json('__gm.list.fieldAt(2)');
|
||||||
|
await click(edited.x, edited.y);
|
||||||
|
await typeText('66');
|
||||||
|
await press('Enter');
|
||||||
|
const removeFirst = await json('__gm.list.removeAt(0)');
|
||||||
|
await click(removeFirst.x, removeFirst.y);
|
||||||
|
await sleep(200);
|
||||||
|
info = await json('__gm.list.info()');
|
||||||
|
check(
|
||||||
|
'deleting a row keeps an edit made to another row before',
|
||||||
|
JSON.stringify(info.items) === '["11","66"]',
|
||||||
|
JSON.stringify(info.items),
|
||||||
|
);
|
||||||
|
await undo();
|
||||||
|
await undo();
|
||||||
|
info = await json('__gm.list.info()');
|
||||||
|
check(
|
||||||
|
'and two undos take delete and edit back',
|
||||||
|
JSON.stringify(info.items) === '["30","11","44"]',
|
||||||
|
info.items,
|
||||||
|
);
|
||||||
|
|
||||||
// 21c. A short list opened after a long one. Its rows leave the content height pinned to the
|
// 21c. 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.
|
// viewport, so a layout skipped on "same size" would stack them all at the top.
|
||||||
await mutate(`
|
await mutate(`
|
||||||
|
|||||||
Reference in new issue
Block a user