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/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/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/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/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/generatewindow_test.go b/internal/guard/generatewindow_test.go index 98474fb4..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" @@ -454,28 +453,38 @@ 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. +// +// 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) } - // 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 +498,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,31 +590,36 @@ 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. 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() - _, 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 { t.Fatalf("there is no Cancel button. The screen has: %v", buttonNames(content)) } cancel.OnTapped() + join(host) + held.stoppedByTheGuard(t) 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,47 +769,18 @@ 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.") - } -} - -// 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) + "What to do: assign r.work first. The work exists by then, so nothing is gained by waiting.") } - t.Fatal("nothing was written within twenty seconds, so there was never a run to interrupt") } 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/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..5f91e058 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", @@ -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", ), ( @@ -5135,7 +5121,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 +5131,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", @@ -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", ), ( @@ -10546,7 +10532,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", @@ -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", ), ( @@ -16580,4 +16564,123 @@ " .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", + ), + ( + "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/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/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..5181628c --- /dev/null +++ b/internal/guard/stopping_test.go @@ -0,0 +1,302 @@ +package guard + +import ( + "context" + "os" + "path/filepath" + "strings" + "sync" + "sync/atomic" + "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/gui/window" + "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) + } +} + +// 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 +// 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..afed02f7 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 @@ -108,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. @@ -274,7 +279,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/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/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/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/open.go b/internal/gui/window/open.go index 350d93d2..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" @@ -43,8 +44,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 +136,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 +149,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 +169,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 @@ -524,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 @@ -537,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/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) +} diff --git a/internal/gui/window/run.go b/internal/gui/window/run.go index 05d040ce..1cdd716a 100644 --- a/internal/gui/window/run.go +++ b/internal/gui/window/run.go @@ -19,14 +19,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 +115,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 +447,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 +472,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 +583,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{} @@ -621,45 +603,33 @@ 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 } 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 +649,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 +732,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 +769,24 @@ 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 } + +// 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. // @@ -819,17 +801,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..c884a10b 100644 --- a/internal/gui/window/runbusy.go +++ b/internal/gui/window/runbusy.go @@ -1,11 +1,14 @@ package window import ( + "context" "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 +51,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 +90,17 @@ 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() + + // 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 @@ -175,6 +190,38 @@ 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() + } +} + +// 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 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() +}