From 1282c9c0cbc5e7f2c29380f9a6f291cc6b7a5056 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 23 Sep 2026 21:59:49 +0200 Subject: [PATCH] gui: typing on the Presets screen no longer works the preset out twice per key Every change of a box settled the form twice - once in refreshLine for the line under the buttons and once in recheck to mark the box - and on the Presets screen each settle expanded the preset. With upload-validation chosen that is image encoding: about 300 ms of the window's thread and 117 MB of garbage for every key, while the seed was what was being typed. recheck now settles once and hands the reading to lineFrom, which says the line from it. refreshLine keeps its shape for the other callers, and its comment no longer says settling costs nothing. The Presets screen remembers the last expansion (lastExpansion), keyed by the preset and a copy of the values it is given. The seed and the output directory are not among them, so typing in them expands nothing. A refusal is kept too, since internal/preset reads no clock, randomness, environment or disk. Every settle is on the window's thread, so there is no lock. The parameters and the defaulted list go to the manifest as copies. Measured in the real window (tools/probes/guilag, main and this branch interleaved, three runs each, median per key): - seed, upload-validation: 303-335 ms / 117 MB -> 0-6 ms / 2 MB; - seed, tabular-import: 70-81 ms / 168 MB -> 0 ms / 1 MB; - the preset's own limit, upload-validation: 295-379 ms -> 149-160 ms. Two optional host interfaces let a guard count readings and expansions (countedSettle, tellExpanding). Nothing in the shipped program implements them. New guards in settleonce_test.go: one change of a box reads the form exactly once on all three screens, typing what a preset is not given does not expand it again, and a changed value or another preset is expanded again with the line following it. Not changed: menus and switches that rebuild the Several batches screen still read the form twice, and a key typed there with a preset switched on still expands the preset once. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 8 ++ internal/guard/settleonce_test.go | 174 ++++++++++++++++++++++++++++++ internal/guard/window_test.go | 11 ++ internal/gui/window/generate.go | 2 +- internal/gui/window/open.go | 32 ++++++ internal/gui/window/preset.go | 70 ++++++++++-- internal/gui/window/recipe.go | 2 +- internal/gui/window/run.go | 35 ++++-- internal/gui/window/runrefuse.go | 12 ++- 9 files changed, 327 insertions(+), 19 deletions(-) create mode 100644 internal/guard/settleonce_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 87aa8e4..1438131 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -494,6 +494,14 @@ because it turns other people's test suites red. again for each one. Both are now done once, and the window uses less memory while you type. +- **Typing on the `Presets` screen no longer lags.** With `upload-validation` + chosen, every key typed into a box held the window for about 0.3 seconds, + and about 0.07 seconds with `tabular-import`, because the preset was worked + out twice for each key. Typing the seed or the output folder now takes no + noticeable time, and changing a setting of the preset itself takes half as + long as before. On the `Several batches` screen with a preset switched on, + a key typed into a box now works the preset out once rather than twice. + - **A preview or a run refused while it was being planned no longer leaves "Working out what this would cost..." standing over the refusal.** diff --git a/internal/guard/settleonce_test.go b/internal/guard/settleonce_test.go new file mode 100644 index 0000000..c12a3d3 --- /dev/null +++ b/internal/guard/settleonce_test.go @@ -0,0 +1,174 @@ +package guard + +import ( + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/engine" + "github.com/donislawdev/TestingFilesGenerator/internal/format" + "github.com/donislawdev/TestingFilesGenerator/internal/gui/parts" + "github.com/donislawdev/TestingFilesGenerator/internal/gui/window" + "github.com/donislawdev/TestingFilesGenerator/internal/recipe" +) + +// One change of a box reads the form once, on every screen. +// +// Reading the form is settle, and on the preset screen settle expands the +// preset. Until 2026-09-23 every change read it twice - once for the line under +// the buttons and once to mark the box - and with upload-validation chosen that +// was 271-295 ms of the window's thread for every key typed, half of it working +// out an answer the line had just been given (docs/GUI-MEMORY-2026-09-23.md +// section 4h). +// +// Counted rather than timed, because a time says how fast this machine is and +// the count is the defect. Asked as exactly one rather than at most one: a +// screen whose readings stopped reaching the host would count nought and pass +// a ceiling, so nought is this guard not being in the state it asks about. +// +// An emptied box is asked about as well, because it takes the other way out of +// the live check - the line is said and the box is left alone - and that early +// return is where a second reading was sitting. +func TestOneChangeOfABoxReadsTheFormOnce(t *testing.T) { + host := newFakeHost(t) + gen, pre, rec := window.NewGenerate(host), window.NewPreset(host), window.NewRecipe(host) + chooserIn(t, pre.Fields(), settingPresetBox).SetSelected("size-boundaries") + + for _, b := range []struct { + screen string + fields *parts.Fields + at string + values []string + }{ + {"single batch", gen.Fields(), engine.SettingSeed, []string{"7", "", "42"}}, + {"single batch", gen.Fields(), format.SettingSize, []string{"3kb", "", "5kb"}}, + {"presets", pre.Fields(), engine.SettingSeed, []string{"7", "", "42"}}, + {"presets", pre.Fields(), "limit", []string{"20mb", "", "30mb"}}, + {"several batches", rec.Fields(), recipe.KeySeed, []string{"7", "", "42"}}, + {"several batches", rec.Fields(), recipe.TargetAddress(1, recipe.KeySize), []string{"3kb", "", "5kb"}}, + } { + for _, v := range b.values { + box := entryIn(t, b.fields, b.at) + if box.Text == v { + t.Fatalf("%q already holds %q on the %s screen, so typing it would change nothing and this guard would ask about no change", + b.at, v, b.screen) + } + host.settles = 0 + box.SetText(v) + switch { + case host.settles == 0: + t.Fatalf("typing %q into %q on the %s screen told the host of no reading of the form - "+ + "either the box reports no change or the screen reads its form without countedSettle, and this guard sees nothing either way", + v, b.at, b.screen) + case host.settles > 1: + t.Errorf("typing %q into %q on the %s screen read the form %d times, expected once - "+ + "on the preset screen every reading expands the preset", + v, b.at, b.screen, host.settles) + } + } + } +} + +// Typing into a box a preset is not given does not expand the preset again. +// +// The seed and the output directory are read with the preset's values but are +// not among them, so what the preset expands to cannot depend on either. The +// screen expanded it on every key anyway until 2026-09-23 - 157-180 ms and +// 60 MB each time for upload-validation, which encodes images to find its +// sizes. The screen remembers its last expansion now (lastExpansion in +// internal/gui/window/preset.go). +// +// upload-validation because it is the one that cost the most, though the +// count does not depend on which preset is chosen. +func TestTypingWhatAPresetIsNotGivenDoesNotExpandItAgain(t *testing.T) { + host := newFakeHost(t) + pre := window.NewPreset(host) + chooserIn(t, pre.Fields(), settingPresetBox).SetSelected("upload-validation") + if host.expansions == 0 { + t.Fatal("building the screen and choosing a preset expanded nothing, so either the preset screen no longer expands through lastExpansion or this guard is not in the state it asks about") + } + + host.settles, host.expansions = 0, 0 + seed := entryIn(t, pre.Fields(), engine.SettingSeed) + for _, v := range []string{"1", "12", "123"} { + seed.SetText(v) + } + dir := entryIn(t, pre.Fields(), engine.SettingOutDir) + dir.SetText(dir.Text + "-x") + + // The half that shows the keys were heard at all. Without it, a screen that + // stopped reading its form would expand nothing and pass. + if host.settles != 4 { + t.Fatalf("four keys read the form %d time(s), so this guard is not counting what it thinks it is", host.settles) + } + if host.expansions != 0 { + t.Errorf("typing into the seed and the output directory expanded the preset %d time(s), and neither is something a preset is given", + host.expansions) + } +} + +// A changed preset value, or another preset chosen, is expanded again. +// +// The other half of the one above, and the half without which a screen that +// remembers one expansion forever would pass it. Asked twice: through the +// count, and through what the line under the buttons says - an expansion +// worked out again and then not used is the same defect with the count right. +func TestAChangedPresetValueIsExpandedAgain(t *testing.T) { + host := newFakeHost(t) + pre := window.NewPreset(host) + chooserIn(t, pre.Fields(), settingPresetBox).SetSelected("size-boundaries") + before := statusLine(t, pre.Object()) + if before == "" { + t.Fatal("the line under the buttons says nothing with size-boundaries chosen, so there is nothing to compare a change against") + } + + host.expansions = 0 + entryIn(t, pre.Fields(), "limit").SetText("20mb") + if host.expansions != 1 { + t.Errorf("a new limit was typed and the preset was expanded %d time(s), expected once", host.expansions) + } + if after := statusLine(t, pre.Object()); after == before { + t.Errorf("a new limit was typed and the line still says what the old one came to:\n %q", after) + } + + // Another preset, with nothing given to either, so only the name tells the + // two apart. Asked of the screen rather than assumed: size-boundaries was + // the first choice here, and its format menu arrives with a value chosen, + // so the two differed by what they were given and a screen that ignored + // the name passed - measured by mutation on 2026-09-23. + fresh := newFakeHost(t) + other := window.NewPreset(fresh) + chooserIn(t, other.Fields(), settingPresetBox).SetSelected("empty-and-minimal") + nothingGiven(t, other.Fields(), "empty-and-minimal") + first := statusLine(t, other.Object()) + fresh.expansions = 0 + chooserIn(t, other.Fields(), settingPresetBox).SetSelected("text-encoding") + nothingGiven(t, other.Fields(), "text-encoding") + if fresh.expansions != 1 { + t.Errorf("another preset was chosen and it was expanded %d time(s), expected once", fresh.expansions) + } + if second := statusLine(t, other.Object()); second == first { + t.Errorf("another preset was chosen and the line still says what the first came to:\n %q", second) + } +} + +// nothingGiven stops a guard whose preset is given a value: every box and menu +// the preset screen draws for the chosen preset has to be empty. +func nothingGiven(t *testing.T, fields *parts.Fields, id string) { + t.Helper() + for _, f := range fields.All() { + switch f.Setting { + case settingPresetBox, engine.SettingSeed, engine.SettingOutDir: + continue + } + if e := firstEntryIn(f.Control); e != nil && e.Text != "" { + t.Fatalf("%s is given %s=%q, so it is not only the name that tells it from another preset", id, f.Setting, e.Text) + } + if c, is := f.Control.(*parts.Chooser); is && c.Selected != "" { + t.Fatalf("%s is given %s=%q, so it is not only the name that tells it from another preset", id, f.Setting, c.Selected) + } + } +} + +// settingPresetBox is the address the preset screen registers its menu of +// presets under - a key of that screen rather than of a recipe, see +// settingPreset in internal/gui/window/preset.go. +const settingPresetBox = "preset" diff --git a/internal/guard/window_test.go b/internal/guard/window_test.go index 96e50a5..e673b11 100644 --- a/internal/guard/window_test.go +++ b/internal/guard/window_test.go @@ -94,6 +94,10 @@ type fakeHost struct { // held is what the window asked to run later and has not run yet, when a // guard is holding the clock - see Later. Nil means the clock runs at once. held *heldClock + + // settles and expansions count what Settling and ExpandingPreset were told. + settles int + expansions int } // heldClock keeps what the window asked for later, so a guard can look at the @@ -193,6 +197,13 @@ func (h *fakeHost) holdTheClock() *heldClock { func (h *fakeHost) SetWaitForWork(fn func()) { h.waitForWork = fn } func (h *fakeHost) Close() { h.closed++ } +// Settling and ExpandingPreset are two more optional interfaces a real window +// does not implement: a screen reading its form, and the preset screen +// expanding a preset rather than taking what it remembers. Counted for +// settleonce_test.go, and every other guard runs through them unaware. +func (h *fakeHost) Settling() { h.settles++ } +func (h *fakeHost) ExpandingPreset() { h.expansions++ } + // HoldDuringRun is the other optional interface, and the other direction: the // window asks the host for this one rather than handing it over. // diff --git a/internal/gui/window/generate.go b/internal/gui/window/generate.go index f0d14b5..2d22ce2 100644 --- a/internal/gui/window/generate.go +++ b/internal/gui/window/generate.go @@ -202,7 +202,7 @@ type Generate struct { // NewGenerate builds the screen. links are the buttons to the other screens. func NewGenerate(host Host, links ...fyne.CanvasObject) *Generate { g := &Generate{runner: newRunner(host.Later), host: host, tips: parts.NewTips(), settingsFolded: true} - g.runner.settle = g.settle + g.runner.settle = countedSettle(host, g.settle) g.runner.offer.through(host) // This screen is one target and draws its boxes under the bare key, so a // refusal that arrives carrying a position belongs to the box of that name. diff --git a/internal/gui/window/open.go b/internal/gui/window/open.go index 66a3224..b6d0ec0 100644 --- a/internal/gui/window/open.go +++ b/internal/gui/window/open.go @@ -7,6 +7,7 @@ import ( "fyne.io/fyne/v2" "fyne.io/fyne/v2/container" "fyne.io/fyne/v2/driver/desktop" + "github.com/donislawdev/TestingFilesGenerator/internal/engine" "github.com/donislawdev/TestingFilesGenerator/internal/gui/parts" "github.com/donislawdev/TestingFilesGenerator/internal/gui/text" ) @@ -448,3 +449,34 @@ func offerHolding(h Host, screens []interface{ HoldBeforeFinishing(func()) }) { screen.HoldBeforeFinishing(hold) } } + +// countedSettle is a screen's reading of its form, told to a host that counts +// the readings and handed back untouched to any other. +// +// The same shape as the two above - an optional interface, checked rather than +// required, and nothing in the shipped program implements it. It is here for a +// number a guard cannot read off the screen: how many times one change of a +// box read the form. It was two until 2026-09-23, once for the line and once +// for the box, and on the preset screen every reading expanded the preset - +// 271-295 ms of the window's thread per key for upload-validation, measured +// in docs/GUI-MEMORY-2026-09-23.md section 4h. +func countedSettle(h Host, settle settler) settler { + c, ok := h.(interface{ Settling() }) + if !ok { + return settle + } + return func() ([]engine.Target, engine.Options, error) { + c.Settling() + return settle() + } +} + +// tellExpanding says to a host that counts them that a preset is being +// expanded rather than taken from what the screen remembers - see +// lastExpansion. The same kind of seam as countedSettle, for the other half +// of the same question. +func tellExpanding(h Host) { + if c, ok := h.(interface{ ExpandingPreset() }); ok { + c.ExpandingPreset() + } +} diff --git a/internal/gui/window/preset.go b/internal/gui/window/preset.go index 035b656..d1395f2 100644 --- a/internal/gui/window/preset.go +++ b/internal/gui/window/preset.go @@ -2,6 +2,8 @@ package window import ( "errors" + "maps" + "slices" "fyne.io/fyne/v2" "fyne.io/fyne/v2/container" @@ -47,6 +49,10 @@ type Preset struct { // fixed is how many fields this screen has before a preset declares any. fixed int + // last is the preset expanded last and what it came to, so that typing in + // a box the preset is not given does not expand it again. + last lastExpansion + body fyne.CanvasObject } @@ -54,7 +60,7 @@ type Preset struct { func NewPreset(host Host, links ...fyne.CanvasObject) *Preset { p := &Preset{runner: newRunner(host.Later), host: host, tips: parts.NewTips()} p.runner.offer.through(host) - p.runner.settle = p.settle + p.runner.settle = countedSettle(host, p.settle) // No readdress here, and that is the boundary of this screen rather than an // omission. The other two screens draw boxes for the settings of a target, // so a refusal carrying a position has a box to be moved onto. This one @@ -252,7 +258,8 @@ func (p *Preset) given() preset.Args { // Source rather than a structure, and the same parser a handwritten file goes // through - PR5. So what this screen runs is what "tfg preset eject" prints and // what "tfg generate --preset" consumes, down to the bytes, because there is -// only one expansion and all three call it. +// only one expansion and all three call it. Called once per set of values +// rather than once per reading of the form - see lastExpansion. func (p *Preset) settle() ([]engine.Target, engine.Options, error) { var none engine.Options @@ -278,7 +285,7 @@ func (p *Preset) settle() ([]engine.Target, engine.Options, error) { bad = append(bad, err) } - expanded, err := preset.Expand(p.pick.Selected, p.given()) + expanded, notes, err := p.last.of(p.host, p.pick.Selected, p.given()) if err != nil { bad = append(bad, err) } @@ -296,7 +303,7 @@ func (p *Preset) settle() ([]engine.Target, engine.Options, error) { } // What the run has to say out loud about a value nobody gave it. - p.notes = expanded.Notes() + p.notes = notes targets := make([]engine.Target, 0, len(rec.Targets)) for _, t := range rec.Targets { @@ -313,14 +320,65 @@ func (p *Preset) settle() ([]engine.Target, engine.Options, error) { Command: "tfg-gui", ManifestName: engine.DefaultManifestName, RecipeHash: hash, + // Copies, because the expansion stays behind for the next reading of + // the form while these go to a worker and into a manifest. Nothing + // writes to either today - checked 2026-09-23 - and no guard would + // notice the day something did, so the copy is the whole defence. Preset: &manifest.Preset{ ID: expanded.Preset.ID, - Parameters: map[string]string(expanded.Settled), - Defaulted: expanded.Defaulted, + Parameters: maps.Clone(map[string]string(expanded.Settled)), + Defaulted: slices.Clone(expanded.Defaulted), }, }, nil } +// lastExpansion is the preset this screen expanded last, what it was given and +// what that came to. +// +// The form is read on every key, and reading it on this screen expanded the +// preset every time: 157-180 ms and 60 MB for upload-validation, which encodes +// images to find its sizes, and 48-50 ms and 87 MB for tabular-import - while +// the seed or the output directory was what was being typed, and neither is +// something a preset is given (measured 2026-09-23, +// docs/GUI-MEMORY-2026-09-23.md section 4h). +// +// Keyed by what Expand reads and nothing else: the preset and the values given +// to it. The package holding it reads no clock, no randomness, no environment +// and no disk (checked 2026-09-23), so the same preset given the same values +// expands to the same bytes - the property eject and D11 already rest on. A +// refusal is kept as well as an expansion, for the same reason. +// +// One entry, because what is typed changes one thing at a time, and a second +// preset chosen is a new expansion whichever way it is remembered. No lock, +// because every reading of the form happens on the window's thread - the +// line, the live check and both presses, which read the form before a worker +// is handed the result. +type lastExpansion struct { + held bool + id string + given preset.Args + got *preset.Expansion + notes []string + err error +} + +// of is the expansion of a preset given these values, worked out only when it +// is not the one worked out last. The notes come with it, because working +// them out reads the same values. +func (l *lastExpansion) of(h Host, id string, given preset.Args) (*preset.Expansion, []string, error) { + if l.held && l.id == id && maps.Equal(l.given, given) { + return l.got, l.notes, l.err + } + tellExpanding(h) + got, err := preset.Expand(id, given) + var notes []string + if err == nil { + notes = got.Notes() + } + *l = lastExpansion{held: true, id: id, given: maps.Clone(given), got: got, notes: notes, err: err} + return got, notes, err +} + // engineTarget turns one recipe target into one engine target. // // The command line has the same conversion and the two cannot be shared - they diff --git a/internal/gui/window/recipe.go b/internal/gui/window/recipe.go index 22c409e..4d4c013 100644 --- a/internal/gui/window/recipe.go +++ b/internal/gui/window/recipe.go @@ -162,7 +162,7 @@ type content struct { // also what the single batch screen shows, so the two read as one tool. func NewRecipe(host Host, links ...fyne.CanvasObject) *Recipe { r := &Recipe{runner: newRunner(host.Later), host: host, tips: parts.NewTips()} - r.runner.settle = r.settle + r.runner.settle = countedSettle(host, r.settle) r.runner.offer.through(host) // A refusal about a size belongs on the box the switch is showing. r.runner.readdress = r.readdressSizeWay diff --git a/internal/gui/window/run.go b/internal/gui/window/run.go index 136431c..8a5e24a 100644 --- a/internal/gui/window/run.go +++ b/internal/gui/window/run.go @@ -291,11 +291,16 @@ func (r *runner) toneOfOutcome(res *engine.Result, runErr error) { // refreshLine works out what the form comes to and puts it on the line. // -// From settle rather than from a plan: settle is what the form parses to and -// costs nothing, and planning is what the engine does with it and can cost -// seconds (see onPreview). A form that does not settle falls back to naming -// the destination alone, which is read off its own box because it is the one -// fact worth having whatever the other boxes say. +// From settle rather than from a plan: settle is what the form parses to, and +// planning is what the engine does with it and can cost seconds (see +// onPreview). +// +// Settling is not free either, and this comment said it was until 2026-09-23. +// On the preset screen it expands the preset, which for upload-validation +// encodes images - 157-180 ms and 60 MB each time, measured that day +// (docs/GUI-MEMORY-2026-09-23.md section 4h). That is why a change of a box +// reads the form once for the line and for the box (recheck), and why the +// preset screen remembers what it expanded last (lastExpansion). // // Not while a run owns the screen: its progress is not to be overwritten by // a summary, and the form is frozen then anyway. @@ -303,12 +308,24 @@ func (r *runner) refreshLine() { if r.settle == nil || r.busy.occupied { return } - dir := "" - if r.destination != nil { - dir = r.destination() - } targets, opt, err := r.settle() + lineFrom(r, targets, opt, err) +} + +// lineFrom puts on the line what one reading of the form came to. A form that +// does not settle falls back to naming the destination alone, which is read +// off its own box because it is the one fact worth having whatever the other +// boxes say. +// +// Apart from refreshLine so that recheck can hand it the reading it has +// already made. A function rather than a method, because the runner stands +// one method under its ceiling. +func lineFrom(r *runner, targets []engine.Target, opt engine.Options, err error) { if err != nil { + dir := "" + if r.destination != nil { + dir = r.destination() + } showOn(r.status, r.line.fallback(dir)) return } diff --git a/internal/gui/window/runrefuse.go b/internal/gui/window/runrefuse.go index b146a91..021729d 100644 --- a/internal/gui/window/runrefuse.go +++ b/internal/gui/window/runrefuse.go @@ -215,11 +215,20 @@ func (r *runner) recheck(setting string) { if r.busy.occupied { return } + // The form is read ONCE, for the line and for the box below. It was read + // twice until 2026-09-23 - through refreshLine here and again after the + // early return - and on the preset screen each reading expanded the + // preset: 271-295 ms of the window's thread for every key typed with + // upload-validation chosen, half of it spent working out an answer the + // line had just been given (docs/GUI-MEMORY-2026-09-23.md section 4h). + // Nothing settle reads is changed between the two places it was called, + // so the one reading is the answer both of them got. + targets, opt, err := r.settle() // Whatever changed, the line says what the form comes to now - over an // outcome or a preview, which described a form that no longer exists. // Before the early return below, because a box emptied to be retyped // changes the count as surely as a box filled in. - r.refreshLine() + lineFrom(r, targets, opt, err) // Only this box, in both directions. What the other boxes were told is // about values nobody has just changed, and it is still true - including // the parts of it this cannot see, because a format minimum and a name @@ -228,7 +237,6 @@ func (r *runner) recheck(setting string) { if r.fields.Blank(setting) { return } - _, _, err := r.settle() for _, one := range spread(err) { var about interface{ AboutSetting() string } if errors.As(one, &about) && r.placeOf(about.AboutSetting()) == setting {