diff --git a/CHANGELOG.md b/CHANGELOG.md index 9166021..d08446a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -475,6 +475,15 @@ because it turns other people's test suites red. ### Fixed +- **The `Several batches` screen no longer slows down the longer it is + used.** Every batch added, removed or copied, every format chosen and every + press of `Start from a preset` made each box on the screen report a change + one more time, so the switch took 0.7 seconds at its first press and 6 + seconds at its twentieth. With a preset switched on, every key typed took + about 0.4 seconds, because the smallest file of every format was worked out + again for each one. Both are now done once, and the window uses less memory + while you type. + - **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/format/registry.go b/internal/format/registry.go index 237ac51..bac6893 100644 --- a/internal/format/registry.go +++ b/internal/format/registry.go @@ -38,6 +38,42 @@ func Register(d Descriptor) { registry[d.ID] = d } +// SmallestWithLabel is d.SmallestAccepted(Request{Label: true}), worked out +// once per format per process and remembered. +// +// The request is the same every time, so the answer is too - and finding it +// means planning the format at growing sizes, which for a picture means +// encoding one. The minimal preset asks it of every format at every +// expansion, and the window expands that preset on every change while the +// batch screen builds on it: measured on 2026-09-23, 50.4 MB allocated per +// expansion afresh against 0.51 MB remembered, and a keystroke that cost +// 380 ms in the real window (docs/GUI-MEMORY-2026-09-23.md section 2.3). +// +// Keyed by id, which is safe because Register refuses a second descriptor +// under one. Here rather than beside its caller because this file is already +// where the registry's reads meet its writes: the window settles from its +// worker as well as from its own goroutine. The size is worked out without +// the lock held, because planning an archive reads the registry itself. +func SmallestWithLabel(d Descriptor) int64 { + smallestMu.Lock() + known, ok := smallestKnown[d.ID] + smallestMu.Unlock() + if ok { + return known + } + size := d.SmallestAccepted(Request{Label: true}) + smallestMu.Lock() + smallestKnown[d.ID] = size + smallestMu.Unlock() + return size +} + +// smallestKnown is what SmallestWithLabel has worked out, by format id. +var ( + smallestMu sync.Mutex + smallestKnown = map[string]int64{} +) + // SortChoices puts a closed set in the order somebody looks for a value in. // // Here rather than in the menu that draws them, and that is the whole point: diff --git a/internal/guard/branching_test.go b/internal/guard/branching_test.go index 9a4588a..5f38ee0 100644 --- a/internal/guard/branching_test.go +++ b/internal/guard/branching_test.go @@ -57,7 +57,11 @@ const ( // every owner, and the loop left behind is two deep rather than three. The // four presets that arrived the same day were flattened to hold the number // where it was - this is the one that went below it. - crowdedDepthFunctions = 50 + // Lowered from 50 on 2026-09-23: Fields.listen wrapped each of three kinds + // of control in its own closure and went one deeper when it learnt to wrap + // only once - the three became one function, chainOnce, and listen came + // out of the band. + crowdedDepthFunctions = 49 // An axis this set does not watch. crowding() asks n >= band, so nothing // reaches it. diff --git a/internal/guard/livecheck_test.go b/internal/guard/livecheck_test.go index 30278f4..a56aef2 100644 --- a/internal/guard/livecheck_test.go +++ b/internal/guard/livecheck_test.go @@ -1,14 +1,19 @@ package guard import ( + "fmt" + "reflect" "strings" "testing" "fyne.io/fyne/v2" + "fyne.io/fyne/v2/test" "fyne.io/fyne/v2/widget" "github.com/donislawdev/TestingFilesGenerator/internal/gui/parts" "github.com/donislawdev/TestingFilesGenerator/internal/gui/text" + "github.com/donislawdev/TestingFilesGenerator/internal/gui/window" + "github.com/donislawdev/TestingFilesGenerator/internal/recipe" ) // Every box that is wrong is marked, not the first one. @@ -253,6 +258,152 @@ func TestTypingIsStillCheckedAfterARunHasFinished(t *testing.T) { // has to know. Its own documentation promises the call works "before or after // the fields exist". Found by an outside review of the whole tree on // 2026-08-23, docs/CODE-REVIEW-2026-08-23.md section 2. +// A control registered again reports a change once, under the address it was +// registered at last, and counts into the caption drawn with it last. +// +// The batch screen registers every field again on every rebuild, because an +// address carries the batch's position - and it keeps the controls, so what +// was typed survives. listen and counter used to wrap the control's callback +// on every registration, so after k rebuilds one change was reported k times, +// under every address the control had ever had, and k captions nobody could +// see were counted into. Measured in the real window on 2026-09-23: the +// preset switch took 0.7 s at the first press and 6.1 s at the twentieth, +// docs/GUI-MEMORY-2026-09-23.md section 2.2. +func TestAControlRegisteredAgainReportsOnceUnderItsLatestAddress(t *testing.T) { + test.NewApp() + t.Cleanup(func() { test.NewApp() }) + + box := parts.NewEntry() + menu := parts.NewChooser([]string{"one", "two"}, nil) + toggle := parts.NewToggle(nil) + for _, c := range []struct { + name string + control fyne.CanvasObject + change func() + }{ + {"a box", box, func() { box.SetText("1kb") }}, + {"a menu", menu, func() { menu.SetSelected("two") }}, + {"a switch", toggle, func() { toggle.SetChecked(true) }}, + } { + t.Run(c.name, func(t *testing.T) { + fields := parts.NewFields() + var told []string + fields.WhenTypedIn(func(setting string) { told = append(told, setting) }) + var drawn []fyne.CanvasObject + for i := 1; i <= 5; i++ { + address := fmt.Sprintf("targets[%d].size", i) + fields.KeepFirst(0) + fields.InBytes(address) + drawn = append(drawn, fields.Add(address, "Size", "", parts.Detail{}, c.control)) + } + c.change() + if want := []string{"targets[5].size"}; !reflect.DeepEqual(told, want) { + t.Errorf("one change after five registrations told the screen %q, expected %q", told, want) + } + if c.control != fyne.CanvasObject(box) { + return + } + last, first := byteCountIn(drawn[len(drawn)-1]), byteCountIn(drawn[0]) + if last == nil || first == nil { + t.Fatal("a size box was registered and no count of bytes was drawn with it, so this guard is not in the state it asks about") + } + if last.Text == "" { + t.Error("the caption drawn with the box last says nothing about the size typed into it") + } + if first.Text != "" { + t.Errorf("the caption from the first registration, which is no longer on any screen, was counted into: %q", first.Text) + } + }) + } +} + +// No control stands under two addresses at once, on any screen. +// +// The fix above rests on it. A control registered again reports under the +// address it was registered at LAST, which is right when the second +// registration replaces the first - a rebuild - and wrong if one control were +// ever registered under two addresses in the same pass, because the first +// would then stop hearing about it. Asked of the registry of every work +// screen, the batch screen in the fullest state it has: three batches, the +// files inside an archive, and a preset with its parameters. +func TestNoControlIsRegisteredUnderTwoAddressesAtOnce(t *testing.T) { + host := newFakeHost(t) + gen, pre, rec := window.NewGenerate(host), window.NewPreset(host), window.NewRecipe(host) + + body := rec.Object() + for i := 0; i < 2; i++ { + add := buttonNamed(body, text.ButtonAddBatch()) + if add == nil { + t.Fatal("the batch screen has no button to add a batch, so this guard cannot reach three") + } + add.OnTapped() + } + chooserIn(t, rec.Fields(), recipe.TargetAddress(1, recipe.KeyFormat)).SetSelected("zip") + contents := buttonNamed(body, text.ButtonAddContents()) + if contents == nil { + t.Fatal("a zip batch offers no way to say what it holds, so this guard cannot reach the table of contents") + } + contents.OnTapped() + for _, f := range rec.Fields().All() { + if toggle, is := f.Control.(*parts.Toggle); is && !toggle.Checked { + toggle.SetChecked(true) + break + } + } + + for _, s := range []struct { + name string + fields *parts.Fields + must []string + }{ + {"single batch", gen.Fields(), []string{"size"}}, + {"presets", pre.Fields(), nil}, + {"several batches", rec.Fields(), []string{ + recipe.TargetAddress(3, recipe.KeyID), + recipe.ContentAddress(1, 1, recipe.KeySize), + recipe.KeyExtends, + }}, + } { + t.Run(s.name, func(t *testing.T) { + under := map[fyne.CanvasObject]string{} + settings := map[string]bool{} + for _, f := range s.fields.All() { + settings[f.Setting] = true + for _, c := range reportingControls(f.Control) { + if was, seen := under[c]; seen && was != f.Setting { + t.Errorf("one %T stands under %q and under %q, and a change would be told under the second alone", c, was, f.Setting) + } + under[c] = f.Setting + } + } + for _, want := range s.must { + if !settings[want] { + t.Fatalf("the registry has no %q, so this guard is not in the state it asks about", want) + } + } + if len(under) == 0 { + t.Fatal("no control on this screen reports a change, so nothing was asked") + } + }) + } +} + +// reportingControls are the controls under one field that tell the screen +// about a change - the three kinds Fields.listen wires. +func reportingControls(o fyne.CanvasObject) []fyne.CanvasObject { + switch it := o.(type) { + case *parts.Entry, *parts.Chooser, *parts.Toggle: + return []fyne.CanvasObject{it} + case *fyne.Container: + var out []fyne.CanvasObject + for _, child := range it.Objects { + out = append(out, reportingControls(child)...) + } + return out + } + return nil +} + func TestAFieldWiredBeforeTheScreenListensReportsOnce(t *testing.T) { for _, order := range []string{"the field first", "the listener first"} { t.Run(order, func(t *testing.T) { diff --git a/internal/guard/minimalset_test.go b/internal/guard/minimalset_test.go index 832bdfb..098f9d3 100644 --- a/internal/guard/minimalset_test.go +++ b/internal/guard/minimalset_test.go @@ -2,6 +2,7 @@ package guard import ( "bytes" + "runtime" "strings" "testing" @@ -138,6 +139,43 @@ func TestTheMinimalSetSitsOnEveryFormatsFloor(t *testing.T) { // The bytes of the files never moved, because a seed comes from the id of a // target rather than from its place in the list. That is what made this quiet: // every file was right and only the record of them disagreed. +// The smallest size of each format is worked out once, not at every expansion. +// +// Finding it means planning the format at growing sizes, and for a picture +// that means encoding one. The window expands this set on every change while +// the batch screen builds on it, so a set worked out afresh each time made a +// keystroke there cost 380 ms and ~379 MB of garbage in the real window on +// 2026-09-23 (docs/GUI-MEMORY-2026-09-23.md section 2.3). Measured here the +// same day, least of five: 50.4 MB per expansion afresh, 0.51 MB remembered. +// The line sits a factor of ten from each. +// +// The least of several readings, because the counter is the whole process's +// and a reading can only be too high - the lesson of tools/probes/alloccount. +// That the remembered sizes are the RIGHT ones is +// TestTheMinimalSetSitsOnEveryFormatsFloor's question, not this one's. +func TestTheMinimalSetIsWorkedOutOnceAndNotAtEveryExpansion(t *testing.T) { + const ceiling = 5 << 20 + if _, err := preset.Expand("empty-and-minimal", preset.Args{}); err != nil { + t.Fatalf("the set did not expand, so nothing was asked: %v", err) + } + least := ^uint64(0) + for i := 0; i < 5; i++ { + var before, after runtime.MemStats + runtime.ReadMemStats(&before) + if _, err := preset.Expand("empty-and-minimal", preset.Args{}); err != nil { + t.Fatal(err) + } + runtime.ReadMemStats(&after) + if spent := after.TotalAlloc - before.TotalAlloc; spent < least { + least = spent + } + } + if least > ceiling { + t.Errorf("expanding the minimal set a second time allocated %d bytes, over %d - "+ + "the smallest size of every format is being worked out again", least, ceiling) + } +} + func TestTheMinimalSetIsTheSameWhateverOrderTheFormatsAreNamedIn(t *testing.T) { // Two formats far apart in the registry, so a walk that kept the typing // cannot pass by accident. diff --git a/internal/gui/parts/entry.go b/internal/gui/parts/entry.go index f5c1d66..997ae41 100644 --- a/internal/gui/parts/entry.go +++ b/internal/gui/parts/entry.go @@ -34,6 +34,13 @@ type Entry struct { // the screen, because the screen is what knows the canvas. onOurs func(fyne.Shortcut) + // reports and counts are the two things a field does when this box + // changes - tell the screen under which address, and count the bytes into + // which caption. Wired once and re-pointed on every registration, see + // wiredOnce in fields.go. + reports wiredOnce[string] + counts wiredOnce[*ByteCount] + // ring is the edge a field draws round this box when the keyboard is in it // or a run refused it, so a box carries the same 2 px mark as the menu and // the switch beside it rather than only the toolkit's own 1 px border - diff --git a/internal/gui/parts/fields.go b/internal/gui/parts/fields.go index 4882abf..d944505 100644 --- a/internal/gui/parts/fields.go +++ b/internal/gui/parts/fields.go @@ -217,13 +217,9 @@ func (s *Fields) counter(setting string, control fyne.CanvasObject) fyne.CanvasO count := newByteCount() for _, b := range boxesIn(control) { b := b - already := b.OnChanged - b.OnChanged = func(value string) { - if already != nil { - already(value) - } - count.show(value) - } + // Into the caption drawn now, whichever registration this is - see + // wiredOnce for why the box is wrapped only the first time. + chainOnce(&b.OnChanged, &b.counts, count, func(value string, into *ByteCount) { into.show(value) }) // And once now, for a box that arrives with a size already in it. count.show(b.Text) } @@ -353,6 +349,12 @@ func (s *Fields) WhenTypedIn(tell func(setting string)) { // reacting. The chain runs the control's own work FIRST, so what is reported // is read off a screen that has already changed. // +// Chained ONCE per control, and the address read at the moment of the change. +// The batch screen registers every field again on every rebuild and keeps the +// controls, and until 2026-09-23 each registration added a link: after k +// rebuilds one change was reported k times, under every address the control +// had ever had. See wiredOnce. +// // Menus and switches since 2026-09-14. Only boxes reported until then, which // was enough while the only listener was the live check and the only thing a // menu could be wrong about was nothing. It stopped being enough when the @@ -360,42 +362,71 @@ func (s *Fields) WhenTypedIn(tell func(setting string)) { // the menu changed the run and the line went on naming the old one. func (s *Fields) listen(setting string, control fyne.CanvasObject) { // Read at the moment somebody types rather than at the moment this is - // wired, so a listener asked for after the field exists still hears it. - report := func() { + // wired, so a listener asked for after the field exists still hears it - + // and so does the address, so a control registered again reports under + // the address it has now. + report := func(_ string, at string) { if s.tell != nil { - s.tell(setting) + s.tell(at) } } walkControls(control, func(o fyne.CanvasObject) { switch it := o.(type) { case *Entry: - already := it.OnChanged - it.OnChanged = func(value string) { - if already != nil { - already(value) - } - report() - } + chainOnce(&it.OnChanged, &it.reports, setting, report) case *Chooser: - already := it.OnChanged - it.OnChanged = func(value string) { - if already != nil { - already(value) - } - report() - } + chainOnce(&it.OnChanged, &it.reports, setting, report) case *Toggle: - already := it.OnChanged - it.OnChanged = func(on bool) { - if already != nil { - already(on) - } - report() - } + chainOnce(&it.OnChanged, &it.reports, setting, func(_ bool, at string) { report("", at) }) } }) } +// wiredOnce is one thing a field does when its control changes, put into the +// control ONCE and pointed at a new target on every registration. +// +// Kept on the control rather than in a map on Fields, because the control is +// what outlives a rebuild: the batch screen builds its panels again and keeps +// the boxes, so what was typed survives. A map here would also keep the boxes +// of a batch that was removed, for as long as the screen lives. +// +// Measured before it existed, in the real window on 2026-09-23 (tools/probes/ +// guilag, docs/GUI-MEMORY-2026-09-23.md section 2.2): every registration +// wrapped the callback again, so the preset switch on the batch screen took +// 0.7 s at its first press and 6.1 s at its twentieth, and a size box counted +// into every caption it had ever been drawn with. +type wiredOnce[T any] struct { + wired bool + target T +} + +// point says where the handler reports from now on, and answers true the +// first time only - when the handler has still to be put in. +func (w *wiredOnce[T]) point(target T) (first bool) { + w.target = target + first = !w.wired + w.wired = true + return first +} + +// chainOnce points one of a control's change handlers at target, and the +// first time only puts it in: the control's own callback runs first, then +// then, with the value and whatever target is pointed at by the time the +// change happens. One function for the three kinds of control and the count +// of bytes, so the rule cannot hold for some of them and not the others. +func chainOnce[V, T any](handler *func(V), to *wiredOnce[T], target T, then func(value V, at T)) { + if !to.point(target) { + return + } + already := *handler + *handler = func(value V) { + if already != nil { + already(value) + } + then(value, to.target) + } +} + // walkControls visits a control and everything inside it, containers included. func walkControls(o fyne.CanvasObject, visit func(fyne.CanvasObject)) { visit(o) diff --git a/internal/gui/parts/ring.go b/internal/gui/parts/ring.go index e64461d..cb6c399 100644 --- a/internal/gui/parts/ring.go +++ b/internal/gui/parts/ring.go @@ -198,6 +198,9 @@ type Chooser struct { // in it. Neither alone is worth anything - a list built correctly and never // shown looks right from the widget's side. opened *OpenList + // reports is the address a choice is told to the screen under - see + // wiredOnce in fields.go. + reports wiredOnce[string] } // Opened is the list this menu last dropped down, or nil if it never has. diff --git a/internal/gui/parts/toggle.go b/internal/gui/parts/toggle.go index 1a0f81a..acb8f15 100644 --- a/internal/gui/parts/toggle.go +++ b/internal/gui/parts/toggle.go @@ -45,6 +45,9 @@ type Toggle struct { // from knows whether the keyboard arrived by press or by key, so that a // press can put the keyboard here without drawing the mark that says so. from PointerFocus + // reports is the address a flip is told to the screen under - see + // wiredOnce in fields.go. + reports wiredOnce[string] } var ( diff --git a/internal/preset/emptyandminimal.go b/internal/preset/emptyandminimal.go index d00a9ce..85a72ec 100644 --- a/internal/preset/emptyandminimal.go +++ b/internal/preset/emptyandminimal.go @@ -255,8 +255,11 @@ func layOut(descs []format.Descriptor) []minimalFile { // MinBytes beside it is the structural floor with no label, and it is NOT the // same number - docx, pdf, targz, wav and zip all differ, measured 2026-09-22. // Asking for MinBytes is refused. +// +// Remembered per format for the life of the process - see +// format.SmallestWithLabel for the measurement behind it. func smallest(d format.Descriptor) int64 { - return d.SmallestAccepted(format.Request{Label: true}) + return format.SmallestWithLabel(d) } // saidAboutTheMinimalSet says when the set came out with only one of its halves.