diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f65806..a213ffe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -134,6 +134,14 @@ because it turns other people's test suites red. ### Fixed +- **The explanation beside a field opens on the first hover, and opens and + closes from the keyboard.** Pointing at the information mark in a fresh + window showed nothing until something else on the screen happened to + repaint, and Space on the mark with the keyboard drew nothing at all. The + window's controls now tell the canvas about each piece they change, and the + sheet the explanation is drawn on says so when a box is put on it or taken + off. + - **A refused preset no longer carries the note of the preset before it.** A limit of 512 B was refused with "no limit was given" under it - a note left over from the last expansion that worked. diff --git a/internal/guard/canvastold_test.go b/internal/guard/canvastold_test.go new file mode 100644 index 0000000..ad6ac46 --- /dev/null +++ b/internal/guard/canvastold_test.go @@ -0,0 +1,536 @@ +package guard + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "go/types" + "path/filepath" + "sort" + "strings" + "testing" +) + +// Two rules about the one thing the test driver cannot see: whether the +// canvas was TOLD that something changed. +// +// The driver paints when its canvas is dirty, and the canvas is dirty when an +// object it has painted before asks to be. That "before" is the whole trap. +// The driver files each object it paints under the value the tree holds - +// type and pointer together - so a refresh sent for any other value finds no +// canvas, marks nothing and says nothing. Measured on 2026-09-16 in a copy of +// the window with the driver logging every refresh (O217): the pointer +// arrived on the explanation button, the button's face changed, the +// explanation was put on its sheet, and the driver's log ended there. No +// repaint. The refresh had been asked for the Button embedded inside +// DetailButton, and the tree holds the DetailButton. +// +// The test driver answers CanvasForObject with the last window's canvas +// whatever the object, and that canvas ignores Refresh - so every guard that +// hovers, taps or reads the tree stayed green through this, and would stay +// green through it again. What can be asked in a test is the shape of the +// code, so that is what these two ask, mechanically and over both packages +// the window is built from. +// +// What neither can see: a piece or a box held in a local variable rather than +// in a field, a write hidden in a function that is not a method of the +// renderer, one element of a slice written and another named, and a box +// changed on a path that returns before the refresh at the end of the +// function. The copy of the window with the logging driver is the instrument +// for those, and it lives in tools, not here. + +// canvasSource is one parsed production file of the window's code. +type canvasSource struct { + rel string + fset *token.FileSet + file *ast.File +} + +// windowSources parses internal/gui/parts and internal/gui/window, production +// files only, and fails rather than returning nothing. +func windowSources(t *testing.T) []canvasSource { + t.Helper() + var out []canvasSource + for _, pkg := range []string{"parts", "window"} { + dir := filepath.Join(repoRoot(t), "internal", "gui", pkg) + files, err := filepath.Glob(filepath.Join(dir, "*.go")) + if err != nil || len(files) == 0 { + t.Fatalf("listing %s: %v", dir, err) + } + for _, file := range files { + if strings.HasSuffix(file, "_test.go") { + continue + } + fset := token.NewFileSet() + parsed, err := parser.ParseFile(fset, file, nil, 0) + if err != nil { + t.Fatalf("parsing %s: %v", file, err) + } + out = append(out, canvasSource{rel: "internal/gui/" + pkg + "/" + filepath.Base(file), fset: fset, file: parsed}) + } + } + return out +} + +// declaredFields indexes every struct type declared in the sources: type name, +// then field name, then the field's type as it is written. +func declaredFields(sources []canvasSource) map[string]map[string]string { + index := map[string]map[string]string{} + for _, src := range sources { + for _, decl := range src.file.Decls { + gen, ok := decl.(*ast.GenDecl) + if !ok { + continue + } + for _, spec := range gen.Specs { + collectDeclaredFields(spec, index) + } + } + } + return index +} + +func collectDeclaredFields(spec ast.Spec, index map[string]map[string]string) { + ts, ok := spec.(*ast.TypeSpec) + if !ok { + return + } + st, ok := ts.Type.(*ast.StructType) + if !ok { + return + } + fields := map[string]string{} + for _, field := range st.Fields.List { + written := types.ExprString(field.Type) + for _, name := range field.Names { + fields[name.Name] = written + } + } + index[ts.Name.Name] = fields +} + +// methodBody is one method with the file it was read from, so a reader that +// follows a call into it can still say where a line is. +type methodBody struct { + src canvasSource + fn *ast.FuncDecl +} + +// methodsOf indexes the methods each receiver type answers, by name. +func methodsOf(sources []canvasSource) map[string]map[string]methodBody { + index := map[string]map[string]methodBody{} + for _, src := range sources { + for _, decl := range src.file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + if _, typeName := receiverOf(fn); typeName != "" { + if index[typeName] == nil { + index[typeName] = map[string]methodBody{} + } + index[typeName][fn.Name.Name] = methodBody{src: src, fn: fn} + } + } + } + return index +} + +// answers says whether a type has every one of these methods. +func answers(has map[string]methodBody, names ...string) bool { + for _, name := range names { + if _, ok := has[name]; !ok { + return false + } + } + return true +} + +// receiverOf is the receiver's name and type of a method, both empty for a +// plain function. A pointer receiver reports the type it points at. +func receiverOf(fn *ast.FuncDecl) (ident, typeName string) { + if fn.Recv == nil || len(fn.Recv.List) != 1 { + return "", "" + } + recv := fn.Recv.List[0] + if len(recv.Names) == 1 { + ident = recv.Names[0].Name + } + return ident, strings.TrimPrefix(types.ExprString(recv.Type), "*") +} + +// fieldChain reduces an expression of the shape recv.a.b to the field names +// a, b when it starts at the receiver, and returns nil for anything else. +func fieldChain(expr ast.Expr, recv string) []string { + var chain []string + for { + sel, ok := expr.(*ast.SelectorExpr) + if !ok { + break + } + chain = append([]string{sel.Sel.Name}, chain...) + expr = sel.X + } + if id, ok := expr.(*ast.Ident); !ok || id.Name != recv || recv == "" || len(chain) == 0 { + return nil + } + return chain +} + +// pieceChain is fieldChain looking through an index: r.fills[i] and r.fills +// name the same field, because the slice is the piece a renderer holds and the +// index is which one it is drawing this time. +func pieceChain(expr ast.Expr, recv string) []string { + var chain []string + for { + switch e := expr.(type) { + case *ast.SelectorExpr: + chain = append([]string{e.Sel.Name}, chain...) + expr = e.X + continue + case *ast.IndexExpr: + expr = e.X + continue + case *ast.ParenExpr: + expr = e.X + continue + } + break + } + if id, ok := expr.(*ast.Ident); !ok || id.Name != recv || recv == "" || len(chain) == 0 { + return nil + } + return chain +} + +// resolveChain follows field names down from a struct type and returns the +// type of the last one as written, or "" when the chain leaves what the +// sources declare. +func resolveChain(fields map[string]map[string]string, typeName string, chain []string) string { + written := "" + for _, name := range chain { + declared, ok := fields[bareType(typeName)] + if !ok { + return "" + } + written, ok = declared[name] + if !ok { + return "" + } + typeName = written + } + return written +} + +// bareType is a written type without the pointer and slice marks in front of +// it, so a field holding one piece and a field holding a row of them resolve +// to the same declared type. +func bareType(written string) string { return strings.TrimLeft(written, "[]*") } + +// calledOn is the receiver expression and method name of a method call, or +// nil for anything else. +func calledOn(n ast.Node) (ast.Expr, string, *ast.CallExpr) { + call, ok := n.(*ast.CallExpr) + if !ok { + return nil, "", nil + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return nil, "", nil + } + return sel.X, sel.Sel.Name, call +} + +// A renderer redraws the pieces it draws, and never the widget. +// +// The rule is mechanical because the failure is silent: canvas.Refresh handed +// the wrong value is not an error, it is nothing. So no code in parts or +// window calls canvas.Refresh at all, every renderer's Refresh names its pieces +// through redraw, and none of those pieces is a widget of this package - a +// widget of this package asked to refresh would ask the canvas about itself, +// which is the same wrong question one step removed. +// +// And EVERY piece the face changes is named, not merely one. Until 2026-09-16 +// this counted the arguments of redraw and stopped, so a renderer setting the +// colour of four pieces and naming three passed - an outside review of the +// pull request pointed at the count. A piece is changed when a field of it is +// set, or when it is shown: Show on a rectangle, an image, a text or a +// container sets Hidden and stops (canvas/base.go, fyne v2.8.1, read in the +// pinned module), where Hide, Move and Resize repaint by themselves. The +// reading follows the renderer into its own methods, because two of the seven +// set their colours in a helper and a reader stopping at Refresh would have +// read nothing about them and stayed green for it. +func TestARendererRedrawsThePiecesItDrawsAndNeverTheWidget(t *testing.T) { + sources := windowSources(t) + fields := declaredFields(sources) + methods := methodsOf(sources) + + widgets := map[string]bool{} + renderers := map[string]bool{} + for typeName, has := range methods { + if answers(has, "CreateRenderer") { + widgets[typeName] = true + } + if answers(has, "Refresh", "Layout", "MinSize", "Objects", "Destroy") { + renderers[typeName] = true + } + } + if len(renderers) == 0 || len(widgets) == 0 { + t.Fatalf("found %d renderers and %d widgets, so this guard read the wrong tree", len(renderers), len(widgets)) + } + + var offences []string + checked, pieces := 0, 0 + for _, src := range sources { + for _, decl := range src.file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok { + continue + } + offences = append(offences, canvasRefreshCalls(src, fn)...) + recv, typeName := receiverOf(fn) + if !renderers[typeName] || fn.Name.Name != "Refresh" { + continue + } + checked++ + read := newRendererReading(recv, typeName, fields, widgets, methods) + read.walk(src, fn) + pieces += len(read.changed) + offences = append(offences, read.verdict(src, fn)...) + } + } + if checked == 0 || pieces == 0 { + t.Fatalf("%d renderer Refresh methods read and %d changed pieces found, so this guard would pass against anything", checked, pieces) + } + t.Logf("%d renderer Refresh methods read in parts, %d pieces changed by them, %d widget types known", checked, pieces, len(widgets)) + sort.Strings(offences) + for _, o := range offences { + t.Error(o) + } +} + +// canvasRefreshCalls names every canvas.Refresh call in a function. +func canvasRefreshCalls(src canvasSource, fn *ast.FuncDecl) []string { + var out []string + ast.Inspect(fn, func(n ast.Node) bool { + on, method, call := calledOn(n) + if call == nil || method != "Refresh" { + return true + } + if pkg, ok := on.(*ast.Ident); ok && pkg.Name == "canvas" { + out = append(out, fmt.Sprintf("%s:%d calls canvas.Refresh - a refresh asked for a value the tree does not hold "+ + "reaches no canvas and says nothing, so a renderer names its pieces through redraw instead", + src.rel, src.fset.Position(call.Pos()).Line)) + } + return true + }) + return out +} + +// rendererReading is what one renderer's Refresh does to the pieces it holds, +// read through every method of the renderer that Refresh calls. +type rendererReading struct { + recv, typeName string + fields map[string]map[string]string + widgets map[string]bool + methods map[string]map[string]methodBody + + // changed is each piece a field was set on or that was shown, with where + // that first happened, and named is each piece handed to redraw. + changed map[string]string + named map[string]bool + offences []string + visited map[string]bool +} + +func newRendererReading(recv, typeName string, fields map[string]map[string]string, + widgets map[string]bool, methods map[string]map[string]methodBody) *rendererReading { + return &rendererReading{recv: recv, typeName: typeName, fields: fields, widgets: widgets, methods: methods, + changed: map[string]string{}, named: map[string]bool{}, visited: map[string]bool{}} +} + +// piece is the field of the renderer an expression names, through any index, +// and whether that field holds a widget of this package - which is not a piece +// but the thing wearing the face. +func (rd *rendererReading) piece(expr ast.Expr) (key string, widget bool) { + chain := pieceChain(expr, rd.recv) + if chain == nil { + return "", false + } + written := resolveChain(rd.fields, rd.typeName, chain) + return strings.Join(chain, "."), rd.widgets[bareType(written)] +} + +// walk reads one method: every field set on a piece, every piece shown, every +// piece named through redraw, and every method of this renderer called along +// the way, once each. +func (rd *rendererReading) walk(src canvasSource, fn *ast.FuncDecl) { + if rd.visited[fn.Name.Name] { + return + } + rd.visited[fn.Name.Name] = true + where := func(n ast.Node) string { return fmt.Sprintf("%s:%d", src.rel, src.fset.Position(n.Pos()).Line) } + ast.Inspect(fn.Body, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.AssignStmt: + for _, lhs := range node.Lhs { + if sel, ok := lhs.(*ast.SelectorExpr); ok { + rd.change(sel.X, where(lhs)) + } + } + case *ast.CallExpr: + rd.call(node, where(node)) + } + return true + }) +} + +// change records a piece whose face is now different from what the canvas +// last painted. +func (rd *rendererReading) change(expr ast.Expr, at string) { + key, widget := rd.piece(expr) + if key == "" || widget { + return + } + if _, seen := rd.changed[key]; !seen { + rd.changed[key] = at + } +} + +// call reads one call: redraw and what it was handed, a method of the renderer +// to follow, Show on a piece, or Refresh on a widget of this package. +func (rd *rendererReading) call(call *ast.CallExpr, at string) { + if id, ok := call.Fun.(*ast.Ident); ok && id.Name == "redraw" { + for _, arg := range call.Args { + key, widget := rd.piece(arg) + if widget { + rd.offences = append(rd.offences, at+" hands redraw a widget of this package, which asks the canvas about itself one step removed") + } + if key != "" { + rd.named[key] = true + } + } + return + } + on, method, _ := calledOn(call) + if on == nil { + return + } + if id, ok := on.(*ast.Ident); ok && id.Name == rd.recv { + if helper, ok := rd.methods[rd.typeName][method]; ok { + rd.walk(helper.src, helper.fn) + } + return + } + key, widget := rd.piece(on) + switch { + case method == "Refresh" && widget: + rd.offences = append(rd.offences, at+" refreshes a widget of this package from inside a renderer, which is the wrong value under embedding") + case method == "Show" && key != "": + rd.change(on, at) + } +} + +// verdict is every offence read, plus one for each piece changed and never +// named, and one for a Refresh that names nothing at all. +func (rd *rendererReading) verdict(src canvasSource, fn *ast.FuncDecl) []string { + out := rd.offences + for key, at := range rd.changed { + if !rd.named[key] { + out = append(out, fmt.Sprintf("%s %s.Refresh changes %s.%s and never names it through redraw, so that piece keeps the face the canvas last painted", + at, rd.typeName, rd.recv, key)) + } + } + if len(rd.named) == 0 { + out = append(out, fmt.Sprintf("%s:%d %s.Refresh names no piece through redraw, so nothing tells the canvas the face changed", + src.rel, src.fset.Position(fn.Pos()).Line, rd.typeName)) + } + return out +} + +// A box that gains or loses a piece says so, in the same function. +// +// Container.Add and Remove lay the box out and stop there. A piece just added +// is not known to the canvas until it has been painted once, so its own +// refresh reaches nothing, and a piece just removed is out of the tree - the +// box is the only thing that can ask for the repaint. Every box in +// internal/gui/window already did (five of six sites, measured 2026-09-16), +// and the sixth was the explanation sheet, which got away with it on the +// pointer's path because the button's face asked in the same tick and never +// on the keyboard's, where no face changes (O217). +func TestABoxThatGainsOrLosesAPieceSaysSo(t *testing.T) { + sources := windowSources(t) + fields := declaredFields(sources) + + var offences []string + sites := 0 + for _, src := range sources { + for _, decl := range src.file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil { + continue + } + recv, typeName := receiverOf(fn) + if recv == "" { + continue + } + changed, refreshed := boxesTouched(fn, recv, typeName, fields) + sites += len(changed) + for box, last := range changed { + line := src.fset.Position(last).Line + switch { + case !refreshed[box].IsValid(): + offences = append(offences, fmt.Sprintf("%s:%d %s changes %s and never refreshes it - the piece it added waits for a repaint from anywhere, and the piece it removed stays drawn until one", + src.rel, line, fn.Name.Name, box)) + case refreshed[box] < last: + offences = append(offences, fmt.Sprintf("%s:%d %s changes %s after the last word to the canvas about it (line %d) - a refresh before the change repaints the old contents", + src.rel, line, fn.Name.Name, box, src.fset.Position(refreshed[box]).Line)) + } + } + } + } + if sites == 0 { + t.Fatal("no Add, Remove or RemoveAll on a container field was read, so this guard would pass against anything") + } + t.Logf("%d container changes read across parts and window", sites) + sort.Strings(offences) + for _, o := range offences { + t.Error(o) + } +} + +// boxesTouched reads one method and returns the container fields it adds to +// or removes from, each with where the LAST change is, and the ones it +// refreshes, each with where the LAST refresh is. +// +// The last of each rather than any: a refresh standing before the change +// repaints what the box held before it, and the change after it is as silent +// as no refresh at all. Until 2026-09-16 any refresh anywhere in the method +// counted, which an outside review of the pull request pointed at. What this +// still reads as one method is the source order, so a change on a path that +// returns before the refresh at the foot of the method passes - two such +// paths exist, both on a value the registry cannot hand the menu. +func boxesTouched(fn *ast.FuncDecl, recv, typeName string, fields map[string]map[string]string) (changed, refreshed map[string]token.Pos) { + changed = map[string]token.Pos{} + refreshed = map[string]token.Pos{} + ast.Inspect(fn.Body, func(n ast.Node) bool { + on, method, call := calledOn(n) + if call == nil { + return true + } + chain := fieldChain(on, recv) + if chain == nil || resolveChain(fields, typeName, chain) != "*fyne.Container" { + return true + } + box := recv + "." + strings.Join(chain, ".") + switch method { + case "Add", "Remove", "RemoveAll": + changed[box] = max(changed[box], call.Pos()) + case "Refresh": + refreshed[box] = max(refreshed[box], call.Pos()) + } + return true + }) + return changed, refreshed +} diff --git a/internal/guard/detailpopup_test.go b/internal/guard/detailpopup_test.go index 9653e25..2892b30 100644 --- a/internal/guard/detailpopup_test.go +++ b/internal/guard/detailpopup_test.go @@ -5,12 +5,10 @@ import ( "testing" "fyne.io/fyne/v2" - "fyne.io/fyne/v2/driver/desktop" "fyne.io/fyne/v2/test" "github.com/donislawdev/TestingFilesGenerator/internal/gui/parts" "github.com/donislawdev/TestingFilesGenerator/internal/gui/text" - "github.com/donislawdev/TestingFilesGenerator/internal/gui/window" ) // The longer explanation is reachable, not merely written down. @@ -27,26 +25,21 @@ import ( // the text beside it. A button that opens nothing would leave every one of // these sentences written, shipped, translated and unread. // -// It presses the button rather than looking for one. A button in the tree -// proves the layout, and only tapping it proves the wiring - which is the -// difference the parity guard's own note calls the threshold for saying a -// capability is reachable. +// It moves the pointer through the canvas rather than calling MouseIn, and +// that was corrected on 2026-09-16. Until then it called button.MouseIn +// directly - a guard that names the hover and does not do it - so it could +// not tell a button the pointer reaches from one it does not, and it could +// not tell a visible screen from a hidden one: the search for a button runs +// over the whole tree and a hidden screen's button answers to a call just as +// well. Through the canvas both questions are asked at once, because a hover +// aimed at a button that is not where the canvas draws it opens nothing. func TestTheLongerExplanationOpensWhenAsked(t *testing.T) { app := test.NewApp() defer test.NewApp() app.Settings().SetTheme(parts.Theme()) - host := newFakeHost(t) - window.Open(host) - content := tabNamed(t, host.content, text.TabOneTarget()) - - // A real canvas, because the button asks the toolkit which canvas it is on - // and opens the explanation there. Built without one it does nothing at - // all, on purpose - several guards build a tree they never show. - w := test.NewWindow(host.content) - defer w.Close() - w.Resize(window.LargestOpening) - host.content.Refresh() + content, c := laidOutWindow(t) + screen := tabNamed(t, content, text.TabOneTarget()) // Every field that has one, rather than a sample. These are the sentences // nobody can see until they press something, so an unwired button is @@ -65,50 +58,69 @@ func TestTheLongerExplanationOpensWhenAsked(t *testing.T) { {text.FieldSeed(), text.DetailSeed()}, {text.FieldLabel(), text.DetailLabel()}, } { - button := detailButtonBeside(content, field.label) + button := detailButtonBeside(screen, field.label) if button == nil { t.Errorf("%q has a longer explanation and no button that opens it", field.label) continue } + centre := drawnCentre(t, button, field.label) // Pointing at it, which is what somebody meeting a small letter i // actually does. Reported on 2026-08-12: it opened on a click and - // people hovered and waited. The toolkit has no tooltip, so this is - // three methods of desktop.Hoverable answered by hand - and a hook that - // is answered but never wired looks identical from outside. - button.MouseIn(&desktop.MouseEvent{}) - if shown := allText(content); !strings.Contains(shown, field.detail) { + // people hovered and waited. The pointer arrives from nowhere, the way + // it does in a window just opened, because that is the arrival that + // showed nothing on 2026-09-16 (O217) - though what was wrong that day + // is invisible here and held by canvastold_test.go instead. + test.MoveMouse(c, centre) + if shown := allText(screen); !strings.Contains(shown, field.detail) { t.Errorf("hovering the button beside %q did not show its explanation.\nWanted: %q\nShown: %q", field.label, field.detail, shown) } // And gone when the pointer leaves. Without this half the guard passes // on a tooltip that opens and never closes, which is worse than one - // that never opens: it covers the field underneath it. - button.MouseOut() - if shown := allText(content); strings.Contains(shown, field.detail) { + // that never opens: it covers the field underneath it. Left along the + // row, onto the field's name, which answers to no pointer. + test.MoveMouse(c, centre.SubtractXY(button.Size().Width*2, 0)) + if shown := allText(screen); strings.Contains(shown, field.detail) { t.Errorf("the explanation for %q stayed on screen after the pointer left", field.label) } - // A click still works, and that is not a nicety. Hovering is not + // A press still works, and that is not a nicety. Hovering is not // something a keyboard can do, and UX9 says whatever the mouse can // reach the keyboard can too - so the tap has to survive the hover // being added. - button.OnTapped() - if shown := allText(content); !strings.Contains(shown, field.detail) { + test.TapCanvas(c, centre) + if shown := allText(screen); !strings.Contains(shown, field.detail) { t.Errorf("pressing the button beside %q did not show its explanation.\nWanted: %q\nShown: %q", field.label, field.detail, shown) } // And a second press takes it away again, because somebody who opened // it from a keyboard has no pointer to move off it. - button.OnTapped() - if shown := allText(content); strings.Contains(shown, field.detail) { + test.TapCanvas(c, centre) + if shown := allText(screen); strings.Contains(shown, field.detail) { t.Errorf("pressing the button beside %q a second time did not take the explanation away", field.label) } - button.MouseOut() } } +// drawnCentre is the middle of a control as the canvas draws it, and a +// refusal when the canvas does not draw it at all. +// +// A position of 0,0 is what the driver answers for anything outside the +// visible tree, and the search that finds these buttons walks hidden screens +// as readily as the shown one. The batch screen carries a Size field with a +// button of its own, so without this a guard can hover a button nobody can +// see, read the sentence it put on a sheet nobody can see, and pass. +func drawnCentre(t *testing.T, o fyne.CanvasObject, label string) fyne.Position { + t.Helper() + at := fyne.CurrentApp().Driver().AbsolutePositionForObject(o) + if at.IsZero() { + t.Fatalf("the button beside %q is not in the visible tree, so this guard is reading a hidden screen", label) + } + return at.Add(fyne.NewPos(o.Size().Width/2, o.Size().Height/2)) +} + // The explanation never uses the overlay layer. // // This is the flicker, written as the rule that actually governs it, and it @@ -138,30 +150,25 @@ func TestTheExplanationNeverUsesTheOverlayLayer(t *testing.T) { defer test.NewApp() app.Settings().SetTheme(parts.Theme()) - host := newFakeHost(t) - window.Open(host) - content := tabNamed(t, host.content, text.TabOneTarget()) - - w := test.NewWindow(host.content) - defer w.Close() - w.Resize(window.LargestOpening) - host.content.Refresh() + content, c := laidOutWindow(t) + screen := tabNamed(t, content, text.TabOneTarget()) - button := detailButtonBeside(content, text.FieldSize()) + button := detailButtonBeside(screen, text.FieldSize()) if button == nil { t.Fatal("there is no explanation button beside the size field, so this guard read the wrong tree") } - button.MouseIn(&desktop.MouseEvent{}) - defer button.MouseOut() + centre := drawnCentre(t, button, text.FieldSize()) + test.MoveMouse(c, centre) + defer test.MoveMouse(c, centre.SubtractXY(button.Size().Width*2, 0)) // It has to be up before its absence anywhere else means anything. A guard // that checked for an empty overlay without opening the explanation would // pass on a button that does nothing. - if shown := allText(content); !strings.Contains(shown, text.DetailSize()) { + if shown := allText(screen); !strings.Contains(shown, text.DetailSize()) { t.Fatal("hovering showed nothing, so this guard is measuring a button that does not work") } - if overlays := w.Canvas().Overlays().List(); len(overlays) != 0 { + if overlays := c.Overlays().List(); len(overlays) != 0 { t.Errorf("the explanation put %d thing(s) in the overlay layer, the first a %T.\n"+ "While an overlay is up the driver looks for the pointer in the overlay and nowhere else, "+ "so the button underneath cannot be found - it is told the pointer left and closes the "+ @@ -180,6 +187,10 @@ func TestTheExplanationNeverUsesTheOverlayLayer(t *testing.T) { // the thing you click, so the row is the switch and its button. Reading only // labels here reported the switch as having no way to open its explanation, // which was this guard being wrong rather than the window. +// +// It answers the LAST row it meets, from wherever it is asked to look. Asked +// of the whole window that is a row on a hidden screen, at position 0,0 - so +// a caller asks it of one screen and checks the answer is drawn (drawnCentre). func detailButtonBeside(o fyne.CanvasObject, label string) *parts.DetailButton { var found *parts.DetailButton walk(o, func(obj fyne.CanvasObject) { diff --git a/internal/guard/testdata/screens/generate-hovered.png b/internal/guard/testdata/screens/generate-hovered.png index 9b686ec..004f62b 100644 Binary files a/internal/guard/testdata/screens/generate-hovered.png and b/internal/guard/testdata/screens/generate-hovered.png differ diff --git a/internal/guard/testdata/screens/generate-refused-setting.png b/internal/guard/testdata/screens/generate-refused-setting.png index f8044af..dee38ba 100644 Binary files a/internal/guard/testdata/screens/generate-refused-setting.png and b/internal/guard/testdata/screens/generate-refused-setting.png differ diff --git a/internal/gui/parts/button.go b/internal/gui/parts/button.go index 79a9487..a18ec91 100644 --- a/internal/gui/parts/button.go +++ b/internal/gui/parts/button.go @@ -322,7 +322,10 @@ func (r *buttonRenderer) Refresh() { } else { r.label.Show() } - canvas.Refresh(r.button) + // The pieces, never r.button - see redraw. This face is worn by the + // explanation button through embedding, and a refresh asked for the inner + // Button reached no canvas there: measured 2026-09-16, O217. + redraw(r.bg, r.ring, r.label, r.icon) r.Layout(r.button.Size()) } diff --git a/internal/gui/parts/detail.go b/internal/gui/parts/detail.go index f893063..822009c 100644 --- a/internal/gui/parts/detail.go +++ b/internal/gui/parts/detail.go @@ -223,11 +223,26 @@ func (t *Tips) open(near fyne.CanvasObject, detail string) fyne.CanvasObject { box.Move(t.place(driver, near, box.Size())) t.sheet.Add(box) + t.sheet.Refresh() return box } +// close takes a box off the sheet, and the sheet says so. +// +// The sheet asks for the repaint in both directions, and the reason is the +// same in both: nothing else will. A box just made is not yet known to the +// canvas - it is filed when first painted, and this is before that - so a +// refresh asked of the box itself reaches nothing, and a box just removed is +// out of the tree. On the pointer's way in and out the button's own face +// changes and asks for a repaint in the same tick, which is how the sheet +// managed without this until 2026-09-16 and why the keyboard never did: a +// press on the focused button changes no face, so the box it put on the sheet +// waited for the next repaint from anywhere, and the box it took off stayed +// drawn until then. The sheet is the thing that changed, so the sheet says so +// (O217, and the convention every box in internal/gui/window already keeps). func (t *Tips) close(box fyne.CanvasObject) { t.sheet.Remove(box) + t.sheet.Refresh() } // place is where the box goes, in the sheet's own coordinates. diff --git a/internal/gui/parts/listrow.go b/internal/gui/parts/listrow.go index 7ce09e8..3bd861e 100644 --- a/internal/gui/parts/listrow.go +++ b/internal/gui/parts/listrow.go @@ -183,10 +183,7 @@ func (r *listRowRenderer) Refresh() { r.kind.Hide() } - r.back.Refresh() - r.tick.Refresh() - r.kind.Refresh() - r.label.Refresh() + redraw(r.back, r.tick, r.kind, r.label) // The width a row asks for changes with the picture, and the row is laid // out by the list rather than by this renderer. r.Layout(r.row.Size()) diff --git a/internal/gui/parts/progress.go b/internal/gui/parts/progress.go index e495adf..6ce5331 100644 --- a/internal/gui/parts/progress.go +++ b/internal/gui/parts/progress.go @@ -129,7 +129,7 @@ func (r *progressRenderer) MinSize() fyne.Size { return r.bar.MinSize() } func (r *progressRenderer) Refresh() { r.applyColours() r.Layout(r.bar.Size()) - canvas.Refresh(r.bar) + redraw(r.track, r.fill) } func (r *progressRenderer) Objects() []fyne.CanvasObject { diff --git a/internal/gui/parts/redraw.go b/internal/gui/parts/redraw.go new file mode 100644 index 0000000..d0de23d --- /dev/null +++ b/internal/gui/parts/redraw.go @@ -0,0 +1,33 @@ +package parts + +import "fyne.io/fyne/v2" + +// redraw tells the canvas about the pieces of a face that changed. A renderer +// calls it at the end of Refresh, naming what it draws. +// +// It exists because of what a renderer must NOT do instead, and that was +// measured rather than read: canvas.Refresh(r.button). The driver files every +// object it paints under the value the tree holds, and a value is a type and a +// pointer together - so a type that embeds Button hands the tree ITSELF, the +// driver files it as that outer type, and a refresh asked for the inner Button +// finds no canvas at all. Nothing is marked dirty, nothing is painted, and no +// error says so. On 2026-09-16 that was the explanation button: MouseIn ran, +// the explanation was put on its sheet, and the window stayed as it was until +// something else happened to repaint it (O217). +// +// The pieces have no such double identity. A rectangle in the tree is that +// rectangle, so asking for each piece reaches the canvas whoever holds the +// renderer. It is also what the toolkit's own renderers do, with one addition +// they can make and this package cannot: they refresh the widget through a +// private accessor that knows the outer type. +// +// The test driver cannot see any of this. It answers CanvasForObject with the +// last window's canvas whatever the object, and its canvas ignores Refresh, so +// a refresh sent to the wrong value is indistinguishable there from one sent to +// the right one. That is why the rule is held by reading the renderers rather +// than by driving them: TestARendererRedrawsThePiecesItDrawsAndNeverTheWidget. +func redraw(pieces ...fyne.CanvasObject) { + for _, piece := range pieces { + piece.Refresh() + } +} diff --git a/internal/gui/parts/segments.go b/internal/gui/parts/segments.go index 34d285e..bbc4ee5 100644 --- a/internal/gui/parts/segments.go +++ b/internal/gui/parts/segments.go @@ -305,17 +305,14 @@ func (r *segmentsRenderer) Refresh() { r.fills[i].FillColor = color.Transparent r.words[i].Color = PaletteColour(theme.ColorNamePlaceHolder, dark) } - r.fills[i].Refresh() - r.words[i].Refresh() + redraw(r.fills[i], r.words[i]) } if r.seg.marked { r.ring.StrokeWidth = ringWidth } else { r.ring.StrokeWidth = 0 } - r.border.Refresh() - r.ring.Refresh() - canvas.Refresh(r.seg) + redraw(r.border, r.ring) r.Layout(r.seg.Size()) } diff --git a/internal/gui/parts/tabs.go b/internal/gui/parts/tabs.go index dc070bb..4175958 100644 --- a/internal/gui/parts/tabs.go +++ b/internal/gui/parts/tabs.go @@ -179,7 +179,7 @@ func (r *tabsRenderer) Refresh() { word.Refresh() } r.placeIndicator(r.strip.Size()) - r.indicator.Refresh() + redraw(r.indicator) } func (r *tabsRenderer) Objects() []fyne.CanvasObject { @@ -353,9 +353,7 @@ func (r *tabWordRenderer) Refresh() { } else { r.ring.StrokeWidth = 0 } - r.back.Refresh() - r.ring.Refresh() - r.text.Refresh() + redraw(r.back, r.ring, r.text) } func (r *tabWordRenderer) Objects() []fyne.CanvasObject { diff --git a/internal/gui/parts/toggle.go b/internal/gui/parts/toggle.go index 8a226cc..2065936 100644 --- a/internal/gui/parts/toggle.go +++ b/internal/gui/parts/toggle.go @@ -255,11 +255,7 @@ func (r *toggleRenderer) Refresh() { r.square.StrokeWidth = edgeWidth r.tick.Hide() } - r.halo.Refresh() - r.ring.Refresh() - r.square.Refresh() - r.tick.Refresh() - canvas.Refresh(r.toggle) + redraw(r.halo, r.ring, r.square, r.tick) } func (r *toggleRenderer) Objects() []fyne.CanvasObject {