From f86b1dbb0328ccfd70d0029d55b5144beb6d91fa Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 3 Nov 2026 20:07:29 +0100 Subject: [PATCH 1/3] gui: a change of damage takes the old parameters off the registry, and what is cut off a list is let go Choosing a damage added its parameters to the generate screen's registry of fields and never took the previous ones away, so every choice left a field behind under nothing on the screen. The parameters now start at a mark set where the format's fields are finished, and a change of damage cuts back to it. KeepFirst clears what it cuts off, and the recipe screen removes batches and contents with slices.Delete, so a cut list no longer keeps the cut pointers alive in its array. The list of batches moved into recipelist.go, because the import took recipe.go past its ceiling. Three guards: the registry against the screen in both directions after twenty choices of damage and twenty changes of format, weak pointers on what KeepFirst cuts off, and no append of a slice's own two halves anywhere in internal/ or cmd/. Six mutation entries, eight repointed. Co-Authored-By: Claude Opus 5.5 --- internal/guard/codeshape_test.go | 3 + internal/guard/damagewindow_test.go | 121 ++++++++++++++++++++++++++++ internal/guard/letgo_test.go | 102 +++++++++++++++++++++++ internal/guard/mutation/entries.py | 56 +++++++++++-- internal/gui/parts/fields.go | 5 ++ internal/gui/window/generate.go | 16 +++- internal/gui/window/recipe.go | 77 ------------------ internal/gui/window/recipelist.go | 93 +++++++++++++++++++++ 8 files changed, 388 insertions(+), 85 deletions(-) create mode 100644 internal/guard/letgo_test.go create mode 100644 internal/gui/window/recipelist.go diff --git a/internal/guard/codeshape_test.go b/internal/guard/codeshape_test.go index 8b5721c7..36d4b1db 100644 --- a/internal/guard/codeshape_test.go +++ b/internal/guard/codeshape_test.go @@ -65,6 +65,9 @@ const ( // Lowered from 399 on 2026-10-07: what makes a container moved out of // format/format.go into container.go when two fields for eml took it // past the ceiling. The longest file is gui/window/recipe.go now. + // Held at 395 on 2026-11-03: the list of batches moved out of + // gui/window/recipe.go into recipelist.go when one import took it past + // the ceiling. The longest file is oracle/oracle.go now. longestFile = 395 // Depth answers a different question than length, and it is the better diff --git a/internal/guard/damagewindow_test.go b/internal/guard/damagewindow_test.go index 8cd9653e..ba7e2d30 100644 --- a/internal/guard/damagewindow_test.go +++ b/internal/guard/damagewindow_test.go @@ -8,8 +8,11 @@ import ( "strings" "testing" + "fyne.io/fyne/v2" + "github.com/donislawdev/TestingFilesGenerator/internal/damage" "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/text" "github.com/donislawdev/TestingFilesGenerator/internal/gui/window" @@ -87,6 +90,124 @@ func TestTheWindowDrawsAFieldForEveryDamageParameter(t *testing.T) { t.Logf("%d damage parameter(s) drawn from the registry", checked) } +// Choosing a damage again, or a format, leaves the registry of fields holding +// what the screen draws and nothing else. +// +// The registry is what a refusal is addressed against and what a run freezes, +// so a field the screen no longer draws is a place a refusal could still go, +// under a box nobody can see. Every choice of a damage used to leave one +// behind - its parameters were added and never taken away - so the count is +// read after many choices rather than one: one stale field reads like the cost +// of the screen, twenty read like what they are (R18 of the stability review). +// Then the same with the format changed in between, because the parameters sit +// after the format's fields, and a mark that did not move with the format +// would cut the format's fields instead of the damage's. +func TestChoosingADamageAgainLeavesNoFieldBehind(t *testing.T) { + host := newFakeHost(t) + gen := window.NewGenerate(host) + content := gen.Object() + t.Cleanup(func() { join(host) }) + fields := gen.Fields() + picker, ok := controlUnder(content, text.FieldDamage()).(*parts.Chooser) + if !ok { + t.Fatal("the damage field is not a list to choose from") + } + var chosen damage.Descriptor + for _, d := range damage.All() { + if len(d.Parameters) > 0 { + chosen = d + break + } + } + if chosen.ID == "" { + t.Fatal("no damage declares a parameter, so this proved nothing") + } + + // The formats that declare the fewest and the most settings, asked of the + // registry: two formats with as many fields each would pass a mark that + // stayed where the first one left it. + formats := chooserIn(t, fields, "format") + declared := map[string]int{} + var few, many string + for _, id := range formats.Options { + d, err := format.Get(id) + if err != nil { + t.Fatalf("the format menu offers %q: %v", id, err) + } + declared[id] = len(d.Properties) + if few == "" || declared[id] < declared[few] { + few = id + } + if many == "" || declared[id] > declared[many] { + many = id + } + } + if declared[few] == declared[many] { + t.Fatalf("every format declares %d settings, so a mark that stayed with the first would pass", declared[few]) + } + + counts := map[string]int{} + for _, f := range []string{few, many} { + // None before the format rather than after it: a change of format + // takes every parameter away, so this is a count no stale field can + // be part of, whatever the screen does with a choice of none. + picker.SetSelected(text.DamageNone()) + formats.SetSelected(f) + without := fields.Len() + picker.SetSelected(chosen.ID) + counts[f] = fields.Len() + if counts[f] <= without { + t.Fatalf("choosing %s with %s added no field (%d before and after), so this proved nothing", chosen.ID, f, without) + } + } + for i := 0; i < 20; i++ { + picker.SetSelected(text.DamageNone()) + picker.SetSelected(chosen.ID) + } + if got := fields.Len(); got != counts[many] { + t.Errorf("after twenty more choices of %s the registry holds %d fields, and %d after the first", chosen.ID, got, counts[many]) + } + for i := 0; i < 20; i++ { + formats.SetSelected([]string{few, many}[i%2]) + picker.SetSelected(text.DamageNone()) + picker.SetSelected(chosen.ID) + } + if got := fields.Len(); got != counts[many] { + t.Errorf("after twenty changes of format the registry holds %d fields, and %d after the first", got, counts[many]) + } + drawn := map[fyne.CanvasObject]bool{} + walk(content, func(o fyne.CanvasObject) { drawn[o] = true }) + for i, control := range fields.Controls() { + if !drawn[control] { + t.Errorf("the registry holds a field for %q the screen does not draw", fields.All()[i].Setting) + } + } + // And the other way round, for what the two choices declare: a box on the + // screen with no field behind it is a box whose refusal has nowhere to go, + // which is what a mark that cut the format's fields would leave. + d, err := format.Get(many) + if err != nil { + t.Fatal(err) + } + for _, name := range settingNames(d.Properties, chosen.Parameters) { + field := fields.Lookup(name) + if field == nil || !drawn[field.Control] { + t.Errorf("%q of %s with %s is drawn without a field in the registry behind it", name, many, chosen.ID) + } + } +} + +// settingNames is the name of every setting in the lists given, in order. +func settingNames(lists ...[]format.Property) []string { + var out []string + for _, list := range lists { + for _, p := range list { + out = append(out, p.Name) + } + } + return out +} + // A run started from the window is damaged, and the bytes say so. // // Pressed rather than settled, because settle is the window's own and a guard diff --git a/internal/guard/letgo_test.go b/internal/guard/letgo_test.go new file mode 100644 index 00000000..452c5520 --- /dev/null +++ b/internal/guard/letgo_test.go @@ -0,0 +1,102 @@ +package guard + +import ( + "go/ast" + "go/types" + "path/filepath" + "runtime" + "testing" + "weak" + + "github.com/donislawdev/TestingFilesGenerator/internal/gui/parts" +) + +// A field taken off the registry is let go, and so is a control without a +// setting that went with it. +// +// Cutting a slice keeps its array, and the array keeps every pointer past the +// cut: the boxes of the format chosen before, their closures and whatever those +// hold, alive for as long as the screen is (R19 of the stability review). +// Asked of the collector through weak pointers, because nothing outside the +// registry can see the array - a guard reading the length would pass on +// exactly this defect. +func TestAFieldTakenOffTheRegistryIsLetGo(t *testing.T) { + fields := parts.NewFields() + fields.Add("kept", "Kept", "", parts.Detail{}, parts.NewEntry()) + field, control := addedToBeCut(fields) + fields.KeepFirst(1) + runtime.GC() + runtime.GC() + + if fields.Len() != 1 || fields.Lookup("kept") == nil { + t.Fatalf("the cut left %d field(s) rather than the first one, so this proved nothing", fields.Len()) + } + if field.Value() != nil { + t.Error("a field cut off the registry is still held by it") + } + if control.Value() != nil { + t.Error("a control without a setting cut off the registry is still held by it") + } + runtime.KeepAlive(fields) +} + +// addedToBeCut puts a field and a control without a setting after the first +// field, and keeps nothing of either but a weak pointer, so the registry is the +// only thing left that can hold them. A function of its own so that no variable +// of the guard outlives the call. +func addedToBeCut(fields *parts.Fields) (weak.Pointer[parts.Field], weak.Pointer[parts.Chooser]) { + fields.Add("gone", "Gone", "", parts.Detail{}, parts.NewEntry()) + menu := parts.NewChooser([]string{"one", "two"}, nil) + fields.Named("Way", parts.Detail{}, menu) + return weak.Make(fields.Lookup("gone")), weak.Make(menu) +} + +// Nothing is deleted from a slice by gluing the halves on either side of it. +// +// append(x[:i], x[i+1:]...) moves the tail down one place and leaves the last +// place of the array holding what stood there, so a list of pointers keeps the +// last one alive until something is written over it - a batch taken off the +// recipe screen, boxes and all (R19 of the stability review). slices.Delete +// clears that place. Asked of the code rather than of a screen, because a +// widget that was ever drawn is held by the toolkit's own cache of renderers, +// and a guard asking whether the batch was let go would be red for a reason +// that has nothing to do with the list. +func TestNothingIsDeletedFromASliceByGluingItsHalves(t *testing.T) { + root := repoRoot(t) + read := 0 + var glued []string + for _, dir := range []string{"internal", "cmd"} { + walkGo(t, filepath.Join(root, dir), func(path string, file *ast.File) { + read++ + ast.Inspect(file, func(n ast.Node) bool { + if call, ok := n.(*ast.CallExpr); ok && gluesItsHalves(call) { + rel, _ := filepath.Rel(root, path) + glued = append(glued, filepath.ToSlash(rel)+": "+types.ExprString(call)) + } + return true + }) + }) + } + if read < 100 { + t.Fatalf("read %d Go files, so the walk did not reach the code", read) + } + for _, g := range glued { + t.Errorf("%s - use slices.Delete, which clears the place left at the end", g) + } +} + +// gluesItsHalves is append(x[:i], x[i+1:]...) with the same x on both sides, +// however x and i are spelled. +func gluesItsHalves(call *ast.CallExpr) bool { + fun, ok := call.Fun.(*ast.Ident) + if !ok || fun.Name != "append" || !call.Ellipsis.IsValid() || len(call.Args) != 2 { + return false + } + head, ok1 := call.Args[0].(*ast.SliceExpr) + tail, ok2 := call.Args[1].(*ast.SliceExpr) + if !ok1 || !ok2 || head.Low != nil || head.High == nil || tail.Low == nil || tail.High != nil { + return false + } + return types.ExprString(head.X) == types.ExprString(tail.X) && + types.ExprString(tail.Low) == types.ExprString(head.High)+" + 1" +} diff --git a/internal/guard/mutation/entries.py b/internal/guard/mutation/entries.py index 02b76cef..18198384 100644 --- a/internal/guard/mutation/entries.py +++ b/internal/guard/mutation/entries.py @@ -617,14 +617,14 @@ ), ( "removing a batch removes nothing, so the ones below it never renumber", - "internal/gui/window/recipe.go", - "r.batches = append(r.batches[:index], r.batches[index+1:]...)", - "r.batches = append(r.batches[:index], r.batches[index:]...)", + "internal/gui/window/recipelist.go", + "r.batches = slices.Delete(r.batches, index, index+1)", + "r.batches = slices.Delete(r.batches, index, index)", "TestAddingAndRemovingBatchesKeepsWhatWasTypedAndRenumbersTheRest", ), ( "the last batch can be removed, leaving a form that can produce nothing", - "internal/gui/window/recipe.go", + "internal/gui/window/recipelist.go", "if len(r.batches) <= 1 && !r.base.carriesTheRun() {", "if len(r.batches) <= 1 && false {", "TestTheLastBatchCannotBeRemoved", @@ -5135,7 +5135,7 @@ # usually differ in one setting. A copy that arrives empty is a form # somebody types out again. "copying a batch copies nothing but the batch", - "internal/gui/window/recipe.go", + "internal/gui/window/recipelist.go", " to.count.SetText(from.count.Text)", " to.count.SetText(\"\")", "TestACopiedBatchCarriesEverythingButItsName", @@ -5145,7 +5145,7 @@ # draws it, so a copy can read it. Built later, a copy took the values # of the other two ways and then showed the first one. "a copied batch forgets which way it states its size", - "internal/gui/window/recipe.go", + "internal/gui/window/recipelist.go", " if from.sizeWay != nil && to.sizeWay != nil {", " if false && from.sizeWay != nil && to.sizeWay != nil {", "TestACopiedBatchCarriesEverythingButItsName", @@ -10546,7 +10546,7 @@ ), ( "a batch added to the screen arrives carrying the first one's name", - "internal/gui/window/recipe.go", + "internal/gui/window/recipelist.go", "\tr.batches[len(r.batches)-1].formatPick.SetSelected(format.IDs()[0])\n", "\tr.batches[len(r.batches)-1].formatPick.SetSelected(format.IDs()[0])\n\tr.batches[len(r.batches)-1].id.SetText(startingBatchName)\n", "TestOnlyTheFirstBatchArrivesFilledIn", @@ -16580,4 +16580,46 @@ " .github/build-tags'", "TestTheRaceJobLaysItsPartsOutByTheWeights", ), + ( + "a field cut off the registry stays held by the array it was cut from", + "internal/gui/parts/fields.go", + "\tclear(s.list[n:])\n", + "", + "TestAFieldTakenOffTheRegistryIsLetGo", + ), + ( + "a control without a setting cut off the registry stays held by the array it was cut from", + "internal/gui/parts/fields.go", + "\tclear(s.bare[len(kept):])\n", + "", + "TestAFieldTakenOffTheRegistryIsLetGo", + ), + ( + "a batch taken off the recipe screen stays held at the end of the list", + "internal/gui/window/recipelist.go", + "r.batches = slices.Delete(r.batches, index, index+1)", + "r.batches = append(r.batches[:index], r.batches[index+1:]...)", + "TestNothingIsDeletedFromASliceByGluingItsHalves", + ), + ( + "a content taken off a batch stays held at the end of the list", + "internal/gui/window/recipelist.go", + "b.contents = slices.Delete(b.contents, index, index+1)", + "b.contents = append(b.contents[:index], b.contents[index+1:]...)", + "TestNothingIsDeletedFromASliceByGluingItsHalves", + ), + ( + "each choice of a damage leaves the parameters of the one before in the registry", + "internal/gui/window/generate.go", + "\tg.fields.KeepFirst(g.damage.from)\n", + "", + "TestChoosingADamageAgainLeavesNoFieldBehind", + ), + ( + "a choice of damage cuts the chosen format's fields off the registry", + "internal/gui/window/generate.go", + "\t\tg.damage.from = g.fields.Len()\n", + "\t\tg.damage.from = g.fixed\n", + "TestChoosingADamageAgainLeavesNoFieldBehind", + ), ] diff --git a/internal/gui/parts/fields.go b/internal/gui/parts/fields.go index 6bb3911f..a9bd997c 100644 --- a/internal/gui/parts/fields.go +++ b/internal/gui/parts/fields.go @@ -496,6 +496,10 @@ func (s *Fields) KeepFirst(n int) { if n < 0 || n > len(s.list) { return } + // Cleared before it is cut, here and for bare below: a cut slice keeps its + // array, and the array would keep every field past the cut alive - box, + // closures and all - for as long as the screen lives. + clear(s.list[n:]) s.list = s.list[:n] // Rebuilt rather than deleted from, because the same key can have been // added twice and the map holds the last one. Walking forwards leaves @@ -510,6 +514,7 @@ func (s *Fields) KeepFirst(n int) { kept = append(kept, b) } } + clear(s.bare[len(kept):]) s.bare = kept } diff --git a/internal/gui/window/generate.go b/internal/gui/window/generate.go index bd841478..b8c647a7 100644 --- a/internal/gui/window/generate.go +++ b/internal/gui/window/generate.go @@ -460,7 +460,12 @@ func (g *Generate) onFormatChosen(id string) { // away too. Deferred rather than called at the end because this function // has three ways out, and the one that returns early on an unknown format // would otherwise leave the screen holding a menu with no fields under it. - defer g.rebuildDamageFields() + // Whichever way it left, the format's fields are all in by then, so that is + // where the parameters start. + defer func() { + g.damage.from = g.fields.Len() + g.rebuildDamageFields() + }() d, err := format.Get(id) if err != nil { @@ -512,6 +517,11 @@ type damagePanel struct { // fold because the fold is built again on every rebuild and one that // remembered nothing would spring open unasked. folded bool + // from is how many fields stand before the parameters in the registry: + // the ones that are always here and the chosen format's. Set where the + // format's fields are finished, the one place a change of format always + // passes, so a change of damage takes away exactly what it drew last time. + from int } func (g *Generate) onDamageChosen(string) { @@ -529,6 +539,10 @@ func (g *Generate) onDamageChosen(string) { // its field with no window code at all, which is what makes the registry // answer D1 rather than this screen answering it. func (g *Generate) rebuildDamageFields() { + // The parameters of the damage chosen before go with their boxes, for the + // reason the format's go in onFormatChosen. Without this each choice left + // one more field in the registry, under nothing on the screen. + g.fields.KeepFirst(g.damage.from) g.damage.box.RemoveAll() g.damage.props = nil g.damage.fold = nil diff --git a/internal/gui/window/recipe.go b/internal/gui/window/recipe.go index 94ac0ea4..436736f1 100644 --- a/internal/gui/window/recipe.go +++ b/internal/gui/window/recipe.go @@ -573,83 +573,6 @@ func (r *Recipe) outputSection() fyne.CanvasObject { ) } -// addBatch puts another batch at the end of the list. -func (r *Recipe) addBatch() { - newBatchAtTheEnd(r) - afterAPress(r) -} - -// newBatchAtTheEnd puts a batch after the last one, without saying what the -// form comes to. Apart from addBatch for the one caller that is not a press: -// the base switched off with no batch left brings one back, and the switch's -// own check says the line. Through addBatch it was said twice - found by the -// review of #133 (docs/REVIEW-133-2026-09-24.md). -func newBatchAtTheEnd(r *Recipe) { - r.batches = append(r.batches, r.newBatch()) - // A new batch has no format until one is chosen, and its declared settings - // come with that choice. Chosen here rather than left empty so that a batch - // arrives looking like the one above it. - r.batches[len(r.batches)-1].formatPick.SetSelected(format.IDs()[0]) -} - -// removeBatch drops one batch. The last cannot go, unless the screen builds -// on a preset (base.carriesTheRun): a screen with no batches and no preset -// can produce nothing, and would answer a press with a refusal about a -// document rather than about anything anybody did. -// duplicateBatch copies one batch and puts the copy under it. -// -// Batches usually differ from each other in one setting - a size, a format, a -// count - so writing the second one from an empty form is typing the first one -// again. The copy is a new batch with the same values rather than a shared one, -// so changing it changes nothing else. -// -// The name is left empty on the copy, on purpose. Two targets with one id is a -// refusal the recipe reader already words and addresses, and handing somebody a -// form that is refused the moment they press a button would be a copy that has -// to be repaired before it can be used. Empty is refused too, and it is refused -// pointing at a box that is asking to be filled in. -func (r *Recipe) duplicateBatch(index int) { - if index < 0 || index >= len(r.batches) { - return - } - from := r.batches[index] - to := r.newBatch() - - to.formatPick.SetSelected(from.formatPick.Selected) - to.count.SetText(from.count.Text) - to.size.SetText(from.size.Text) - to.sizeRange.SetText(from.sizeRange.Text) - to.boundary.SetText(from.boundary.Text) - to.name.SetText(from.name.Text) - to.notes.takeFrom(from.notes) - if from.sizeWay != nil && to.sizeWay != nil { - to.sizeWay.SetSelected(from.sizeWay.Selected) - } - - rest := append([]*batch{to}, r.batches[index+1:]...) - r.batches = append(r.batches[:index+1], rest...) - afterAPress(r) -} - -func (r *Recipe) removeBatch(index int) { - if index < 0 || index >= len(r.batches) { - return - } - if len(r.batches) <= 1 && !r.base.carriesTheRun() { - return - } - r.batches = append(r.batches[:index], r.batches[index+1:]...) - afterAPress(r) -} - -func (r *Recipe) removeContent(b *batch, index int) { - if index < 0 || index >= len(b.contents) { - return - } - b.contents = append(b.contents[:index], b.contents[index+1:]...) - afterAPress(r) -} - // onFormatChosen replaces the declared settings of one batch. func (r *Recipe) onFormatChosen(b *batch, id string) { d, err := format.Get(id) diff --git a/internal/gui/window/recipelist.go b/internal/gui/window/recipelist.go new file mode 100644 index 00000000..85fb0c08 --- /dev/null +++ b/internal/gui/window/recipelist.go @@ -0,0 +1,93 @@ +package window + +import ( + "slices" + + "github.com/donislawdev/TestingFilesGenerator/internal/format" +) + +// The list of batches on the recipe screen, and the list of what each batch +// holds: what a press adds to them, copies and takes away. Out of recipe.go +// on 2026-11-03, when that file reached its ceiling - the screen builds the +// batches, and this is the part that only changes how many there are. + +// addBatch puts another batch at the end of the list. +func (r *Recipe) addBatch() { + newBatchAtTheEnd(r) + afterAPress(r) +} + +// newBatchAtTheEnd puts a batch after the last one, without saying what the +// form comes to. Apart from addBatch for the one caller that is not a press: +// the base switched off with no batch left brings one back, and the switch's +// own check says the line. Through addBatch it was said twice - found by the +// review of #133 (docs/REVIEW-133-2026-09-24.md). +func newBatchAtTheEnd(r *Recipe) { + r.batches = append(r.batches, r.newBatch()) + // A new batch has no format until one is chosen, and its declared settings + // come with that choice. Chosen here rather than left empty so that a batch + // arrives looking like the one above it. + r.batches[len(r.batches)-1].formatPick.SetSelected(format.IDs()[0]) +} + +// duplicateBatch copies one batch and puts the copy under it. +// +// Batches usually differ from each other in one setting - a size, a format, a +// count - so writing the second one from an empty form is typing the first one +// again. The copy is a new batch with the same values rather than a shared one, +// so changing it changes nothing else. +// +// The name is left empty on the copy, on purpose. Two targets with one id is a +// refusal the recipe reader already words and addresses, and handing somebody a +// form that is refused the moment they press a button would be a copy that has +// to be repaired before it can be used. Empty is refused too, and it is refused +// pointing at a box that is asking to be filled in. +func (r *Recipe) duplicateBatch(index int) { + if index < 0 || index >= len(r.batches) { + return + } + from := r.batches[index] + to := r.newBatch() + + to.formatPick.SetSelected(from.formatPick.Selected) + to.count.SetText(from.count.Text) + to.size.SetText(from.size.Text) + to.sizeRange.SetText(from.sizeRange.Text) + to.boundary.SetText(from.boundary.Text) + to.name.SetText(from.name.Text) + to.notes.takeFrom(from.notes) + if from.sizeWay != nil && to.sizeWay != nil { + to.sizeWay.SetSelected(from.sizeWay.Selected) + } + + rest := append([]*batch{to}, r.batches[index+1:]...) + r.batches = append(r.batches[:index+1], rest...) + afterAPress(r) +} + +// removeBatch drops one batch. The last cannot go, unless the screen builds +// on a preset (base.carriesTheRun): a screen with no batches and no preset +// can produce nothing, and would answer a press with a refusal about a +// document rather than about anything anybody did. +func (r *Recipe) removeBatch(index int) { + if index < 0 || index >= len(r.batches) { + return + } + if len(r.batches) <= 1 && !r.base.carriesTheRun() { + return + } + // slices.Delete rather than append of the two halves, because it clears the + // place the last batch stood in: the halves glued together leave it + // holding that batch, boxes and all, until something is written over it. + r.batches = slices.Delete(r.batches, index, index+1) + afterAPress(r) +} + +// removeContent drops one entry from what a batch holds. +func (r *Recipe) removeContent(b *batch, index int) { + if index < 0 || index >= len(b.contents) { + return + } + b.contents = slices.Delete(b.contents, index, index+1) + afterAPress(r) +} From 46c986db506ef2f1b2f924da369058f5225dc2cf Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 4 Nov 2026 00:22:57 +0100 Subject: [PATCH 2/3] gui: Cancel no longer waits for the work, and closing the window waits for it off the window's thread Cancel, Escape and closing the window all cancelled the work and then waited for the worker on the interface thread. The engine asks its context between writes, so on a disk that holds on to a write the window stopped drawing for as long as the write took, and Windows offers to end a window that has not drawn for five seconds - which ends the process in the middle of a file. Cancel now asks the work to stop, switches itself off and says it is stopping until the worker's last word replaces the line. Closing the window during work writes the folder down, shows a Closing notice in place of the screens, points the keyboard at that notice so no key starts work, waits for every screen on a goroutine of its own and closes from the window's queue. A second press of the close button does nothing. A window with nothing running closes at once, as before. There is no way to close without waiting. work.go is what a screen keeps of its work, in place of the runner's stop and settled and the Tools tab's toolJob: cancel, done, and a mutex under which Cancel and the worker's news change the screen. In the program both are on the interface thread and the lock is never contended. Under the toolkit's test driver fyne.Do runs on the worker, and without the lock the four guards that press Cancel during a run were 12 of 12 red under -race. work.go and closing.go are declared in the concurrency guard and in the race job's watched list. On the Tools tab Escape presses Cancel only while it is shown, as on the other screens, and the hold the guards use moved into the busy state so the tab has it too. Four new guards, four that now wait for the work after pressing, eleven new mutation entries and eight repointed. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 2 +- CHANGELOG.md | 8 + internal/guard/alwaysvisible_test.go | 3 + internal/guard/concurrency_test.go | 13 ++ internal/guard/generatewindow_test.go | 38 +++-- internal/guard/guitext_test.go | 1 + internal/guard/mutation/entries.py | 131 +++++++++++---- internal/guard/mutationcoverage_test.go | 9 +- internal/guard/runstate_test.go | 4 +- internal/guard/segmentface_test.go | 4 +- internal/guard/stopping_test.go | 211 ++++++++++++++++++++++++ internal/guard/window_test.go | 8 +- internal/gui/text/locale/en.json | 15 ++ internal/gui/text/locale/pl.json | 3 + internal/gui/text/text.go | 20 +++ internal/gui/window/closing.go | 122 ++++++++++++++ internal/gui/window/open.go | 61 +------ internal/gui/window/run.go | 181 ++++++++------------ internal/gui/window/runbusy.go | 39 ++++- internal/gui/window/tools.go | 92 +++++------ internal/gui/window/work.go | 114 +++++++++++++ 21 files changed, 808 insertions(+), 271 deletions(-) create mode 100644 internal/guard/stopping_test.go create mode 100644 internal/gui/window/closing.go create mode 100644 internal/gui/window/work.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 337532ad..47a5ee43 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1060,7 +1060,7 @@ jobs: # in somebody else's file. run: | set -euo pipefail - watched='internal/format/registry.go internal/damage/damage.go cmd/tfg/main.go internal/gui/window/run.go internal/gui/run_cgo.go internal/gui/window/tidy.go internal/audit/parallel.go internal/engine/parallel.go internal/tool/tool.go internal/gui/window/tools.go internal/format/video/ahead.go go.mod .github/workflows/ci.yml .github/build-tags .github/race-weights .github/scripts/race_parts.py' + watched='internal/format/registry.go internal/damage/damage.go cmd/tfg/main.go internal/gui/window/run.go internal/gui/run_cgo.go internal/gui/window/tidy.go internal/audit/parallel.go internal/engine/parallel.go internal/tool/tool.go internal/gui/window/tools.go internal/gui/window/work.go internal/gui/window/closing.go internal/format/video/ahead.go go.mod .github/workflows/ci.yml .github/build-tags .github/race-weights .github/scripts/race_parts.py' # On a pull request there is no "before" - the field belongs to a push # - so this asked for something empty and every pull request answered # "touched". That quietly undid the decision of 2026-08-20, because diff --git a/CHANGELOG.md b/CHANGELOG.md index ca7131e8..578b8169 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,14 @@ because it turns other people's test suites red. ### Fixed +- **Cancel answers at once, and closing the window during a run no longer + freezes it.** Both used to wait for the write in progress with the window + standing still, which on a slow or network disk could last long enough for + Windows to offer to end the program in the middle of a file. Cancel now + switches itself off and says the run is stopping until it has. Closing the + window shows that it closes once the work has stopped, and then it does. + Nothing is left half written either way, and the manifest still describes + what was. - **A run of many files no longer runs out of memory while it saves its manifest.** The manifest was put together in memory in one piece, about ten times its own size, after the last file. In a container with 1 GB a diff --git a/internal/guard/alwaysvisible_test.go b/internal/guard/alwaysvisible_test.go index d789f46d..33e4d426 100644 --- a/internal/guard/alwaysvisible_test.go +++ b/internal/guard/alwaysvisible_test.go @@ -83,7 +83,10 @@ func TestAddingABatchIsOutOfUseWhileARunIsGoing(t *testing.T) { if cancel == nil { t.Fatalf("there is no Cancel button. The screen has: %v", buttonNames(body)) } + // Cancel asks the run to stop and does not wait for it, so the run is + // waited for here - the form comes back with the run's last word (R06). cancel.OnTapped() + screen.Settled() if add.Disabled() { t.Error("the add button is still out of use after the run stopped, so it never comes back.") diff --git a/internal/guard/concurrency_test.go b/internal/guard/concurrency_test.go index 3388d3aa..f527edfa 100644 --- a/internal/guard/concurrency_test.go +++ b/internal/guard/concurrency_test.go @@ -52,6 +52,19 @@ var mayBeConcurrent = map[string]string{ // invariant G7 exists to hold. Added 2026-08-05 with the first generate // window, and the owner was told. "internal/gui/window/run.go": "the run happens beside the window, and closing the window waits for it", + // The handle on one piece of work beside the window, out of run.go and + // tools.go on 2026-11-03 when Cancel stopped waiting (O262, the owner's + // decision that day). It holds the first lock in the window, and the lock + // is for the test driver: there fyne.Do runs the worker's progress on the + // worker, so a press of Cancel and a line of progress are two goroutines + // writing one status line. In the shipped program both are on the + // interface thread and the lock is never contended. + "internal/gui/window/work.go": "Cancel and the worker's news say things to one screen, ordered by one lock", + // Closing the window during work waits for it on a goroutine of its own, + // so the window goes on drawing while the disk finishes the write in hand + // - the owner's decision of 2026-11-03 (O262). Waiting on the interface + // thread was a window Windows offers to end, in the middle of a file. + "internal/gui/window/closing.go": "closing the window waits for the work beside the window, not on its thread", // The window's clock. time.AfterFunc calls back on a goroutine of its // own, which is how a delayed piece of work - the busy face, the wait for // quiet - waits without holding the window, and the callback hands its diff --git a/internal/guard/generatewindow_test.go b/internal/guard/generatewindow_test.go index 98474fb4..af46f3ca 100644 --- a/internal/guard/generatewindow_test.go +++ b/internal/guard/generatewindow_test.go @@ -454,6 +454,12 @@ func TestGeneratingFromTheWindowWritesTheFilesAndTheManifest(t *testing.T) { // and closing straight away ends the process while the engine is still winding // down, so the manifest describing what did get written never lands - and that // manifest is the only thing cleanup can work from. +// +// The wait happens off the interface thread since 2026-11-03 (R06), so the +// closing is joined here before the disk is read. That the window is let go +// only once the run has stopped is held by +// TestClosingDuringARunKeepsTheWindowDrawingUntilTheRunHasStopped, which can +// see the moment in between. func TestClosingTheWindowDuringARunStopsItAndWaitsForIt(t *testing.T) { dir := t.TempDir() host, content := screen(t) @@ -468,14 +474,13 @@ func TestClosingTheWindowDuringARunStopsItAndWaitsForIt(t *testing.T) { t.Fatal("the window has no close intercept, so closing it does not reach the run at all") } host.intercept() + join(host) if host.closed != 1 { t.Errorf("the window was closed %d time(s) and it should be closed exactly once", host.closed) } - // The wait is what makes this assertable at all. If closing did not wait, - // the engine would still be writing right now and this count would be a - // number that changes while it is read. + // Joined, so this count is not a number that changes while it is read. names := namesIn(t, dir) for _, name := range names { if strings.Contains(name, core.PartialMarker) { @@ -489,6 +494,11 @@ func TestClosingTheWindowDuringARunStopsItAndWaitsForIt(t *testing.T) { if info.Size() == 0 { t.Error("the manifest is the empty file the run claimed, so closing the window did not wait for it to be written") } + // Stopped rather than waited out: a close that let all four hundred + // through would be a window that cannot be closed during a run. + if raw, err := os.ReadFile(filepath.Join(dir, "manifest.json")); err != nil || !strings.Contains(string(raw), `"complete": false`) { + t.Errorf("the manifest does not say the run was stopped, so closing the window did not reach it (%v)", err) + } t.Logf("closing the window stopped a 400 file run with %d file(s) on disk and a manifest", len(names)-1) } @@ -576,12 +586,13 @@ func TestWhatIsTypedOnTheScreenIsWhatGetsWritten(t *testing.T) { // brief, and every other guard here was green. A control nobody presses is a // control nobody has checked. // -// Deterministic without polling, because stopping waits. By the time the press -// returns the engine has wound down, so what is on the disk is not a number -// that changes while it is read. +// Deterministic without polling, because the run is joined after the press. +// Cancel itself does not wait since 2026-11-03 (R06) - that is held by +// TestCancelAnswersAtOnceAndSaysWhatItWaitsFor - so what is on the disk is +// read once the engine has wound down, not while it changes. func TestCancelStopsTheRun(t *testing.T) { dir := t.TempDir() - _, content := screen(t) + host, content := screen(t) fill(t, content, text.FieldOutputDir(), dir) fill(t, content, text.FieldSize(), "64kb") @@ -594,13 +605,14 @@ func TestCancelStopsTheRun(t *testing.T) { t.Fatalf("there is no Cancel button. The screen has: %v", buttonNames(content)) } cancel.OnTapped() + join(host) info, err := os.Stat(filepath.Join(dir, "manifest.json")) if err != nil { t.Fatalf("no manifest after cancelling: %v", err) } if info.Size() == 0 { - t.Fatal("the manifest is the empty file the run claimed, so cancelling did not wait for the run to wind down") + t.Fatal("the manifest is the empty file the run claimed, so the run that was cancelled never wrote down what it did") } // Stopped rather than finished. A cancel that let all four hundred through @@ -750,17 +762,19 @@ func TestARunIsStoppableFromTheMomentItStarts(t *testing.T) { fn = fn[:end] } - handle := strings.Index(fn, "r.stop = func()") + // The handle is the screen's work since 2026-11-03 (work.go), which took + // the place of r.stop. + handle := strings.Index(fn, "r.work = w") worker := strings.Index(fn, "go func()") switch { case handle < 0: - t.Fatal("startRun no longer assigns r.stop, so nothing would stop a run at all") + t.Fatal("startRun no longer assigns r.work, so nothing would stop a run at all") case worker < 0: t.Fatal("startRun no longer starts a goroutine, so this guard is reading the wrong function") case handle > worker: - t.Error("startRun starts the worker before it assigns r.stop.\n" + + t.Error("startRun starts the worker before it assigns r.work.\n" + "Reason: between those two statements files are being written and Stop has nothing to call, so closing the window there ends the process inside a file with no manifest.\n" + - "What to do: assign r.stop first. ctx, cancel and done all exist by then, so nothing is gained by waiting.") + "What to do: assign r.work first. The work exists by then, so nothing is gained by waiting.") } } diff --git a/internal/guard/guitext_test.go b/internal/guard/guitext_test.go index a2edaed2..3a9f61a5 100644 --- a/internal/guard/guitext_test.go +++ b/internal/guard/guitext_test.go @@ -110,6 +110,7 @@ var notWords = map[string]string{ `"tool-result"`: "the key the section of a tool's result is remembered under, never shown", `"preferences"`: "what the preferences screen is, the key it is known by between screens, never shown", `"about"`: "what the about screen is, the key it is known by between screens, never shown", + `"closing"`: "what the keyboard acts on while the window waits to close - no screen, so no key does anything - never shown", `"file"`: "the scheme of a local address, which is a protocol name rather than prose", `"."`: "the working directory, when the system will not say which one it is", `"tfg-out"`: "the folder the window offers to write into. A directory name rather than " + diff --git a/internal/guard/mutation/entries.py b/internal/guard/mutation/entries.py index 18198384..5f91e058 100644 --- a/internal/guard/mutation/entries.py +++ b/internal/guard/mutation/entries.py @@ -3478,16 +3478,11 @@ "TestGeneratingFromTheWindowWritesTheFilesAndTheManifest", ), ( - # G7, and the half that is easy to leave out. Cancelling and closing at - # once ends the process while the engine is still winding down, so the - # manifest describing what did get written never lands - and that - # manifest is the only thing cleanup can work from. Measured on - # 2026-08-05: red three times out of three, with no manifest at all. "closing the window cancels the run without waiting for it", - "internal/gui/window/run.go", - "\t\tcancel()\n\t\t<-done\n\t}\n\n\tgo func() {\n\t\tplanned, planErr := engine.PlanContext(ctx, targets, opt)\n\t\tif planErr != nil {\n\t\t\t// A refusal", - "\t\tcancel()\n\t\t_ = done\n\t}\n\n\tgo func() {\n\t\tplanned, planErr := engine.PlanContext(ctx, targets, opt)\n\t\tif planErr != nil {\n\t\t\t// A refusal", - "TestClosingTheWindowDuringARunStopsItAndWaitsForIt", + "internal/gui/window/work.go", + "\tw.cancel()\n\t<-w.done\n", + "\tw.cancel()\n\t_ = w.done\n", + "TestClosingDuringARunKeepsTheWindowDrawingUntilTheRunHasStopped", ), ( # Found by running the built window and looking at it, on 2026-08-05. @@ -4247,26 +4242,17 @@ "TestEveryFormatIsClassifiedAsTextOrBinary", ), ( - # Found by coverage rather than by reading: onCancel came back at - # nought per cent while every guard around it was green. The button was - # on the screen and in the brief, and nothing pressed it. "the Cancel button stops reaching the run", "internal/gui/window/run.go", - "func (r *runner) Stop() {\n\tif r.stop != nil {", - "func (r *runner) Stop() {\n\tif r.stop != nil && false {", + "func (r *runner) onCancel() { r.busy.halt(r.work, r.status) }", + "func (r *runner) onCancel() {}", "TestCancelStopsTheRun", ), ( - # O63. Fyne 2.8 warns at every start that changes have to go through - # fyne.Do and says the next major release stops tolerating it. Measured - # on 2026-08-05: under the test driver fyne.Do runs on the calling - # goroutine and a widget touched from a worker does not complain, so - # neither -race nor a rendering test can see this. Static reading is - # what is left. "the window is told the run finished straight from the worker", "internal/gui/window/run.go", - "\t\tfyne.Do(func() { r.runFinished(res, runErr, saveErr, room, saved) })", - "\t\tr.runFinished(res, runErr, saveErr, room, saved)", + "\t\tfyne.Do(func() { w.end(func() { r.runFinished(res, runErr, saveErr, room, saved) }) })", + "\t\tw.end(func() { r.runFinished(res, runErr, saveErr, room, saved) })", "TestTheWindowIsOnlyTouchedFromTheInterfaceThread", ), ( @@ -5287,9 +5273,9 @@ ), ( "closing the window forgets where the files were going", - "internal/gui/window/open.go", - " if screen, ok := working[*showing]; ok {\n h.Remembered().RememberDirectory(screen.OutDir())\n }", - " if screen, ok := working[*showing]; ok && false {\n h.Remembered().RememberDirectory(screen.OutDir())\n }", + "internal/gui/window/closing.go", + "\tif screen, ok := c.working[*c.showing]; ok {\n\t\tc.host.Remembered().RememberDirectory(screen.OutDir())\n\t}", + "\tif screen, ok := c.working[*c.showing]; ok && false {\n\t\tc.host.Remembered().RememberDirectory(screen.OutDir())\n\t}", "TestClosingTheWindowRemembersWhereTheFilesWereGoing", ), ( @@ -10766,8 +10752,8 @@ ( "the worker of a run stays behind after reporting, waiting for a cancel nobody sends", "internal/gui/window/run.go", - "\t\tclose(done)\n\t}()\n}\n\n// runFinished", - "\t\tclose(done)\n\t\t<-ctx.Done()\n\t}()\n}\n\n// runFinished", + "\t\tclose(w.done)\n\t}()\n}\n\n// runFinished", + "\t\tclose(w.done)\n\t\t<-ctx.Done()\n\t}()\n}\n\n// runFinished", "TestTheSameWorkDoneAgainLeavesTheWindowAsItWas", ), # --- the instructions beside the manifest (2026-09-25, @@ -12986,8 +12972,8 @@ ( "Restart now closes the window without the close button's way", "internal/gui/window/open.go", - "prefs := NewPreferences(h, func() { leave() },", - "prefs := NewPreferences(h, func() { _ = leave; h.Close() },", + "prefs := NewPreferences(h, func() { out.leave() },", + "prefs := NewPreferences(h, func() { _ = out; h.Close() },", "TestRestartWaitsForTheRunAndClosesTheWayAPersonDoes", ), ( @@ -16172,8 +16158,8 @@ ( "a run in the window ignores the memory setting kept on Preferences", "internal/gui/window/run.go", - "\topt.Memory, opt.MachineMemory = memoryOf(r.kept()), r.machine\n\tctx, cancel", - "\topt.Memory, opt.MachineMemory = core.Memory{}, r.machine\n\tctx, cancel", + "\topt.Memory, opt.MachineMemory = memoryOf(r.kept()), r.machine\n\t// Set BEFORE", + "\topt.Memory, opt.MachineMemory = core.Memory{}, r.machine\n\t// Set BEFORE", "TestARunPlansUnderTheMemorySettingKeptOnPreferences", ), ( @@ -16246,12 +16232,10 @@ "TestTheWindowSaysWhatARunSaidAboutItsMemory", ), ( - # The run asks the system rather than the host the Preferences screen - # asks, so the screen and the run can name two different machines. "a run in the window asks the system for the machine rather than the host", "internal/gui/window/run.go", - "\topt.Memory, opt.MachineMemory = memoryOf(r.kept()), r.machine\n\tctx, cancel", - "\topt.Memory, opt.MachineMemory = memoryOf(r.kept()), nil\n\tctx, cancel", + "\topt.Memory, opt.MachineMemory = memoryOf(r.kept()), r.machine\n\t// Set BEFORE", + "\topt.Memory, opt.MachineMemory = memoryOf(r.kept()), nil\n\t// Set BEFORE", "TestTheWindowSaysWhatARunSaidAboutItsMemory", ), ( @@ -16622,4 +16606,81 @@ "\t\tg.damage.from = g.fixed\n", "TestChoosingADamageAgainLeavesNoFieldBehind", ), + ( + "Cancel waits for the run again, so the window stops drawing while the disk holds a write", + "internal/gui/window/run.go", + "func (r *runner) onCancel() { r.busy.halt(r.work, r.status) }", + "func (r *runner) onCancel() { r.Stop() }", + "TestCancelAnswersAtOnceAndSaysWhatItWaitsFor", + ), + ( + "Cancel on the Tools tab waits for the tool again", + "internal/gui/window/tools.go", + "func (t *Tools) onCancel() { t.busy.halt(t.job, t.status) }", + "func (t *Tools) onCancel() { t.Stop() }", + "TestCancelOnTheToolsTabAnswersAtOnce", + ), + ( + "Cancel says the run is stopping and never asks it to stop", + "internal/gui/window/work.go", + "\tw.stopping = true\n\tw.cancel()\n", + "\tw.stopping = true\n", + "TestCancelStopsTheRun", + ), + ( + "Cancel stays on after it was pressed, so a second press looks like it would do something", + "internal/gui/window/runbusy.go", + "\t\tb.cancel.Disable()\n\t\tshowOn(status, text.Stopping())\n", + "\t\tshowOn(status, text.Stopping())\n", + "TestCancelAnswersAtOnceAndSaysWhatItWaitsFor", + ), + ( + "Cancel comes back at once and says nothing, so it looks like a press that did nothing", + "internal/gui/window/runbusy.go", + "\t\tshowOn(status, text.Stopping())\n", + "\t\t_ = text.Stopping()\n", + "TestCancelAnswersAtOnceAndSaysWhatItWaitsFor", + ), + ( + "closing the window waits for the run without asking it to stop", + "internal/gui/window/work.go", + "\tw.cancel()\n\t<-w.done\n", + "\t<-w.done\n", + "TestClosingTheWindowDuringARunStopsItAndWaitsForIt", + ), + ( + "closing the window during a run lets it go at once, in the middle of a file", + "internal/gui/window/closing.go", + "\tif !anyBusy(c.busy) {\n", + "\tif true {\n", + "TestClosingDuringARunKeepsTheWindowDrawingUntilTheRunHasStopped", + ), + ( + "the window closing during a run shows nothing of what it waits for", + "internal/gui/window/closing.go", + "\tc.host.SetContent(parts.Screen(parts.Titled(text.HeadingClosing(), text.SubtitleClosing())))\n", + "\t_ = parts.Screen(parts.Titled(text.HeadingClosing(), text.SubtitleClosing()))\n", + "TestClosingDuringARunKeepsTheWindowDrawingUntilTheRunHasStopped", + ), + ( + "a second press of the close button begins a second wait and lets the window go twice", + "internal/gui/window/closing.go", + "\tif c.begun {\n\t\treturn\n\t}\n", + "", + "TestClosingDuringARunKeepsTheWindowDrawingUntilTheRunHasStopped", + ), + ( + "a key starts work on an idle screen while the window waits to close", + "internal/gui/window/closing.go", + "\t*c.looking = tabClosing\n", + "", + "TestNoKeyStartsWorkWhileTheWindowWaitsToClose", + ), + ( + "closing the window asks no screen to stop, so it goes with the run still writing", + "internal/gui/window/closing.go", + "\t\t\tscreen.Stop()\n", + "\t\t\t_ = screen\n", + "TestClosingTheWindowDuringARunStopsItAndWaitsForIt", + ), ] diff --git a/internal/guard/mutationcoverage_test.go b/internal/guard/mutationcoverage_test.go index b3f5ba04..7e1196c3 100644 --- a/internal/guard/mutationcoverage_test.go +++ b/internal/guard/mutationcoverage_test.go @@ -203,11 +203,10 @@ var provenByProbe = map[string]string{ "A probe rather than a mutation entry because there is no product code underneath: it reads docs/REGRESSION.md (both halves of it since 2026-09-21), which is not in the repository, " + "so a substitution in a .go file never reaches it.", - "TestARunIsStoppableFromTheMomentItStarts": "checked 2026-08-23 with tools/probes/run-stop-order.py, which reproduces the original defect faithfully: it moves the assignment of r.stop " + - "to after the goroutine that writes the files, builds, and runs the guard. Reported CAUGHT, and the file was restored. " + - "A probe rather than a mutation entry for two reasons, and the first one is the interesting one. The obvious substitution - deleting the handle - does NOT compile, because cancel is then unused, " + - "and a mutation that does not compile proves nothing while reporting as though it did. The faithful version is a MOVE of fifteen lines rather than a substitution of one piece of text for another, " + - "and the entries in entries.py have to be literals.", + "TestARunIsStoppableFromTheMomentItStarts": "checked 2026-08-23 and again 2026-11-04 with tools/probes/run-stop-order.py, which reproduces the original defect faithfully: it moves the assignment of the handle " + + "(r.stop then, r.work since the work got a type of its own) to after the goroutine that writes the files, builds, and runs the guard. Reported CAUGHT both times, and the file was restored. " + + "A probe rather than a mutation entry because the defect is a MOVE of a statement past the goroutine, not a substitution of one piece of text for another, " + + "and the entries in entries.py have to be literals. Deleting the handle would compile since 2026-11-04 and would only prove that it is assigned, not where.", "TestTheTaskbarIconIsTheSameDrawingAsTheWindowIcon": "checked 2026-08-13 with tools/probes/exe-icon.ps1, run on both binaries. " + "It reported 7 of 7 icon images inside tfg-gui.exe and 0 of 7 inside tfg.exe, which is the right answer for a command line with no window. " + diff --git a/internal/guard/runstate_test.go b/internal/guard/runstate_test.go index 380619f6..5b7baa96 100644 --- a/internal/guard/runstate_test.go +++ b/internal/guard/runstate_test.go @@ -24,7 +24,7 @@ import ( // only looked at the middle of a run. func TestTheFormCannotBeEditedWhileARunIsGoing(t *testing.T) { dir := t.TempDir() - _, content := screen(t) + host, content := screen(t) fill(t, content, text.FieldOutputDir(), dir) fill(t, content, text.FieldSize(), "64kb") @@ -44,7 +44,9 @@ func TestTheFormCannotBeEditedWhileARunIsGoing(t *testing.T) { if cancel == nil { t.Fatalf("there is no Cancel button. The screen has: %v", buttonNames(content)) } + // Cancel does not wait for the run (R06), so the run is waited for here. cancel.OnTapped() + join(host) if box.Disabled() { t.Error("the size box is still frozen after the run stopped, so the form never comes back.") diff --git a/internal/guard/segmentface_test.go b/internal/guard/segmentface_test.go index 1015e559..6ff3ff35 100644 --- a/internal/guard/segmentface_test.go +++ b/internal/guard/segmentface_test.go @@ -26,7 +26,7 @@ import ( // single batch screen, which has no such switch (O223). func TestTheSizeWaySwitchFreezesWithTheRestOfTheForm(t *testing.T) { dir := t.TempDir() - batches, _, _ := screenInAWindowWithHost(t, text.TabRecipe()) + batches, _, host := screenInAWindowWithHost(t, text.TabRecipe()) entryUnder(t, batches, text.FieldTargetID()).SetText("invoices") entryUnder(t, batches, text.FieldOutputDir()).SetText(dir) entryUnder(t, batches, text.FieldSize()).SetText("64kb") @@ -47,7 +47,9 @@ func TestTheSizeWaySwitchFreezesWithTheRestOfTheForm(t *testing.T) { if cancel == nil { t.Fatalf("there is no Cancel button. The screen has: %v", buttonNames(batches)) } + // Cancel does not wait for the run (R06), so the run is waited for here. cancel.OnTapped() + join(host) if sizeWaySwitch(t, batches).Disabled() { t.Error("the switch is still frozen after the run stopped, so it never comes back") } diff --git a/internal/guard/stopping_test.go b/internal/guard/stopping_test.go new file mode 100644 index 00000000..a619830f --- /dev/null +++ b/internal/guard/stopping_test.go @@ -0,0 +1,211 @@ +package guard + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" + + "fyne.io/fyne/v2" + + "github.com/donislawdev/TestingFilesGenerator/internal/gui/parts" + "github.com/donislawdev/TestingFilesGenerator/internal/gui/text" + "github.com/donislawdev/TestingFilesGenerator/internal/tool/checksum" +) + +// Stopping work does not hold the window (R06 in +// docs/STABILITY-REVIEW-2026-10-26.md, O262, the owner's decision of +// 2026-11-03). +// +// Cancel, Escape and closing the window all used to cancel and then wait for +// the worker, on the interface thread. The engine asks its context between +// writes, so on a disk that answers that was milliseconds, and on one that +// holds on to a write it was a window that did not draw for as long - five +// seconds of that and Windows offers to end it, which ends the process in the +// middle of a file. Every guard here parks the worker just before it reports, +// which is a write that has not come back as far as the window can tell, and +// asks the press to come back while it is parked. + +// Close counts, and tells letGo the first time when a guard is listening. +// Closing during work lets the window go from a goroutine of its own, so a +// guard that has to see it NOT happen while a run is held waits on this rather +// than reading the count at a moment nothing orders. Here rather than beside +// the stand in, whose file is in the band below the ceiling on test files. +func (h *fakeHost) Close() { + h.closed++ + if h.closed == 1 && h.letGo != nil { + close(h.letGo) + } +} + +// comesBackWhileHeld presses something that must not wait for the work, with the +// worker parked, and says whether it came back. A press that waits would wait +// for ever - the worker is parked until the guard lets it go - so it is given +// a while and then the worker is let go, which lets the press come back and +// the guard fail rather than hang the package. +func comesBackWhileHeld(hold *holdDuringRun, press func()) bool { + back := make(chan struct{}) + go func() { + press() + close(back) + }() + select { + case <-back: + return true + case <-time.After(5 * time.Second): + hold.free() + <-back + return false + } +} + +// Cancel comes back while the run is still going, switches itself off, and +// the line says what is being waited for - until the run's last word replaces +// it. +func TestCancelAnswersAtOnceAndSaysWhatItWaitsFor(t *testing.T) { + host, content, hold := heldScreen(t) + fill(t, content, text.FieldOutputDir(), t.TempDir()) + fill(t, content, text.FieldSize(), "1kb") + fill(t, content, text.FieldCount(), "3") + press(t, content, text.ButtonGenerate()) + + var cancel *parts.Button + hold.look(func() { + cancel = buttonNamed(content, text.ButtonCancel()) + if cancel == nil || !cancel.Visible() || cancel.Disabled() { + t.Fatal("there is no live Cancel while the run is held, so this guard presses nothing") + } + if !comesBackWhileHeld(hold, cancel.OnTapped) { + t.Fatal("Cancel waited for the run, so the window stops drawing for as long as the disk holds on to a write") + } + if !cancel.Disabled() { + t.Error("Cancel is still on after it was pressed, so a second press looks like it would do something") + } + if said := textIn(content); !strings.Contains(said, text.Stopping()) { + t.Errorf("the screen does not say it is stopping, so a press that came back at once looks like a press that did nothing:\n%s", said) + } + }) + join(host) + + if said := textIn(content); strings.Contains(said, text.Stopping()) { + t.Errorf("the run has ended and the screen still says it is stopping:\n%s", said) + } + if cancel.Visible() { + t.Error("the run has ended and Cancel is still on the screen") + } +} + +// The same on the Tools tab, whose Cancel is a button of its own. +func TestCancelOnTheToolsTabAnswersAtOnce(t *testing.T) { + host, _, hold := heldScreen(t) + screen := selectTab(t, host.content, text.TabTools()) + chooseTool(t, screen, checksum.ID) + fillField(t, screen, text.SettingLabel(checksum.InputFile), writeTemp(t, "stopped.txt", "abc")) + pressNamed(t, screen, text.ButtonRunTool()) + + hold.look(func() { + cancel := buttonNamed(screen, text.ButtonCancel()) + if cancel == nil || !cancel.Visible() || cancel.Disabled() { + t.Fatal("there is no live Cancel while the tool is held, so this guard presses nothing") + } + if !comesBackWhileHeld(hold, cancel.OnTapped) { + t.Fatal("Cancel on the Tools tab waited for the tool") + } + if !cancel.Disabled() { + t.Error("Cancel on the Tools tab is still on after it was pressed") + } + if said := textIn(screen); !strings.Contains(said, text.Stopping()) { + t.Errorf("the Tools tab does not say it is stopping:\n%s", said) + } + }) + join(host) + if said := textIn(screen); strings.Contains(said, text.Stopping()) { + t.Errorf("the tool has ended and the Tools tab still says it is stopping:\n%s", said) + } +} + +// Closing the window during a run comes back at once and shows what it waits +// for, the window goes only once the run has stopped, a second press of the +// close button in the meantime does nothing, and the folder is written down as +// a close at rest writes it. +func TestClosingDuringARunKeepsTheWindowDrawingUntilTheRunHasStopped(t *testing.T) { + host, content, hold := heldScreen(t) + host.letGo = make(chan struct{}) + dir := t.TempDir() + fill(t, content, text.FieldOutputDir(), dir) + fill(t, content, text.FieldSize(), "1kb") + fill(t, content, text.FieldCount(), "3") + press(t, content, text.ButtonGenerate()) + + hold.look(func() { + if host.intercept == nil { + t.Fatal("the window has no close intercept, so closing it does not reach the run at all") + } + if !comesBackWhileHeld(hold, host.intercept) { + t.Fatal("closing the window waited for the run on the interface thread, so the window stops drawing while it waits") + } + // The window is let go from a goroutine of its own, so "not yet" is + // asked over a while rather than read once. A wait that is right + // never ends while the run is held, and one that does not wait at all + // lets the window go in microseconds. + select { + case <-host.letGo: + t.Fatal("the window was let go while the run was still going, so the process can end in the middle of a file") + case <-time.After(500 * time.Millisecond): + } + if said := textIn(host.content); !strings.Contains(said, text.SubtitleClosing()) { + t.Errorf("the closing window does not say what it waits for. It shows:\n%s", said) + } + notice := host.content + if !comesBackWhileHeld(hold, host.intercept) { + t.Fatal("a second press of the close button waited for the run") + } + if host.content != notice { + t.Error("a second press of the close button began a second wait, so the window would be let go twice") + } + }) + join(host) + + if host.closed != 1 { + t.Errorf("the window was let go %d time(s) and it should be let go exactly once", host.closed) + } + if got := host.Remembered().Directory(); got != dir { + t.Errorf("the folder written down on the way out is %q, not %q", got, dir) + } + if info, err := os.Stat(filepath.Join(dir, "manifest.json")); err != nil || info.Size() == 0 { + t.Errorf("the run that the window waited for left no manifest (%v)", err) + } +} + +// No key starts work while the window waits to close. +// +// A screen whose work has stopped, or never started, has a live Generate - and +// the shortcuts are on the canvas, not on the screens the notice replaced. A +// run started there after the wait had passed that screen would be ended in +// the middle of a file when the window goes. Asked on a screen that is idle +// while another one winds down, which is where a person who closed the window +// and pressed Ctrl+Enter out of habit would be. +func TestNoKeyStartsWorkWhileTheWindowWaitsToClose(t *testing.T) { + host, content, hold := heldScreen(t) + fill(t, content, text.FieldOutputDir(), t.TempDir()) + fill(t, content, text.FieldSize(), "1kb") + fill(t, content, text.FieldCount(), "3") + press(t, content, text.ButtonGenerate()) + + hold.look(func() { + presets := selectTab(t, host.content, text.TabPresets()) + box := entryUnder(t, presets, text.FieldOutputDir()) + if live := buttonNamed(presets, text.ButtonGenerate()); live == nil || live.Disabled() || !live.Visible() { + t.Fatal("the preset screen has no live Generate, so a key that reached it would start nothing and this guard asks about nothing") + } + if !comesBackWhileHeld(hold, host.intercept) { + t.Fatal("closing the window waited for the run on the interface thread") + } + before := host.settles + pressInABox(t, box, fyne.KeyReturn, fyne.KeyModifierControl) + if host.settles != before { + t.Error("Ctrl+Enter read the preset screen's form while the window waited to close, so a run can start that the window will end in the middle of a file") + } + }) +} diff --git a/internal/guard/window_test.go b/internal/guard/window_test.go index dbb0b831..b1eed012 100644 --- a/internal/guard/window_test.go +++ b/internal/guard/window_test.go @@ -76,8 +76,11 @@ type fakeHost struct { // cancelling the preview it wanted to read. waitForWork func() closed int - picked string - asked int + // letGo is closed the first time the window is let go, when a guard made + // it - see Close, in stopping_test.go. + letGo chan struct{} + picked string + asked int // pickedFile is what ChooseFile answers, askedFile how often it was asked, // and copied the last text Copy was given - the things only the Tools tab // asks of a window. Counted apart from asked, so a guard about the @@ -274,7 +277,6 @@ func (h *fakeHost) holdTheClock() *heldClock { // SetWaitForWork is the optional interface the window offers rather than // requires - a real window has no use for it and does not implement it. 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 diff --git a/internal/gui/text/locale/en.json b/internal/gui/text/locale/en.json index 2d569a2b..3538abda 100644 --- a/internal/gui/text/locale/en.json +++ b/internal/gui/text/locale/en.json @@ -441,6 +441,11 @@ "one": "1 format", "other": "{{.Count}} formats" }, + "HeadingClosing": { + "description": "The heading at the top of a screen.", + "hash": "sha256-f79ebae4aae3", + "other": "Closing" + }, "HintBasePreset": { "description": "The line under a field name, saying what the field does.", "hash": "sha256-d75c8b442fd9", @@ -986,6 +991,16 @@ "hash": "sha256-f08278015581", "other": "Stopped after {{.Files}}. The manifest describes exactly those." }, + "Stopping": { + "description": "Shown in the window.", + "hash": "sha256-823789a4f4b7", + "other": "Stopping - the step in progress is finished first, so nothing is left half written." + }, + "SubtitleClosing": { + "description": "The sentence under the heading at the top of a screen, saying what the screen is for.", + "hash": "sha256-e623c2ae70c3", + "other": "The window closes once the work in progress has stopped. The step in progress is finished first, so nothing is left half written." + }, "SubtitleGenerate": { "description": "The sentence under the heading at the top of a screen, saying what the screen is for.", "hash": "sha256-e7dcd4001e28", diff --git a/internal/gui/text/locale/pl.json b/internal/gui/text/locale/pl.json index 8609d52a..7edb2fed 100644 --- a/internal/gui/text/locale/pl.json +++ b/internal/gui/text/locale/pl.json @@ -87,6 +87,7 @@ "FieldTool": { "hash": "sha256-2e53bdcd0740", "other": "Narzędzie" }, "Files": { "hash": "sha256-70212ca62bcd", "one": "1 plik", "few": "{{.Count}} pliki", "many": "{{.Count}} plików", "other": "{{.Count}} pliku" }, "FormatCount": { "hash": "sha256-12de0241f4a5", "one": "1 format", "few": "{{.Count}} formaty", "many": "{{.Count}} formatów", "other": "{{.Count}} formatu" }, + "HeadingClosing": { "hash": "sha256-f79ebae4aae3", "other": "Zamykanie" }, "HintBasePreset": { "hash": "sha256-d75c8b442fd9", "other": "Jego pliki idą pierwsze." }, "HintBoundary": { "hash": "sha256-3ef02abcdddf", "other": "Trzy pliki: bajt poniżej limitu, dokładnie na nim i bajt powyżej." }, "HintDamage": { "hash": "sha256-67e5c4c4940a", "other": "Celowo psuje pliki." }, @@ -196,6 +197,8 @@ "SizeWayExact": { "hash": "sha256-225cdeb34c4b", "other": "Jeden rozmiar" }, "SizeWayRange": { "hash": "sha256-8879adb25c7d", "other": "Zakres" }, "StoppedAfter": { "hash": "sha256-f08278015581", "other": "Zatrzymano. Zapisano: {{.Files}}. Manifest opisuje dokładnie te pliki." }, + "Stopping": { "hash": "sha256-823789a4f4b7", "other": "Zatrzymuję - najpierw kończy się bieżący krok, żeby nic nie zostało zapisane do połowy." }, + "SubtitleClosing": { "hash": "sha256-e623c2ae70c3", "other": "Okno zamknie się, gdy praca w toku się zatrzyma. Najpierw kończy się bieżący krok, żeby nic nie zostało zapisane do połowy." }, "SubtitleGenerate": { "hash": "sha256-e7dcd4001e28", "other": "Pliki jednego formatu i jednego rozmiaru, z manifestem, który mówi, jak system pod testem powinien na nie zareagować." }, "SubtitlePreferences": { "hash": "sha256-836103012003", "other": "Jak to okno mówi, ile pamięci mogą wziąć jego przebiegi i co pamięta między przebiegami. Nic tutaj nie zmienia plików, które tworzy." }, "SubtitlePreset": { "hash": "sha256-7962df9b6a39", "other": "Gotowe zestawy plików, każdy zbudowany, by odpowiedzieć na jedno pytanie o system pod testem." }, diff --git a/internal/gui/text/text.go b/internal/gui/text/text.go index d41fb553..111d2d61 100644 --- a/internal/gui/text/text.go +++ b/internal/gui/text/text.go @@ -463,6 +463,26 @@ func InstructionsNotSaved(path, cause string) string { // NothingProduced is the outcome when a run ended with no manifest at all. func NothingProduced() string { return say("NothingProduced", "Nothing was produced.") } +// Stopping is said from the press of Cancel until the work has stopped. The +// work stops between steps - a write, a picture, a block read - so the step in +// hand is finished first, and on a slow or a networked disk that is what the +// screen waits for. A file cut off part way is removed, which is the second +// half of the sentence. +func Stopping() string { + return say("Stopping", "Stopping - the step in progress is finished first, so nothing is left half written.") +} + +// HeadingClosing is what the window shows in place of its screens once it has +// been closed during work, until the work has stopped and the window goes +// (docs/GUI.md G7). +func HeadingClosing() string { return say("HeadingClosing", "Closing") } + +// SubtitleClosing says under it what the window is waiting for. +func SubtitleClosing() string { + return say("SubtitleClosing", + "The window closes once the work in progress has stopped. The step in progress is finished first, so nothing is left half written.") +} + // StoppedAfter is the outcome of a run that was cancelled or failed part way. func StoppedAfter(written int) string { return sayf("StoppedAfter", "Stopped after {{.Files}}. The manifest describes exactly those.", diff --git a/internal/gui/window/closing.go b/internal/gui/window/closing.go new file mode 100644 index 00000000..920b3100 --- /dev/null +++ b/internal/gui/window/closing.go @@ -0,0 +1,122 @@ +package window + +import ( + "fyne.io/fyne/v2" + + "github.com/donislawdev/TestingFilesGenerator/internal/gui/parts" + "github.com/donislawdev/TestingFilesGenerator/internal/gui/text" +) + +// Closing the window, which during work is a cancellation and not a kill - G7. +// +// The invariant that the output directory never holds a half written file +// rests on the signal handler in cmd/tfg, and closing a window is not a signal. +// So the window asks every screen to stop and waits for them before it lets +// go, and that has not changed. What changed on 2026-11-03 (R06 in +// docs/STABILITY-REVIEW-2026-10-26.md) is where it waits: on a goroutine of +// its own rather than on the interface thread, with the window saying what it +// is waiting for. Waiting on the interface thread was a window that stopped +// drawing for as long as the disk held on to a write, and Windows offers to end +// a window that has not drawn for five seconds - which is ending the process in +// the middle of a file, the one thing the wait is there to prevent. There is no +// way to close without waiting (the owner's decision of 2026-11-03): leaving +// half a file and no manifest behind is the defect, not a faster way out. + +// closing is the window's way out, taken once. +type closing struct { + host Host + // screens are asked to stop and waited for - every one rather than the one + // on show, because a run keeps going while somebody moves to another + // screen, so the busy one is not necessarily the visible one. Stop does + // nothing on a screen that is idle. + screens []interface{ Stop() } + // busy is the state of the same screens, asked whether there is anything + // to wait for at all. + busy []*busy + // quiet is the window's wait for quiet (tidy.go), called off with the rest. + quiet interface{ Stop() } + + // working and showing are where the directory to write down comes from: + // the screen somebody was last on. The three carry it between themselves, + // so the screen being looked at is the one holding the answer, and showing + // is a pointer because it changes as somebody moves. + working map[string]interface { + OutDir() string + SetOutDir(string) + } + showing *string + // looking is the tab the keyboard acts on - see wireKeyboard. + looking *string + + // begun is whether the way out has been taken, so a second press of the + // close button while the window waits does nothing. + begun bool + // gone is closed once the window has been let go after a wait, for + // Settled. Nil until a wait begins. + gone chan struct{} +} + +// tabClosing is what is looked at while the window waits to close: the notice, +// which no key acts on. +const tabClosing = "closing" + +// closeCleanly puts the way out on the window's close button, for the window +// it was made for. +// +// Handed back as well, because closing the window from inside it - Restart now +// on the Preferences screen - has to go this same way. The toolkit's Close does +// not pass through the intercept (driver/glfw window.go, read on 2026-09-29), +// so calling it alone would skip the runs and the folder. +func closeCleanly(h Host, c *closing) *closing { + c.host = h + h.SetCloseIntercept(c.leave) + return c +} + +// leave writes down where the files were going, and lets the window go - at +// once when nothing is working, and once the work has stopped when something +// is. +// +// The directory is written down first, from the screen somebody was on when +// they closed: one moment rather than "whenever it changes", so what comes back +// is describable in a sentence - the window opens where you left it. Writing on +// every keystroke would also put a half typed path on somebody's disk. +// +// While it waits, the window shows a notice in place of the screens and the +// keyboard acts on nothing. Both for the same reason: a screen whose work has +// stopped while another is still winding down is a screen whose Generate works +// again, and a run started there after the wait had passed it would be ended +// in the middle of a file when the window goes. +func (c *closing) leave() { + if c.begun { + return + } + c.begun = true + c.quiet.Stop() + if screen, ok := c.working[*c.showing]; ok { + c.host.Remembered().RememberDirectory(screen.OutDir()) + } + if !anyBusy(c.busy) { + c.host.Close() + return + } + *c.looking = tabClosing + c.host.SetContent(parts.Screen(parts.Titled(text.HeadingClosing(), text.SubtitleClosing()))) + gone := make(chan struct{}) + c.gone = gone + go func() { + for _, screen := range c.screens { + screen.Stop() + } + fyne.Do(func() { c.host.Close() }) + close(gone) + }() +} + +// Settled waits for a window that is closing to have closed, for a guard - the +// same seam as runner.Settled. Returns at once when no wait has begun. +func (c *closing) Settled() { + if c.gone != nil { + <-c.gone + } +} diff --git a/internal/gui/window/open.go b/internal/gui/window/open.go index 350d93d2..13b1abb8 100644 --- a/internal/gui/window/open.go +++ b/internal/gui/window/open.go @@ -43,8 +43,8 @@ func Open(h Host) fyne.Size { busyScreens := []*busy{gen.runner.busy, pre.runner.busy, rec.runner.busy, tools.busy} // The way out is wired below, once everything it stops exists. The screen // is built now, so it is handed a way to reach it rather than the thing. - var leave func() - prefs := NewPreferences(h, func() { leave() }, func() bool { return anyBusy(busyScreens) }) + var out *closing + prefs := NewPreferences(h, func() { out.leave() }, func() bool { return anyBusy(busyScreens) }) // Tabs across the top rather than buttons at the foot, reported from use on // 2026-08-11. The way between the screens used to sit in the row of actions @@ -135,15 +135,12 @@ func Open(h Host) fyne.Size { focusFirst(item.ID, byKeyboard) } - // Closing the window during a run is a cancellation and not a kill, G7. The - // invariant that the output directory never holds a half written file rests - // on the signal handler in cmd/tfg, and closing a window is not a signal - - // so without this the run would carry on with nobody watching it, or die in - // the middle of a file. + // Closing the window during a run is a cancellation and not a kill, G7 - + // see closing.go. // One wait for quiet for the whole window, told by every screen, and // stopped with them when the window closes - see tidy.go. - quiet := tidyWhenLeftAlone(h, busyScreens, gen.runner, pre.runner, rec.runner) - leave = closeCleanly(h, []interface{ Stop() }{gen, pre, rec, tools, quiet}, working, &showing) + out = closeCleanly(h, &closing{screens: []interface{ Stop() }{gen, pre, rec, tools}, busy: busyScreens, working: working, + showing: &showing, looking: &looking, quiet: tidyWhenLeftAlone(h, busyScreens, gen.runner, pre.runner, rec.runner)}) // Restart now stands down while any screen is making files, and stands up // again when it stops - told rather than asked, so the button is right // the moment a run ends while the Preferences tab is on show. The Tools tab @@ -151,8 +148,8 @@ func Open(h Host) fyne.Size { for _, b := range busyScreens { b.changed = prefs.BusyChanged } - offerSettling(h, []interface{ Settled() }{gen, pre, rec, tools}) - offerHolding(h, []interface{ HoldBeforeFinishing(func()) }{gen, pre, rec}) + offerSettling(h, []interface{ Settled() }{gen, pre, rec, tools, out}) + offerHolding(h, []interface{ HoldBeforeFinishing(func()) }{gen, pre, rec, tools}) wireKeyboard(h, keyed, &looking) @@ -171,48 +168,6 @@ func Open(h Host) fyne.Size { return firstOpening(tabbed, gen) } -// closeCleanly stops whatever is running, writes down where the files were -// going, and then lets the window go. -// -// It asks every screen to stop rather than the one on show, and that is the part -// worth writing down: a run keeps going while somebody moves to another screen, -// so the busy one is not necessarily the visible one. Stop does nothing on a -// screen that is idle. -// -// The directory is taken from the screen somebody was last on rather than from a -// fixed one - the three carry it between themselves, so the screen being looked -// at is the one holding the answer. showing is a pointer for that reason: it -// changes as somebody moves, and what matters is where they were when they shut -// the window. -// -// One moment rather than "whenever it changes", so what comes back is -// describable in a sentence: the window opens where you left it. Writing on -// every keystroke would also put a half typed path on somebody's disk. -// -// Split out of Open on 2026-08-25, when that function went past three quarters -// of the ceiling. The ceiling is a ratchet, so the answer is a split and never a -// higher number. -func closeCleanly(h Host, running []interface{ Stop() }, working map[string]interface { - OutDir() string - SetOutDir(string) -}, showing *string) (leave func()) { - leave = func() { - for _, screen := range running { - screen.Stop() - } - if screen, ok := working[*showing]; ok { - h.Remembered().RememberDirectory(screen.OutDir()) - } - h.Close() - } - h.SetCloseIntercept(leave) - // Handed back as well, because closing the window from inside it - Restart - // now on the Preferences screen - has to go this same way. The toolkit's - // Close does not pass through the intercept (driver/glfw window.go, read - // on 2026-09-29), so calling it alone would skip the runs and the folder. - return leave -} - // offerWhereItLastWrote puts the directory of the last run on every screen. // // Every screen rather than the one that opens first: the three carry the diff --git a/internal/gui/window/run.go b/internal/gui/window/run.go index 05d040ce..ca289caa 100644 --- a/internal/gui/window/run.go +++ b/internal/gui/window/run.go @@ -1,7 +1,6 @@ package window import ( - "context" "errors" "path/filepath" "sort" @@ -19,14 +18,15 @@ import ( "github.com/donislawdev/TestingFilesGenerator/internal/gui/text" ) -// The one place in the window where work happens off the interface thread, and -// the only file of this tree in the concurrency guard's list. +// Where a run and a preview happen off the interface thread, one of the files +// in the concurrency guard's list. // -// It is one goroutine and one channel. The goroutine exists because engine.Run -// writes files and a window that waits for it is a window the desktop reports -// as not responding. The channel exists because closing the window has to wait -// for that goroutine to wind down - cancelling and exiting immediately would -// end the process somewhere inside a file, which is the invariant G7 is about. +// It is one goroutine per piece of work, and the work's channel (work.go). The +// goroutine exists because engine.Run writes files and a window that waits for +// it is a window the desktop reports as not responding. The channel exists +// because closing the window has to wait for that goroutine to wind down - +// cancelling and exiting immediately would end the process somewhere inside a +// file, which is the invariant G7 is about. // // Everything the interface is told from in here crosses back through fyne.Do. // Measured on 2026-08-05 and worth writing down, because it decides what a test @@ -114,45 +114,28 @@ type runner struct { // they added it. Fields.Add is the only way to build a field now. fields *parts.Fields - // hold, when a guard supplies one, is run on the worker just before the - // screen is told the work has ended. Nil in the shipped program - see - // HoldBeforeFinishing for what it is for and why nothing else can do it. - hold func() - - // stop ends the run in progress and waits for the worker to finish, and is - // only ever touched on the interface thread. + // work is the last run or preview this screen started, and the way to end + // it or wait for it - see work.go. Only ever touched on the interface + // thread, and set before the worker starts. // - // "Waits for the worker" is as far as it reaches, and the limit is worth - // writing down because G7 gets read out of this sentence. The worker closes - // the channel this waits on immediately after handing its last piece of - // work to fyne.Do, and fyne.Do QUEUES that work rather than running it - - // DoAndWait there would be the interface thread waiting on a worker that is - // waiting on the interface thread. So when stop returns, the worker is - // finished and the widget writes it asked for may still be in the toolkit's - // queue. + // "Waits for the worker" is as far as stopping reaches, and the limit is + // worth writing down because G7 gets read out of this sentence. The worker + // closes the channel stopping waits on immediately after handing its last + // piece of work to fyne.Do, and fyne.Do QUEUES that work rather than + // running it - DoAndWait there would be the interface thread waiting on a + // worker that is waiting on the interface thread. So when Stop returns, the + // worker is finished and the widget writes it asked for may still be in the + // toolkit's queue. // // What that buys is what G7 asks for: nothing of ours is still running and // nothing more is going to the disk. What it does not buy is the screen // having caught up - and Settled does not buy that either, because it waits // on the same channel. // - // It is deliberately left in place once a run has ended rather than - // cleared, and that is a threading decision rather than an oversight. - // Clearing it would be a write from the worker under the toolkit's test - // driver, where fyne.Do runs on the calling goroutine - so -race would - // report a race that production does not have, and the honest way out is a - // handle that is safe to hold on to. Calling it after the run has ended - // cancels a finished context and receives from a closed channel, both of - // which return at once. - // - // So it says nothing about whether a run is going. It used to be asked - // that, and the answer was wrong from the second run onwards - see running. - stop func() - // settled is closed when the work in flight has finished, and is what - // Settled waits on. Separate from stop because stop CANCELS first, which is - // right for closing the window and for Esc and wrong for anybody who wants - // to read the answer. - settled chan struct{} + // It says nothing about whether a run is going, because it is kept once + // the work has ended. It used to be asked that, and the answer was wrong + // from the second run onwards - see busy.occupied. + work *work // scroll is the part of this screen that moves, so a refusal can bring the // box it is about into view. Set by the screen, because only the screen @@ -463,29 +446,23 @@ func (r *runner) onPreview() { r.busy.set(true, busyFace{stoppable: true}) r.say(text.WorkingOutTheCost()) - ctx, cancel := context.WithCancel(context.Background()) - done := make(chan struct{}) - r.settled = done - // Waited for on the way out, even though a preview writes nothing and there // would be no half written file to protect. What it does do is touch // widgets when it comes back, so a preview still in flight after the window // has gone is a worker drawing into a screen that is being torn down. // // Set before the goroutine starts, for the reason startRun spells out. - r.stop = func() { - cancel() - <-done - } + ctx, w := begin() + r.work = w go func() { planned, planErr := engine.PlanContext(ctx, targets, opt) if planErr != nil { // Do rather than DoAndWait, for the same reason startRun gives: the // interface thread must never be left waiting on a worker. - r.holdBeforeFinishing() - fyne.Do(func() { r.previewFinished(nil, nil, opt, diskRoom{}, planErr) }) - close(done) + r.busy.holdBeforeFinishing() + fyne.Do(func() { w.end(func() { r.previewFinished(nil, nil, opt, diskRoom{}, planErr) }) }) + close(w.done) return } res, runErr := engine.Run(ctx, planned, opt) @@ -494,9 +471,9 @@ func (r *runner) onPreview() { // share takes, and the interface thread is the one thing that must // not wait for it. It used to be asked after crossing back. room := roomOn(opt.OutDir) - r.holdBeforeFinishing() - fyne.Do(func() { r.previewFinished(res, planned, opt, room, runErr) }) - close(done) + r.busy.holdBeforeFinishing() + fyne.Do(func() { w.end(func() { r.previewFinished(res, planned, opt, room, runErr) }) }) + close(w.done) }() } @@ -605,9 +582,13 @@ func (r *runner) onGenerate() { func (r *runner) startRun(targets []engine.Target, opt engine.Options) { opt.Memory, opt.MachineMemory = memoryOf(r.kept()), r.machine - ctx, cancel := context.WithCancel(context.Background()) - done := make(chan struct{}) - r.settled = done + // Set BEFORE the goroutine starts, and that ordering is the whole point. + // The other way round there is a window - short, and real - where files are + // being written and the screen holds no handle on them, so closing the + // window in it finds nothing to stop, waits for nothing, and ends the + // process somewhere inside a file. + ctx, w := begin() + r.work = w started := time.Now() limit := &throttle{} @@ -626,40 +607,27 @@ func (r *runner) startRun(targets []engine.Target, opt engine.Options) { } elapsed := time.Since(started) fyne.Do(func() { - r.busy.bar.SetValue(float64(p.Percent())) - r.status.SetText(progressText(p, elapsed)) + w.news(func() { + r.busy.bar.SetValue(float64(p.Percent())) + r.status.SetText(progressText(p, elapsed)) + }) }) } - // Cancelling and waiting, in that order, is the whole of G7. Closing the - // window is not a signal, so nothing else brings the run to a stop - and - // ending the process without waiting would leave it somewhere inside a file - // with no manifest, which is the one thing the output directory is promised - // never to hold. - // - // Set BEFORE the goroutine starts, and that ordering is the whole point. - // The other way round there is a window - short, and real - where files are - // being written and this is still nil, so closing the window in it finds - // nothing to call, waits for nothing, and ends the process somewhere inside - // a file. ctx, cancel and done all exist already, so there is nothing to - // gain by waiting. - r.stop = func() { - cancel() - <-done - } - go func() { planned, planErr := engine.PlanContext(ctx, targets, opt) if planErr != nil { // A refusal from planning leaves nothing started and nothing on the // disk, so it goes back as a refusal rather than as the end of a // run - runFinished would talk about files that never existed. - r.holdBeforeFinishing() + r.busy.holdBeforeFinishing() fyne.Do(func() { - r.busy.set(false, busyFace{}) - r.refuse(planErr) + w.end(func() { + r.busy.set(false, busyFace{}) + r.refuse(planErr) + }) }) - close(done) + close(w.done) return } // Nothing is said from here, and that is deliberate rather than an @@ -679,19 +647,20 @@ func (r *runner) startRun(targets []engine.Target, opt engine.Options) { // The room left on the disk is the room left AFTER the files, which // is not the number a preview measured before them. room := roomOn(opt.OutDir) - // Do rather than DoAndWait. The interface thread may already be inside - // stop, waiting on the channel closed below, and a worker waiting for - // that thread to run something would be both of them waiting. - r.holdBeforeFinishing() - fyne.Do(func() { r.runFinished(res, runErr, saveErr, room, saved) }) - close(done) + // Do rather than DoAndWait. Whoever waits on the channel closed below + // must find the last word queued, and a worker waiting for the + // interface thread to run something would make any wait for the worker + // on that thread a wait on itself. + r.busy.holdBeforeFinishing() + fyne.Do(func() { w.end(func() { r.runFinished(res, runErr, saveErr, room, saved) }) }) + close(w.done) }() } // runFinished is the end of a run, back on the interface thread. // -// Note what it does not do: clear stop. That is deliberate and the reason is at -// the declaration of the field. +// Note what it does not do: clear the work. That is deliberate and the reason +// is at the declaration of the field. // // saved is where the record went, with nothing in it when no record was // written - which is a refused run, a preview, and a run whose manifest could @@ -761,16 +730,18 @@ func (r *runner) keepScroll(scroll *container.Scroll) *container.Scroll { return scroll } -func (r *runner) onCancel() { r.Stop() } +// onCancel asks the run or the preview to stop and does not wait for it - the +// screen goes on drawing and says what it is waiting for, and the worker's +// last word ("Stopped after ...") ends it, as it ends a run that was not +// stopped. Waiting here was the window not drawing for as long as the disk +// took over the write in hand (R06). +func (r *runner) onCancel() { r.busy.halt(r.work, r.status) } // Stop ends a run in progress and waits for it, or does nothing when there is -// none. Safe to call at any time, which is what lets one close intercept ask -// every screen without knowing which of them is busy. -func (r *runner) Stop() { - if r.stop != nil { - r.stop() - } -} +// none. Safe to call at any time, which is what lets closing the window ask +// every screen without knowing which of them is busy. It waits, so it is called +// off the interface thread - see closing.go. +func (r *runner) Stop() { r.work.stop() } // HoldBeforeFinishing sets a hold the worker passes through on its way to // reporting, so that a guard can read the screen WHILE a run is going. @@ -796,15 +767,7 @@ func (r *runner) Stop() { // orders that read before the worker's later writes. There is then no // concurrent access left to report, which is why this restores the assertions // instead of hiding them. -func (r *runner) HoldBeforeFinishing(fn func()) { r.hold = fn } - -// holdBeforeFinishing runs the hold, if a guard supplied one. Called on the -// worker, before anything on the screen is touched. -func (r *runner) holdBeforeFinishing() { - if r.hold != nil { - r.hold() - } -} +func (r *runner) HoldBeforeFinishing(fn func()) { r.busy.hold = fn } // Settled waits for work in flight to finish, without stopping it. // @@ -819,17 +782,13 @@ func (r *runner) holdBeforeFinishing() { // closing really does cancel, which is the point of the change, and a guard // that closed the window to read the preview would be cancelling the preview. // -// It waits on the same channel stop does and carries the same limit: the worker +// It waits on the same channel Stop does and carries the same limit: the worker // is finished and its last fyne.Do may still be queued. Under the test driver // that distinction disappears, because there fyne.Do runs on the calling // goroutine, so the callback has already run by the time the channel closes - // which is why a guard may read the screen the moment this returns, and why the // limit is a production one rather than a test one. -func (r *runner) Settled() { - if r.settled != nil { - <-r.settled - } -} +func (r *runner) Settled() { r.work.settled() } // wholeNumber reads a plain count out of a box. // diff --git a/internal/gui/window/runbusy.go b/internal/gui/window/runbusy.go index 6840ac9b..2e9d0b38 100644 --- a/internal/gui/window/runbusy.go +++ b/internal/gui/window/runbusy.go @@ -4,8 +4,10 @@ import ( "time" "fyne.io/fyne/v2" + "fyne.io/fyne/v2/widget" "github.com/donislawdev/TestingFilesGenerator/internal/gui/parts" + "github.com/donislawdev/TestingFilesGenerator/internal/gui/text" ) // The screen while a run or a preview owns it, and the face it wears for that. @@ -48,9 +50,10 @@ type later func(after time.Duration, then func()) (callOff func()) // off at once when it ends. type busy struct { // occupied is the state: whether work owns the screen. It exists because - // the runner's stop cannot answer that - stop is set on the first press - // and never cleared. Only ever touched on the interface thread, which is - // what makes a plain bool enough. + // the screen's work cannot answer that - it is set on the first press and + // never cleared. Only ever touched on the interface thread, which is what + // makes a plain bool enough. It stays on while the work winds down after + // Cancel, so nothing new starts until the worker has said its last word. occupied bool // worn is whether the face is on, so it is taken off exactly when it // was put on and never twice. @@ -86,6 +89,12 @@ type busy struct { // still runs - after the next work has started, if the next press comes // in that gap. An outside review of the pull request named it. epoch int + + // hold, when a guard supplies one, is run on the worker just before the + // screen is told the work has ended. Nil in the shipped program - see + // runner.HoldBeforeFinishing for what it is for. Here rather than on the + // screens since 2026-11-03, when the Tools tab came to need it as well. + hold func() } // busyFace is which parts of the face a piece of work earns: whether there @@ -175,6 +184,30 @@ func (b *busy) undress() { b.relay() } +// halt asks the work wearing this face to stop, and turns the face to +// stopping - Cancel and Escape on every screen that does work. +// +// Cancel stays where it is, switched off, and the line says what is waited +// for. Off rather than hidden, so the row does not move under the pointer that +// has just pressed it and a second press has nothing to land on. The face of +// the next work switches it on again. The rest of the face stays on until the +// worker's last word takes it off: the work still owns the screen, and a +// second run must not start while the first is winding down. +func (b *busy) halt(w *work, status *widget.Label) { + w.halt(func() { + b.cancel.Disable() + showOn(status, text.Stopping()) + }) +} + +// holdBeforeFinishing runs the hold, if a guard supplied one. Called on the +// worker, before anything on the screen is touched. +func (b *busy) holdBeforeFinishing() { + if b.hold != nil { + b.hold() + } +} + // relay lays the row of buttons out again, because the toolkit does not. // // Measured in the pinned toolkit on 2026-09-21, from the owner's report that diff --git a/internal/gui/window/tools.go b/internal/gui/window/tools.go index 113aff86..88ea5218 100644 --- a/internal/gui/window/tools.go +++ b/internal/gui/window/tools.go @@ -46,7 +46,7 @@ type Tools struct { busy *busy status *widget.Label fault *parts.ErrorArea - job *toolJob + job *work view toolsView offer folderOffer @@ -141,7 +141,7 @@ func NewTools(host Host) *Tools { t.result = parts.FieldColumn() run := parts.NewButton(parts.Primary, text.ButtonRunTool(), t.PressGenerate).InTheBar() - cancel := parts.NewButton(parts.Secondary, text.ButtonCancel(), t.PressCancel).InTheBar() + cancel := parts.NewButton(parts.Secondary, text.ButtonCancel(), t.onCancel).InTheBar() cancel.Disable() cancel.Hide() t.offer.btn = parts.NewButton(parts.Secondary, text.ButtonOpenFolder(), func() { @@ -198,23 +198,27 @@ func (t *Tools) Fields() *parts.Fields { return t.fields } // asks every screen the same four things. func (t *Tools) PressPreview() {} -// PressCancel stops the tool that is running, if one is. -func (t *Tools) PressCancel() { t.Stop() } +// PressCancel presses Cancel, which is there only while a tool is running - +// see pressIfLive. Until 2026-11-03 Escape stopped the tool itself, so it +// reached a tool in the moment before the screen showed it busy, which no +// other screen allowed. +func (t *Tools) PressCancel() { pressIfLive(t.busy.cancel) } + +// onCancel asks the tool to stop and does not wait for it, for the reason +// runner.onCancel gives. +func (t *Tools) onCancel() { t.busy.halt(t.job, t.status) } // Stop ends the tool that is running and waits for it, or does nothing. Safe // to call at any time, which is what lets closing the window ask every screen. -func (t *Tools) Stop() { - if t.job != nil { - t.job.stop() - } -} +// It waits, so it is called off the interface thread - see closing.go. +func (t *Tools) Stop() { t.job.stop() } // Settled waits for the tool that is running to finish, for a guard. -func (t *Tools) Settled() { - if t.job != nil { - <-t.job.done - } -} +func (t *Tools) Settled() { t.job.settled() } + +// HoldBeforeFinishing sets a hold the tool passes through on its way to +// reporting, for a guard - see runner.HoldBeforeFinishing. +func (t *Tools) HoldBeforeFinishing(fn func()) { t.busy.hold = fn } // onChosen draws what the chosen tool works on and what it takes. func (t *Tools) onChosen(choice string) { @@ -272,22 +276,28 @@ func (t *Tools) PressGenerate() { } // progressed and finished are what a running tool tells the screen, on the -// interface thread. -func (t *Tools) progressed(done, total int64) { - t.busy.bar.SetValue(float64(core.Percent(done, total))) - showOn(t.status, text.ToolReading(text.HumanBytes(done), text.HumanBytes(total))) +// interface thread and through the work they are about - see work.go. The +// work is handed over rather than read off the screen, because the screen is +// given it only once startTool returns, after the worker has started. +func (t *Tools) progressed(job *work, done, total int64) { + job.news(func() { + t.busy.bar.SetValue(float64(core.Percent(done, total))) + showOn(t.status, text.ToolReading(text.HumanBytes(done), text.HumanBytes(total))) + }) } -func (t *Tools) finished(d tool.Descriptor, r tool.Result, err error) { - t.busy.set(false, busyFace{}) - showOn(t.status, "") - if err != nil { - t.offer.withdraw(t.busy.relay) - t.refuse(err) - return - } - t.showResult(d, &r) - t.offer.show(t.busy.relay) +func (t *Tools) finished(job *work, d tool.Descriptor, r tool.Result, err error) { + job.end(func() { + t.busy.set(false, busyFace{}) + showOn(t.status, "") + if err != nil { + t.offer.withdraw(t.busy.relay) + t.refuse(err) + return + } + t.showResult(d, &r) + t.offer.show(t.busy.relay) + }) } // refuse puts a refusal under the box it is about, or at the foot of the @@ -306,26 +316,15 @@ func (t *Tools) refuse(err error) { t.fault.Say(core.ShownText(text.Refusal(err, ""))) } -// toolJob is one run of a tool beside the window. +// startTool runs one tool beside the window and tells the screen how it went. // // Beside it because a file of gigabytes takes seconds to read and a window // that waits for it is a window the desktop calls not responding - the same -// reason a run of the engine happens beside it. The stop is set before the +// reason a run of the engine happens beside it. The work is made before the // goroutine starts, so closing the window at any moment of the run finds it // and waits for the read to wind down (G7). -type toolJob struct { - stop func() - done chan struct{} -} - -// startTool runs one tool and tells the screen how it went. -func startTool(d tool.Descriptor, in tool.Request, screen *Tools) *toolJob { - ctx, cancel := context.WithCancel(context.Background()) - job := &toolJob{done: make(chan struct{})} - job.stop = func() { - cancel() - <-job.done - } +func startTool(d tool.Descriptor, in tool.Request, screen *Tools) *work { + ctx, job := begin() screen.busy.set(true, busyFace{stoppable: true, progressing: true}) screen.busy.bar.SetValue(0) limit := &throttle{} @@ -333,12 +332,13 @@ func startTool(d tool.Descriptor, in tool.Request, screen *Tools) *toolJob { if !limit.allow(time.Now()) { return } - fyne.Do(func() { screen.progressed(done, total) }) + fyne.Do(func() { screen.progressed(job, done, total) }) } go func() { - defer cancel() + defer job.cancel() result, err := d.Start(ctx, in, progress) - fyne.Do(func() { screen.finished(d, result, err) }) + screen.busy.holdBeforeFinishing() + fyne.Do(func() { screen.finished(job, d, result, err) }) close(job.done) }() return job diff --git a/internal/gui/window/work.go b/internal/gui/window/work.go new file mode 100644 index 00000000..6cc2f188 --- /dev/null +++ b/internal/gui/window/work.go @@ -0,0 +1,114 @@ +package window + +import ( + "context" + "sync" +) + +// One piece of work beside the window - a run, a preview, a tool - and the +// three ways the window ends it or waits for it. +// +// Out of the runner and the Tools tab on 2026-11-03, when Cancel stopped +// waiting (R06 in docs/STABILITY-REVIEW-2026-10-26.md, O262). Until then each +// held a stop that cancelled and then waited, and Cancel, Escape and closing +// the window all called it on the interface thread. The engine asks its context +// between writes, so on a disk that answers the wait was milliseconds - and on +// one that holds on to a write it was as long as the disk took, with a window +// that did not draw. Windows offers to end a window that has not drawn for five +// seconds, and ending the process in the middle of a file is the one thing G7 +// is there to prevent. + +// work is what a screen keeps of the work it started. +// +// Kept after the work has ended rather than cleared, for the reason the runner +// gave its stop before this: clearing it would be a write from the worker, and +// a handle that is safe to hold on to is the honest way out. Stopping work +// that has ended cancels a context that is done and receives from a closed +// channel, and both return at once. +type work struct { + cancel context.CancelFunc + // done is closed by the worker once it has handed its last word to the + // interface thread - see end. + done chan struct{} + + // talking is held by whoever changes what the screen says about this work: + // a press of Cancel, and each piece of news the worker sends back. + // + // In the shipped program both happen on the interface thread, so it is + // never contended. It is here for the test driver, where fyne.Do runs the + // worker's news on the worker (O124) - there a press of Cancel and a line of + // progress are two goroutines writing one status line, and the race + // detector is right to say so. HoldBeforeFinishing cannot order the two, + // because the progress comes from the writers part way through a run, and + // a lock can: it keeps a guard free to press Cancel at any moment of a run. + talking sync.Mutex + // stopping is whether Cancel has been pressed. Only under talking. + stopping bool +} + +// begin starts a piece of work: the context it runs under, and its handle. +func begin() (context.Context, *work) { + ctx, cancel := context.WithCancel(context.Background()) + return ctx, &work{cancel: cancel, done: make(chan struct{})} +} + +// halt asks the work to stop and returns at once - Cancel, and Escape through +// it. +// +// say is what the screen says about it, run once under the same lock as the +// worker's news. Nothing asks whether the work has already ended: Cancel is +// hidden and switched off by the worker's last word, and Escape presses Cancel +// only while it is live, so no press reaches a finished piece of work. +func (w *work) halt(say func()) { + if w == nil { + return + } + w.talking.Lock() + defer w.talking.Unlock() + if w.stopping { + return + } + w.stopping = true + w.cancel() + say() +} + +// stop asks the work to stop and waits until it has - for closing the window, +// which does it on a goroutine of its own (see closing.go). +func (w *work) stop() { + if w == nil { + return + } + w.cancel() + <-w.done +} + +// settled waits for the work without asking it to stop - see runner.Settled. +func (w *work) settled() { + if w == nil { + return + } + <-w.done +} + +// news is something the worker says part way through - the bar and the line +// of progress - and it runs on the interface thread. Dropped once Cancel has +// been pressed: a line of progress written over "Stopping" would say the press +// did nothing. In the shipped program fyne.Do queues the news, so a line +// queued just before the press arrives just after it. +func (w *work) news(say func()) { + w.talking.Lock() + defer w.talking.Unlock() + if !w.stopping { + say() + } +} + +// end is the worker's last word, on the interface thread, said whatever was +// pressed. The worker closes done after handing this to fyne.Do and never +// before, so whoever waits on done finds the last word at least queued. +func (w *work) end(say func()) { + w.talking.Lock() + defer w.talking.Unlock() + say() +} From c106eaccc8e4b16865e203b860aec4506ea422e4 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 4 Nov 2026 00:55:15 +0100 Subject: [PATCH 3/3] guard: a run is stopped in the middle by construction, and the first screen drawn is thrown away The two guards that stop a run of four hundred files watched the folder for its first entry and stopped the run from there, a bet that the run would still be going a moment later. runner.HoldWhileWriting hands a guard the run's context at every report of progress. The engine reports under one lock, so holding the first report holds every writer, and the guards' hold lets them go when the context is done - that is, when Cancel or closing the window asked them to stop. waitUntilTheRunHasStarted has no callers left and is removed. The seam takes a context rather than its channel, so the window package declares no channel for it, and the shipped program pays a nil check per report. contextRoots names work.go begin in place of the three places the context moved out of. The first screen drawn in a process that has run other guards comes back with a tab bar of no height (O148). Two guards drew a throwaway screen first, the Tools layout guard did not, and it went red in the race job once the guards were laid out so that it drew first. The warm-up is now in renderScene, once a process, so no guard that draws a screen is without it. Co-Authored-By: Claude Opus 5.5 --- internal/guard/contextroots_test.go | 4 +- internal/guard/generatewindow_test.go | 50 ++++----------- internal/guard/screenpixels_test.go | 70 +++++++++++++-------- internal/guard/screenrefusal_test.go | 3 - internal/guard/stopping_test.go | 91 +++++++++++++++++++++++++++ internal/guard/window_test.go | 2 + internal/gui/window/open.go | 22 +++++++ internal/gui/window/run.go | 19 ++++++ internal/gui/window/runbusy.go | 14 +++++ 9 files changed, 205 insertions(+), 70 deletions(-) diff --git a/internal/guard/contextroots_test.go b/internal/guard/contextroots_test.go index e35cddfa..9fa9a69d 100644 --- a/internal/guard/contextroots_test.go +++ b/internal/guard/contextroots_test.go @@ -27,9 +27,7 @@ import ( // code inside a function does not make the list lie (pitfall 11). var contextRoots = map[string]string{ "cmd/tfg/main.go main": "the process's own context, the one the signal cancels", - "internal/gui/window/run.go runner.onPreview": "a preview starts here, and Cancel on the screen is what stops it", - "internal/gui/window/run.go runner.startRun": "a run started in the window begins here, and Cancel is what stops it", - "internal/gui/window/tools.go startTool": "a tool started in the window begins here, and its Cancel is what stops it", + "internal/gui/window/work.go begin": "a preview, a run or a tool started in the window begins here, and Cancel or closing the window is what stops it", "internal/oracle/oracle.go Checker.Check": "the oracle runs somebody else's reader under a time limit of its own - a measuring instrument for the guards, never on the path of a run", "internal/oracle/oracle.go PictureChanges": "the same instrument, a reference reader under a time limit", "internal/oracle/oracle.go Strict": "the same instrument, a reference reader under a time limit", diff --git a/internal/guard/generatewindow_test.go b/internal/guard/generatewindow_test.go index af46f3ca..2901d31f 100644 --- a/internal/guard/generatewindow_test.go +++ b/internal/guard/generatewindow_test.go @@ -7,7 +7,6 @@ import ( "sort" "strings" "testing" - "time" "fyne.io/fyne/v2" @@ -460,21 +459,26 @@ func TestGeneratingFromTheWindowWritesTheFilesAndTheManifest(t *testing.T) { // only once the run has stopped is held by // TestClosingDuringARunKeepsTheWindowDrawingUntilTheRunHasStopped, which can // see the moment in between. +// +// The writers are held until the run is asked to stop, so the window is +// closed in the middle of the run rather than at whatever point a run of +// four hundred files has reached by then - see writersHeld. func TestClosingTheWindowDuringARunStopsItAndWaitsForIt(t *testing.T) { dir := t.TempDir() - host, content := screen(t) + host, content, held := writersHeldScreen(t) fill(t, content, text.FieldOutputDir(), dir) fill(t, content, text.FieldSize(), "64kb") fill(t, content, text.FieldCount(), "400") press(t, content, "Generate") - waitUntilTheRunHasStarted(t, dir) + held.inTheMiddle(t) if host.intercept == nil { t.Fatal("the window has no close intercept, so closing it does not reach the run at all") } host.intercept() join(host) + held.stoppedByTheGuard(t) if host.closed != 1 { t.Errorf("the window was closed %d time(s) and it should be closed exactly once", host.closed) @@ -589,16 +593,18 @@ func TestWhatIsTypedOnTheScreenIsWhatGetsWritten(t *testing.T) { // Deterministic without polling, because the run is joined after the press. // Cancel itself does not wait since 2026-11-03 (R06) - that is held by // TestCancelAnswersAtOnceAndSaysWhatItWaitsFor - so what is on the disk is -// read once the engine has wound down, not while it changes. +// read once the engine has wound down, not while it changes. The writers are +// held until the run is asked to stop, so Cancel is pressed in the middle of +// the run - see writersHeld. func TestCancelStopsTheRun(t *testing.T) { dir := t.TempDir() - host, content := screen(t) + host, content, held := writersHeldScreen(t) fill(t, content, text.FieldOutputDir(), dir) fill(t, content, text.FieldSize(), "64kb") fill(t, content, text.FieldCount(), "400") press(t, content, "Generate") - waitUntilTheRunHasStarted(t, dir) + held.inTheMiddle(t) cancel := buttonNamed(content, "Cancel") if cancel == nil { @@ -606,6 +612,7 @@ func TestCancelStopsTheRun(t *testing.T) { } cancel.OnTapped() join(host) + held.stoppedByTheGuard(t) info, err := os.Stat(filepath.Join(dir, "manifest.json")) if err != nil { @@ -777,34 +784,3 @@ func TestARunIsStoppableFromTheMomentItStarts(t *testing.T) { "What to do: assign r.work first. The work exists by then, so nothing is gained by waiting.") } } - -// waitUntilTheRunHasStarted blocks until the run has put something on the disk. -// -// A guard about interrupting a run has to interrupt a run, and since 2026-08-26 -// pressing Generate no longer means one is going: planning moved off the -// interface thread, so for a moment after the press the worker is still working -// out what to write. Cancelling in that moment is answered correctly and -// uselessly - nothing has been written, so there is nothing to record and no -// manifest, which is the rule Result.Started states. -// -// The manifest claims its name before the first file, so anything at all -// appearing here means the writing has begun. -// -// This one polls a budget on purpose, and it is the ONLY wait here that still -// should. Its siblings stopped: waiting for a manifest is now a join, because -// the worker writes the record before it closes its channel, and the budget -// there was bounding the whole run rather than the wait for a filesystem - -// four failures came of that, O111 and O126. Joining cannot work here, because -// this waits for a run to START so that it can be interrupted, and a join by -// definition returns when the work is over. -func waitUntilTheRunHasStarted(t *testing.T, dir string) { - t.Helper() - deadline := time.Now().Add(20 * time.Second) - for time.Now().Before(deadline) { - if entries, err := os.ReadDir(dir); err == nil && len(entries) > 0 { - return - } - time.Sleep(time.Millisecond) - } - t.Fatal("nothing was written within twenty seconds, so there was never a run to interrupt") -} diff --git a/internal/guard/screenpixels_test.go b/internal/guard/screenpixels_test.go index 4843c6d9..6df95508 100644 --- a/internal/guard/screenpixels_test.go +++ b/internal/guard/screenpixels_test.go @@ -11,6 +11,7 @@ import ( "path/filepath" "runtime" "strings" + "sync" "testing" "fyne.io/fyne/v2" @@ -614,32 +615,6 @@ func TestEveryScreenStillDrawsItsStoredPicture(t *testing.T) { // stops here only on the system that has a reason. t.Skip("the stored screens are not compared on macOS - the system hides scroll bars, see the comment above") } - // One screen is built and thrown away before the loop, and that is a - // measurement rather than a superstition. Under Fyne 2.8.1 the first screen - // built in a process that has already run the rest of this package comes - // back with a tab bar of no height: the tab buttons keep their correct - // widths and lose their heights, and the text inside one reports a height - // of minus twelve. Measured on 2026-08-27, when the toolkit moved off - // 2.8.0: - // - // - it follows the position and not the screen. Swapping the first two - // entries of screenScenes moves the failure to the other one. - // - it needs the rest of the package. This test on its own passes, and - // goes on passing when run five times over. - // - it is the instrument and not the window. The guirender probe draws - // the same screen correctly, and the built binary draws all four tabs - // properly - looked at, not assumed. - // - another Resize does not help, and neither does waiting 200ms after - // the Refresh. So it is not a layout pass that was skipped, and not a - // race against a worker either. - // - // This hides nothing. Every screen is still compared against its stored - // tree and its stored picture, and only the first render is paid for. - // Whether the guard can still see a real change was not taken on trust - // either - both of its mutations were run again after this line went in. - // - // What causes it inside the toolkit is NOT known, and O148 says so. - renderScene(t, screenScenes()[0]) for _, sc := range screenScenes() { t.Run(sc.name, func(t *testing.T) { got, markup := renderScene(t, sc) @@ -851,10 +826,51 @@ func differingLines(want, got string, cap int) (string, int) { return b.String(), total } +// firstScreen is the screen drawn and thrown away before the first one a +// process asks for, and that is a measurement rather than a superstition. +// Under Fyne 2.8.1 the first screen built in a process that has already run +// the rest of this package comes back with a tab bar of no height: the tab +// buttons keep their correct widths and lose their heights, and the text +// inside one reports a height of minus twelve. Measured on 2026-08-27, when +// the toolkit moved off 2.8.0: +// +// - it follows the position and not the screen. Swapping the first two +// entries of screenScenes moves the failure to the other one. +// - it needs the rest of the package. This test on its own passes, and +// goes on passing when run five times over. +// - it is the instrument and not the window. The guirender probe draws +// the same screen correctly, and the built binary draws all four tabs +// properly - looked at, not assumed. +// - another Resize does not help, and neither does waiting 200ms after +// the Refresh. So it is not a layout pass that was skipped, and not a +// race against a worker either. +// +// This hides nothing. Every screen is still compared against its stored tree +// and its stored picture, and only the first render is paid for. Whether the +// guard can still see a real change was not taken on trust either - both of +// its mutations were run again after the warm-up went in. +// +// Once a process rather than once before each loop since 2026-11-04. Three +// guards draw screens and which of them draws first depends on how the race +// job lays the guards out: the one without a warm-up of its own was first +// that day and went red, only in the tab bar of the first tool it drew. Here, +// no guard that draws a screen can be without it. +// +// What causes it inside the toolkit is NOT known, and O148 says so. +var firstScreen sync.Once + // renderScene builds the window, puts one screen into one state and returns -// both what the canvas holds and how it was built. +// both what the canvas holds and how it was built - after the throwaway +// screen above, the first time a process asks. func renderScene(t *testing.T, sc screenScene) (image.Image, string) { t.Helper() + firstScreen.Do(func() { drawScene(t, screenScenes()[0]) }) + return drawScene(t, sc) +} + +// drawScene is renderScene without the warm-up. +func drawScene(t *testing.T, sc screenScene) (image.Image, string) { + t.Helper() app := test.NewApp() // The test driver ships a deliberately garish theme so colour changes show diff --git a/internal/guard/screenrefusal_test.go b/internal/guard/screenrefusal_test.go index 2784ef2d..a8f1ae07 100644 --- a/internal/guard/screenrefusal_test.go +++ b/internal/guard/screenrefusal_test.go @@ -26,9 +26,6 @@ import ( // Six scenes fit under the ceiling. func TestEveryPictureNamedAfterARefusalShowsOne(t *testing.T) { scenes := screenScenes() - // The first render of a process lays out differently - see O148 and the - // same call in TestEveryScreenStillDrawsItsStoredPicture. - renderScene(t, scenes[0]) checked := 0 for _, sc := range scenes { if !strings.Contains(sc.name, "refused") { diff --git a/internal/guard/stopping_test.go b/internal/guard/stopping_test.go index a619830f..5181628c 100644 --- a/internal/guard/stopping_test.go +++ b/internal/guard/stopping_test.go @@ -1,9 +1,12 @@ package guard import ( + "context" "os" "path/filepath" "strings" + "sync" + "sync/atomic" "testing" "time" @@ -11,6 +14,7 @@ import ( "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/tool/checksum" ) @@ -39,6 +43,93 @@ func (h *fakeHost) Close() { } } +// writersHeld holds a run's writers at their first report of progress until +// the run is asked to stop, for a guard that stops a run and has to have +// stopped it in the middle. +// +// In place of watching the folder for its first entry and stopping the run +// from there, which bet that four hundred files would still be going a moment +// later - an outside review of the pull request named it. Held, the run cannot +// end before it is stopped, and what lets the writers go is the stopping +// itself. See runner.HoldWhileWriting. +type writersHeld struct { + // reached is buffered for the reason holdDuringRun's is. + reached chan struct{} + // abandoned lets the writers go when a guard ended without stopping the + // run, so a guard that failed early fails rather than waiting out the + // bound. + abandoned chan struct{} + // first makes it one hold: the writers queued behind it, and every later + // report, pass straight through once it has let go. + first sync.Once + // outlasted says the bound ran out with nothing asking the run to stop. + outlasted atomic.Bool +} + +// writersHeldAtMost bounds the hold, so a run that nothing stops is let go and +// the guard says so, rather than the package hanging on its join. +const writersHeldAtMost = 20 * time.Second + +// enter is what a writer calls, with the run's context. +func (p *writersHeld) enter(ctx context.Context) { + p.first.Do(func() { + p.reached <- struct{}{} + select { + case <-ctx.Done(): + case <-p.abandoned: + case <-time.After(writersHeldAtMost): + p.outlasted.Store(true) + } + }) +} + +// HoldWhileWriting is the optional interface the window asks for that hold +// through. Nil unless a guard asked for one with writersHeldScreen, so every +// other guard runs exactly as before. +func (h *fakeHost) HoldWhileWriting() func(context.Context) { + if h.writers == nil { + return nil + } + return h.writers.enter +} + +// writersHeldScreen is the one target screen with the writers held. +func writersHeldScreen(t *testing.T) (*fakeHost, fyne.CanvasObject, *writersHeld) { + t.Helper() + held := &writersHeld{reached: make(chan struct{}, 1), abandoned: make(chan struct{})} + host := newFakeHost(t) + host.writers = held + window.Open(host) + if host.content == nil { + t.Fatal("opening the window put no screen in it") + } + t.Cleanup(func() { + close(held.abandoned) + join(host) + }) + return host, tabNamed(t, host.content, text.TabOneTarget()), held +} + +// inTheMiddle waits for a writer to be held: a run that has claimed its +// manifest, is writing files and cannot end before it is stopped. +func (p *writersHeld) inTheMiddle(t *testing.T) { + t.Helper() + select { + case <-p.reached: + case <-time.After(writersHeldAtMost): + t.Fatal("no file was being written within twenty seconds, so there was never a run to stop") + } +} + +// stoppedByTheGuard says whether it was the guard's press that let the writers +// go, rather than the bound. +func (p *writersHeld) stoppedByTheGuard(t *testing.T) { + t.Helper() + if p.outlasted.Load() { + t.Errorf("nothing asked the run to stop within %v of a file being written, so the writers were let go to finish", writersHeldAtMost) + } +} + // comesBackWhileHeld presses something that must not wait for the work, with the // worker parked, and says whether it came back. A press that waits would wait // for ever - the worker is parked until the guard lets it go - so it is given diff --git a/internal/guard/window_test.go b/internal/guard/window_test.go index b1eed012..afed02f7 100644 --- a/internal/guard/window_test.go +++ b/internal/guard/window_test.go @@ -111,6 +111,8 @@ type fakeHost struct { // hold parks the worker just before it reports, so that a guard can read // the screen while a run is going. Nil unless a guard asked for one. hold *holdDuringRun + // writers holds a run's writers until it is stopped - see writersHeld. + writers *writersHeld // 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. diff --git a/internal/gui/window/open.go b/internal/gui/window/open.go index 13b1abb8..8ac84614 100644 --- a/internal/gui/window/open.go +++ b/internal/gui/window/open.go @@ -1,6 +1,7 @@ package window import ( + "context" "os" "fyne.io/fyne/v2" @@ -479,6 +480,7 @@ func offerSettling(h Host, screens []interface{ Settled() }) { // stand still while the guard looks, which is an ordering rather than a delay - // see runner.HoldBeforeFinishing. func offerHolding(h Host, screens []interface{ HoldBeforeFinishing(func()) }) { + offerHoldingTheWriters(h, screens) p, ok := h.(interface{ HoldDuringRun() func() }) if !ok { return @@ -492,6 +494,26 @@ func offerHolding(h Host, screens []interface{ HoldBeforeFinishing(func()) }) { } } +// offerHoldingTheWriters is the same for the hold a run's writers pass +// through, given to the screens that write files - see +// runner.HoldWhileWriting. Asked from offerHolding rather than from Open, so +// Open does not grow a line for something only the guards use. +func offerHoldingTheWriters(h Host, screens []interface{ HoldBeforeFinishing(func()) }) { + p, ok := h.(interface{ HoldWhileWriting() func(context.Context) }) + if !ok { + return + } + hold := p.HoldWhileWriting() + if hold == nil { + return + } + for _, screen := range screens { + if writes, ok := screen.(interface{ HoldWhileWriting(func(context.Context)) }); ok { + writes.HoldWhileWriting(hold) + } + } +} + // watchedSettle is a screen's reading of its form, with two things told about // it on the way. // diff --git a/internal/gui/window/run.go b/internal/gui/window/run.go index ca289caa..1cdd716a 100644 --- a/internal/gui/window/run.go +++ b/internal/gui/window/run.go @@ -1,6 +1,7 @@ package window import ( + "context" "errors" "path/filepath" "sort" @@ -602,6 +603,7 @@ func (r *runner) startRun(targets []engine.Target, opt engine.Options) { // file. Thinned out here, before anything crosses over, so the interface is // asked to redraw ten times a second rather than thousands. opt.OnProgress = func(p engine.Progress) { + r.busy.holdWhileWriting(ctx) if !limit.allow(time.Now()) { return } @@ -769,6 +771,23 @@ func (r *runner) Stop() { r.work.stop() } // instead of hiding them. func (r *runner) HoldBeforeFinishing(fn func()) { r.busy.hold = fn } +// HoldWhileWriting sets a hold a run's writers pass through each time they +// report progress, handed the run's context. A seam for the guards like +// HoldBeforeFinishing above, and the shipped program never sets it. The +// context rather than its Done channel, so the program pays a nil check per +// report and this package declares no channel for a seam +// (concurrency_test.go). +// +// It is what keeps a run in the middle while a guard stops it. The engine +// reports under one lock (Options.OnProgress, never two at once), so a hold +// on the first report holds every writer at its next one, and a hold that +// returns when the context is done lets them go only once Cancel or closing +// the window has asked them to stop. Before it, the guards that stop a run +// of four hundred files watched the folder for the first entry and stopped +// the run from there, which was a bet that the run would still be going a +// moment later - an outside review of the pull request named it. +func (r *runner) HoldWhileWriting(fn func(ctx context.Context)) { r.busy.writing = fn } + // Settled waits for work in flight to finish, without stopping it. // // A seam for the guards, and named as one rather than dressed up - the same diff --git a/internal/gui/window/runbusy.go b/internal/gui/window/runbusy.go index 2e9d0b38..c884a10b 100644 --- a/internal/gui/window/runbusy.go +++ b/internal/gui/window/runbusy.go @@ -1,6 +1,7 @@ package window import ( + "context" "time" "fyne.io/fyne/v2" @@ -95,6 +96,11 @@ type busy struct { // runner.HoldBeforeFinishing for what it is for. Here rather than on the // screens since 2026-11-03, when the Tools tab came to need it as well. hold func() + + // writing, when a guard supplies one, is run on a writer each time a run + // reports progress, with the run's context. Nil in the shipped program - + // see runner.HoldWhileWriting. + writing func(ctx context.Context) } // busyFace is which parts of the face a piece of work earns: whether there @@ -208,6 +214,14 @@ func (b *busy) holdBeforeFinishing() { } } +// holdWhileWriting runs the hold on the writers, if a guard supplied one. +// Called by a writer reporting progress, before anything crosses over. +func (b *busy) holdWhileWriting(ctx context.Context) { + if b.writing != nil { + b.writing(ctx) + } +} + // relay lays the row of buttons out again, because the toolkit does not. // // Measured in the pinned toolkit on 2026-09-21, from the owner's report that