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 {