From c5f300b6707fa614347d24a9c89ed23fc5a47328 Mon Sep 17 00:00:00 2001 From: mixeme Date: Mon, 27 Jul 2026 16:42:20 +0300 Subject: [PATCH] docs: close the GUI layout review, release 0.16.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage 9 of the GUI layout plan: the roadmap item the review was raised under is closed, so the plan and the findings document go with it — what they established now lives in STANDARDS and the CHANGELOG. STANDARDS gains the rule the review produced: a size that must follow the theme is measured at build time, not written as a pixel constant, because a hand-tuned number is only correct for the theme it was tuned against. rowOverlap, captionColumnWidth, textColumnWidth, activityRowsHeight and initialSplitOffset are the worked examples. The CHANGELOG entry keeps to what the user can see: the window opens at the size it asks for and drags smaller, the Jobs divider is draggable, History columns hold their content on a scaled UI, and the Settings button row and block spacing are as their layouts intended. Co-Authored-By: Claude Opus 5 --- docs/CHANGELOG.md | 45 ++++ docs/GUI-LAYOUT-REVIEW.md | 386 ------------------------------ docs/PLAN-gui-layout.md | 477 -------------------------------------- docs/ROADMAP.md | 44 ---- docs/STANDARDS.md | 10 + src/app/version.go | 2 +- 6 files changed, 56 insertions(+), 908 deletions(-) delete mode 100644 docs/GUI-LAYOUT-REVIEW.md delete mode 100644 docs/PLAN-gui-layout.md diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index dad3b91..28c6b83 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -2,6 +2,51 @@ All notable GoSentry changes are recorded in this file. +## 0.16.0 - 2026-07-27 + +**The window opens at the size it asks for, and the Jobs divider can be +dragged.** + +**Window:** + +- **The window opens at 1024×660 and can now be dragged narrower than it opens.** + Fyne treats the assembled content's minimum size as a hard floor over the + requested size, and two widgets in Settings pushed that minimum past 1024 px: + a fixed width applied to seven controls that the layout already stretched, and + the read-only config path, which grew the whole tab with the length of the + path it was showing — a 75-character path alone demanded 1501 px. The path now + clips when the window is genuinely narrow instead of widening the window, and + the content minimum is 972 px. + +**Jobs:** + +- **The divider between the job list and the details pane is draggable.** + Previously the list was pinned at its natural width and the details pane took + whatever was left, so a long command or a deep folder path could not be given + more room. Either pane can now be widened at the other's expense, and neither + can be dragged below its own content, so the details pane condenses rather + than clipping. The divider opens at the list's natural width; its position is + not saved, so a restart reopens at that default. + +**History:** + +- **Columns measure their own content.** Time, Trigger and State were fixed + pixel widths with as little as 1.6 px of headroom and truncated their own + values on a scaled UI or at a larger text size; all five now size themselves + from the text they have to show, under the current theme. Job and Detail stay + bounded so one long row cannot take over the table. + +**Settings:** + +- The **Save / Cancel / Restore defaults** row sits 4 px from the left edge, as + its layout always intended, rather than 8. +- The caption column is as wide as the widest caption instead of a fixed width, + which gives each value column about 22 px more and keeps the captions readable + at a larger text size. +- The **Application** and **About** blocks are about 2 px tighter: every stacked + row group in the app now shares one spacing derived from the theme rather than + three separately tuned numbers. + ## 0.15.0 - 2026-07-26 **Settings points at the jobs file itself, not the folder holding it.** diff --git a/docs/GUI-LAYOUT-REVIEW.md b/docs/GUI-LAYOUT-REVIEW.md deleted file mode 100644 index 7d7ef23..0000000 --- a/docs/GUI-LAYOUT-REVIEW.md +++ /dev/null @@ -1,386 +0,0 @@ -# GUI layout review — custom layouts and composition - -Findings for the *"GUI review — custom layouts and composition"* item in -[ROADMAP.md](ROADMAP.md). Scope is composition only: the four custom -`fyne.Layout` implementations in [`src/ui/layout.go`](../src/ui/layout.go), the -tuned constants the views drive them with, and how the assembled views behave -when the window or the theme scale changes. It is not a general code review. - -Reviewed at Fyne v2.7.4. Every number below was measured with a throwaway -headless probe (`test.NewApp()` + `theme.DefaultTheme()`), not estimated: at the -default text size (14; `theme.Padding()` = 4, `theme.InnerPadding()` = 8) and, -where scale matters, at text size 20. The probes were deleted after the -measurements were taken; the numbers are reproducible from the recipes quoted in -each finding. - -Nothing in the code was changed by this pass. Each finding carries a -disposition: **single fix** (goes straight in) or **roadmap** (larger than one -fix, comes back to ROADMAP). - -## Summary - -The four custom layouts are mostly justified — but one of them -(`compactVBoxLayout`) is a re-implementation of a stock Fyne layout that has -existed since 2.5, and another (`minWidthLayout`) is applied at nine sites of -which eight enforce a width that turns out to have no visual effect at all. - -The bigger result is about the constants rather than the layouts. Three raw-pixel -width constants (`settingsLabelWidth`, `settingsControlWidth`, -`minJobsSidebarWidth`) are *floors*, and content already reaches or exceeds all -three at the default text size or one step above it. They no longer shape -anything on screen. What they still do is set the minimum size of the window — -and that minimum (**1165×543** for a typical install) is **wider than the -1024×660 window the app asks for at startup**. GoSentry cannot open at its own -default size, and the user cannot drag it narrower. - -## Per-layout verdict - -The roadmap asks three questions of each layout: is it still needed, is it the -smallest thing that works, does it hold up at other window sizes and theme -scales. - -| Layout | Call sites | Still needed | Smallest thing that works | Holds up when scaled | -|---|---|---|---|---| -| `minWidthLayout` | 9 | Partly — 1 site is load-bearing, 8 are not | No — see F2, F7 | Yes: it takes the max with the child's own minimum, so it can never clip | -| `compactVBoxLayout` | 3 | **No** — stock `layout.NewCustomPaddedVBoxLayout` is identical | No — delete the type (F4) | Yes, but the *spacing values* do not (F5) | -| `fixedHeightLayout` | 1 | **Yes** — keep | Yes (see below) | Yes: its one caller derives the height from the theme | -| `captionValueLayout` | 1 (via `detailRow`, 11 rows) | Yes | Yes | Yes for the caption; the value column has no floor of its own (F9) | - -On `fixedHeightLayout`, which the roadmap singles out as a one-call-site type: -there is no stock layout that forces an exact height. The closest stock -construction is `container.NewStack(list, rect)` with a transparent -`canvas.Rectangle` carrying `SetMinSize(fyne.NewSize(0, activityRowsHeight(3)))`, -which is equivalent *here* only because the parent is a `Border` bottom slot and -a bottom slot grants exactly `MinSize().Height`. That is a 29-line type traded -for a one-line invisible-rectangle trick that depends on the parent staying a -`Border`. **Keep the type** — and note the codebase already uses the -invisible-rectangle trick twice in `settings_view.go` (F8), so the two idioms -currently coexist for no reason. Standardise on the named layout. - -## Per-constant verdict - -| Constant | Value | What content actually needs | Binds? | -|---|---|---|---| -| `settingsLabelWidth` | 180 | 169.2 — widest caption, *"Default overlap policy"* | Only below text size 15 (180.1 at 15). Visible effect: yes, it sets the caption column | -| `settingsControlWidth` | 330 | 306.7 — widest control, the notifications checkbox | Only below text size 16. Visible effect: **none** (F2) | -| `minJobsSidebarWidth` | 400 | 448.0 — the 5-button toolbar row | **Never** (F7) | -| `detailRowSpacing` | −8 | = −`theme.InnerPadding()` at the default theme | Yes — but the relation to the theme is only implicit (F5) | -| `jobRowSpacing` | −8 | as above | as above | -| `settingsRowSpacing` | −6 | no principled relation to any theme metric | as above | -| `logColumnMinWidth` / `MaxWidth` / `Padding` | 240 / 520 / 24 | a real log name measures 268.9 at text size 14, 373.7 at 20 | Yes; the min/max are sane, the 24 is a guess at `2×InnerPadding` = 16 (F14) | -| History column widths | 150/90/170/90/260 | 148.4/75.7/114.9/86.7/144.2 at text size 14 | Yes — Time has 1.6 px of headroom, State 3.3, which a scaled UI eats (F6) | -| `activityRowsHeight()` | derived | — | **Correct by construction — this is the model** | -| `detailCaptionWidth()` | derived | — | **Correct by construction — this is the model** | - -## Findings - -### F1 — The window cannot open at the size it asks for *(high, roadmap)* - -`run.go:54-56` asks for 1024×660 (or the persisted preference). The assembled -content's minimum is **1165.5×542.9** for an install whose config path is 53 -characters (`C:\Users\alice\AppData\Roaming\GoSentry\gosentry.json`). Fyne -enforces that minimum in two places: `window.Resize` takes -`size.Max(content.MinSize())`, and `fitContent` calls -`view.SetSizeLimits(minWidth, minHeight, …)`. So the window silently opens ~140 px -wider than requested and cannot be dragged narrower. - -The height is fine — 543 leaves room on a 720p screen, which is what the -condensed details pane was for. The problem is entirely horizontal, and it comes -from the Settings tab: `AppTabs` reports the maximum of its children, and the -three tabs measure 920.0 (Jobs), 40.0 (History), **1165.5 (Settings)**. - -The Settings minimum is also not a constant — it tracks the length of the config -file path: 1040 for a 10-character path, 1165 for 53, **1501 for 75**. A user -with a long Windows account name gets a window that will not fit on a 1366×768 -laptop screen. - -`container.NewVScroll` (`settings_view.go:304`) caps the tab's minimum *height* -at 32, and the comment there explains exactly why. The same reasoning was never -applied to the width. Two independent causes, F2 and F3, both need fixing to -bring the floor under 1024; measured together they take it to **993.3**, at which -point the binding row is the *Notifications* checkbox (490.7) and the tab no -longer widens with the config path at all. - -This also interacts with the frozen *Window size persistence* roadmap item: a -restored width below the content minimum would be silently widened anyway. - -### F2 — `settingsControlWidth` wrappers change nothing but the minimum *(medium, single fix)* - -Seven rows wrap their control in `container.New(minWidthLayout{width: 330}, …)` -(`settings_view.go:248, 252, 253, 254, 262, 263, 264`). Measured with a select at -three row widths, wrapped versus bare: - -| Row width | Wrapped select size | Bare select size | -|---|---|---| -| 700 | 516 | 516 | -| 514 | 330 | 330 | -| 420 | 236 | 236 | - -Identical, at every width, because `settingsRow` puts the control in a `Border` -centre slot, which already stretches it to whatever the column gives it — and -`minWidthLayout.Layout` in turn resizes its child to the container size. The -wrapper's *only* observable effect is on `MinSize`: the row reports 514 instead -of 311.9, ×2 columns, which is the 1040 floor in F1. - -**Fix:** delete the constant and the seven wrappers. No visual change at any -window size the user can reach; seven containers fewer; the Settings floor drops -by 46.7 px (the notifications checkbox then binds at 490.7). `minWidthLayout` -itself stays — its remaining caller, the caption box in `settingsRow`, sits in a -`Border` *left* slot, which grants exactly `MinSize().Width`, so there the -constant is what renders. - -### F3 — One label in Settings is not truncated, and it sets the window width *(medium, single fix)* - -`settingsRow("Config JSON", widget.NewLabel(store.Paths.ConfigPath))` -(`settings_view.go:273`) is the only value label in the tab with default -wrapping, so its minimum width is the full pixel width of the path: the label -measures 392.8 for a 53-character path (12.7 truncated), which makes the row 576.8 -against 196.7. Fyne's grid gives every column the widest cell's width, so that row -alone costs **2×** its width in the tab minimum. - -The rule this row is missing is already established one screen away — -`autostartStatus.Wrapping = fyne.TextTruncate` at `settings_view.go:128`, with the -comment *"Truncating keeps a long status message from forcing the column wider."* -The read-only path row simply never got it. - -**Fix:** truncate it like its neighbour. The full path stays readable whenever the -window is wide enough, which it will be by default. - -### F4 — `compactVBoxLayout` re-implements a stock Fyne layout *(medium, single fix)* - -`layout.NewCustomPaddedVBoxLayout(padding float32)` has existed since Fyne 2.5 -and does exactly what `compactVBoxLayout` does, negative padding included. -Compared directly at spacings −8, −6, 0 and 4, the two produce **identical** -`MinSize` values and identical position and size for every child; they also agree -on a hidden middle child (62.16), an empty object list (0×0) and a single child -(43.25×35.08). The stock layout is a strict superset — it additionally -distributes `layout.Spacer` objects, which the local copy ignores. - -**Fix:** delete the type (≈40 lines) and replace its three call sites with -`container.New(layout.NewCustomPaddedVBoxLayout(spacing), …)`. Keep the *comment* -explaining why the spacing is negative — that rationale is not in the stock docs. - -### F5 — The negative spacings are magic numbers with a theme expression available *(medium, single fix)* - -`detailRowSpacing = -8` and `jobRowSpacing = -8` are exactly -`-theme.InnerPadding()` at the default theme, and the match is not accidental: -two stacked labels contribute 8 px of inner padding each at their shared edge, so -−8 removes one label's worth and leaves the other. Written as −8 the reasoning is -invisible and the value stops tracking a theme that changes -`SizeNameInnerPadding` — the exact class of breakage the roadmap flags. - -Text does not currently overlap at either measured scale (line height 19.1 within -a 35.1 label at text size 14; 27.2 within 43.2 at text size 20), so this is a -robustness fix, not a rendering bug. - -`settingsRowSpacing = -6` has no such derivation — it is 0.75 of the inner -padding, chosen by eye. - -**Fix:** replace the three constants with one function, in the style -`activityRowsHeight` and `detailCaptionWidth` already set: - -```go -// rowOverlap pulls stacked label rows together by exactly one label's vertical -// inner padding, which is the whitespace two adjacent labels double up on. -func rowOverlap() float32 { return -theme.InnerPadding() } -``` - -A function, not a `const`, because `theme.InnerPadding()` must be read after the -app exists. Settings then either adopts the same value (−8, a 2 px change) or -keeps a documented fraction of it. - -### F6 — History column widths are raw pixels and truncate on a scaled UI *(medium, single fix)* - -`table.SetColumnWidth(0…4, 150/90/170/90/260)` leaves as little as 1.6 px of -headroom at the default text size (Time, holding `2026-06-01 10:00:00`) and 3.3 -(State, holding `Succeeded`). At text size 20 three of the five columns truncate -their own content: - -| Column | Width | Needs at text size 20 | -|---|---|---| -| Time | 150 | 204.9 (`2026-06-01 10:00:00`) | -| Trigger | 90 | 101.3 (`Schedule`) | -| State | 90 | 117.0 (`Succeeded`) | - -The Log column is already immune — `logColumnWidth` measures its content with -`fyne.MeasureText(…, theme.TextSize(), …)`. The remaining five columns should be -sized the same way, from a representative sample string per column (a timestamp, -the longest trigger and state names) rather than from a pixel count. - -### F7 — `minJobsSidebarWidth` never binds *(low, single fix)* - -The Jobs sidebar's natural minimum is **448.0**, set by the toolbar row of five -buttons; the constant is 400. It has never had an effect at the default theme, -and scaling only widens the gap. The `Border` left slot gives the sidebar exactly -its `MinSize` width, so the wrapper contributes nothing. - -**Fix:** delete the constant and the wrapper. One caveat: `jobsSidebar` in -[`jobs_view_test.go`](../src/ui/jobs_view_test.go:141) locates the sidebar by -looking for a container whose layout is `minWidthLayout`, so it needs a different -anchor in the same change. See also F15 — an `HSplit` would remove the question. - -### F8 — The settings button row is indented twice as far as its comment claims *(low, single fix)* - -`settings_view.go:299-311` builds two transparent `canvas.Rectangle` spacers, the -second to *"[indent] the buttons from the left edge the same amount"* as -`theme.Padding()`. Measured, the first button lands at **x = 8**, not 4: an -`HBox` inserts its own `theme.Padding()` gap *after* the spacer, so the inset is -doubled. - -**Fix:** drop both rectangles for the stock -`layout.NewCustomPaddedLayout(2*theme.Padding(), 0, theme.Padding(), 0)` wrapped -around a plain `HBox`. That reproduces the current 12 px top gap exactly (VBox -padding + 2× padding) while giving the intended 4 px left inset — and removes the -codebase's second spacing idiom (see the `fixedHeightLayout` note above). If the -8 px inset is what was actually wanted, the constant should say so instead. - -### F9 — The details value column has no floor of its own *(low, roadmap)* - -`captionValueLayout` gives the caption a fixed width and hands the remainder to -the value, with no lower bound: `valueWidth = size.Width - captionWidth - -padding`, clamped at 0. Measured across the pane's reachable widths: - -| Window width | Caption | Value | -|---|---|---| -| 1583 | 116.2 | 439.3 | -| 1024 | 116.2 | 159.8 | -| 920 (the panel's own minimum) | 116.2 | 107.8 | - -So the value never actually vanishes — but only because -`commandOutputScroll.SetMinSize(fyne.NewSize(460, 70))` -([`jobs_view_details.go:62`](../src/ui/jobs_view_details.go:62)) keeps the pane -460 px wide, and that constant exists for an unrelated reason (readable command -output). Lower it and the value column silently starves; feed the layout 240 px -directly and the value renders at width 0 with no warning. - -The coupling is invisible in both files. Either give `captionValueLayout` its own -minimum (shrink the caption once the value would drop below some floor, so the -caption truncates first), or record the dependency at both ends. - -Related, same type: `MinSize` and `Layout` both `return` silently when -`len(objects) != 2`. A miswired caller renders an empty row rather than failing. -The type is package-private with one constructor (`detailRow`), so this is a -documentation-grade nit, not a defect. - -### F10 — The detail caption list is written twice *(low, single fix)* - -`detailCaptionWidth()` ([`jobs_view_details.go:165`](../src/ui/jobs_view_details.go:165)) -hard-codes the eleven caption strings to measure the widest; `container()` writes -the same eleven strings again, thirty lines above, to build the rows. They match -today. Add a twelfth row and forget the list, and the new caption silently -truncates — with no test to catch it, because the width is correct for the -captions the function knows about. - -**Fix:** build the rows from one `[]struct{caption string; value fyne.CanvasObject}` -and derive the width from that same slice. - -### F11 — The History table re-sorts its whole backing slice once per cell *(medium, single fix)* - -```go -func(id widget.TableCellID, item fyne.CanvasObject) { - label.SetText(historyCellText(id, sortedEvents())) -``` - -`sortedEvents()` copies the event slice and `sort.SliceStable`s it — and it is -called from the per-cell update callback. Measured with 300 events in a 1200×800 -window: **126 `UpdateCell` calls per `Refresh`, so 126 copies and 126 sorts of a -300-element slice** for one redraw. A redraw runs on every recorded run, every -service error, and every UI action, since `mainwindow.go`'s observer calls -`refresh()` unconditionally. - -**Fix:** sort once per refresh into a slice the callback reads. The same callback -also assigns a constant `fyne.TextStyle{}` that the row template already carries, -then calls `label.Refresh()` for that assignment; with the assignment gone the -`Refresh` goes too, because `SetText` already refreshes. - -### F12 — Jobs handlers repeat the work `refreshView` is about to do *(low, single fix)* - -`refreshView` already calls `syncFromService()`, recomputes `filteredJobs`, -updates the details panel and calls `list.Refresh()`. Six handlers call -`list.Refresh()` immediately before calling `refreshView()` -(`jobs_view.go:238, 256, 270, 302, 315, 347`, plus `181`/`191` in the folder -filter), and the pause handler additionally repeats `syncFromService()`. -`widget.List.Refresh()` re-creates the row template and re-measures the row -height, so this is not free. Where the earlier `syncFromService()` is genuinely -needed — `folderOptions(jobs)` reads the refreshed slice — it should stay, with -only the redundant `list.Refresh()` removed. - -### F13 — `settings_view.go` is 445 lines and speaks two dialects *(low, roadmap)* - -Past the ~250-line guideline in [ARCHITECTURE.md](ARCHITECTURE.md), and the split -`jobs_view.go` already demonstrates the shape. Three seams are visible in the -file as it stands: - -- **`settings_view.go`** — `settingsView`: field construction, save/load/validate. -- **`settings_view_layout.go`** — `settingsSection`, `settingsRow`, the two - columns, the button row. -- **`settings_view_helpers.go`** — `fyneVersion`, `mustParseURL`, - `settingsFolderPath`, `openFolder`, the file/folder pickers. - -Two composition inconsistencies to settle in the same pass rather than carry -across the split: - -- The *Application* and *About* blocks use `settingsSection` (condensed spacing); - *Queue* and *Storage* inline `container.NewVBox(header, rows…)` (theme - spacing). Two spellings of "a titled block of rows" in one function. Both - comments justify the difference, but a `settingsSection(title, spacing, rows…)` - — or two named constructors — would say it once. -- `chooseFile` and `chooseJSONFile` are the same eight lines apart from - `SetFilter`. One function taking a filter (`nil` for none) removes the copy. - Note `chooseFile` is also used by `job_dialog.go`, so it belongs with the - helpers, not with Settings. - -### F14 — `logColumnPadding = 24` *(low, single fix)* - -The cell is a `widget.Label`, whose text is inset by `theme.InnerPadding()` on -each side: 16 px, plus table padding. 24 is a hand-tuned guess at that. Express -it as `2*theme.InnerPadding()` (plus whatever margin is wanted, named) so it -tracks the theme like the width it is added to already does. The three constants -are also untyped `int` while every other width constant in the package is a typed -`float32`. - -### F15 — Master/detail is a fixed `Border`, not a split *(low, roadmap)* - -`jobs_view.go:365-366` pins the sidebar at its `MinSize` in a `Border` left slot, -so the user can never give the details pane more room or the job list less. This -is what `container.NewHSplit` is for: a draggable divider, `SetOffset` for the -initial ratio, and no `minWidthLayout` wrapper or `minJobsSidebarWidth` constant -needed. It changes behaviour rather than just structure, so it is a roadmap item, -not a cleanup — but it subsumes F7 outright, gives F9 a user-controlled escape -(drag the divider left to widen the value column), and is the idiomatic Fyne -composition for this screen. - -## What holds up — do not "fix" these - -- **`activityRowsHeight()` and `detailCaptionWidth()`.** Both derive their result - by measuring a real widget under the current theme. They are the pattern - everything else in the package should converge on, and they already behave - correctly at text size 20 (114.2→138.7 and 116.2→159.1). -- **`minWidthLayout` degrades safely.** Because `MinSize` takes the *max* of the - configured width and the child's own minimum, a width routed through this layout - can never clip its content — it can only inflate a minimum. That is why F1 is a - sizing problem and not a rendering one, and it is the property the History - column widths in F6 lack. -- **The compact/detailed row mechanism.** `applyRowMode` expressing the mode as - visibility, with the comment about `widget.List` caching the template's - `MinSize`, is correct and non-obvious; both call sites are needed. -- **The negative spacing itself.** It does not overlap text at either measured - scale. F5 is about how the number is written, not about abandoning the - technique. -- **`container.NewVScroll` around Settings.** It correctly keeps the tab from - dictating the window's minimum height. F1 is the same idea left half-applied. - -## Suggested order - -1. F2 + F3 together — they are the two causes of F1, and neither changes anything - visible above the minimum window width. Measured result: 1165.5 → **993.3**. - Verify by asserting the assembled content's `MinSize().Width` stays under the - 1024 default in a test; that is the regression guard F1 has been missing. If - more headroom is wanted afterwards, deriving `settingsLabelWidth` from the - widest caption the way `detailCaptionWidth` does buys another 21.7 (→ 971.7). -2. F4, F7, F14 — deletions, no behaviour change. F7 needs the test helper - re-anchored in the same commit. -3. F11, F12 — redraw cost, self-contained. -4. F5, F8, F10 — the workaround-shaped constants, once the deletions have settled. -5. F6 — needs a per-column sample string decided first. -6. F9, F13, F15 — back to [ROADMAP.md](ROADMAP.md). diff --git a/docs/PLAN-gui-layout.md b/docs/PLAN-gui-layout.md deleted file mode 100644 index dd2375a..0000000 --- a/docs/PLAN-gui-layout.md +++ /dev/null @@ -1,477 +0,0 @@ -# Implementation plan — GUI layout cleanup - -## Context - -[GUI-LAYOUT-REVIEW.md](GUI-LAYOUT-REVIEW.md) recorded fifteen findings against -the `ui` package's custom layouts, tuned constants, and view composition. This -plan turns all fifteen into landable work. - -**Part A** (stages 1–6) is the set the review classified as single fixes: each -stage is one commit, none of them changes what the user sees except where the -stage says so. **Part B** (stages 7–8) is the roadmap-sized work — a file split -and a behaviour change. **Stage 9** closes the roadmap item and ships the batch. - -Three decisions were open when this plan was written; all three are settled and -folded in below: - -- **Scope: everything.** Part A, the `settings_view.go` split (stage 7) and the - draggable split pane (stage 8) all land, so the roadmap item closes completely - rather than carrying F9, F13 and F15 forward. -- **The divider position is not persisted** (stage 8). It stays a `ui`-only - change with the initial ratio computed at build time; no new `Config` field. -- **Row spacing unifies on −8** (stage 2), so `rowOverlap()` is the single - expression for the whole package. - -Finding IDs (F1…F15) refer to the review. One item found while writing this plan -and not in the review is labelled N1. - -## Design decisions - -These are cross-cutting; settling them once keeps the stages from contradicting -each other. - -- **A size that must track the theme is a function, not a `const`.** - `theme.Padding()` and friends read `fyne.CurrentApp()`, so they cannot be - evaluated at package init. `activityRowsHeight()` and `detailCaptionWidth()` - already establish the shape — measure a real widget under the current theme, - at build time. Every constant this plan replaces converges on that form, and - Stage 9 writes the rule into [STANDARDS.md](STANDARDS.md). -- **One spacing idiom: named layouts.** The transparent-`canvas.Rectangle` - spacer disappears (F8). Where an exact size is needed and no stock layout - expresses it, a named `fyne.Layout` in `layout.go` does — which is why - `fixedHeightLayout` stays. -- **`minWidthLayout` stays, its callers mostly do not.** The type is sound (it - takes the max, so it can never clip). Seven of its nine call sites are inert - (F2) and one never binds (F7); the survivor is the settings caption box, and - even that stops being a raw pixel count in Stage 6. -- **The assembled window must fit the size the app asks for.** `run.go` opens at - 1024×660; Fyne treats the content minimum as a hard floor. That becomes an - invariant with a test behind it (Stage 1), not a thing to re-measure by hand. -- **"No visual change" is asserted, not claimed.** Stages 1 and 3 delete widths - on the grounds that nothing renders differently. Each carries a test that - measures geometry rather than trusting the argument in this document. -- **No new dependencies.** Everything here is stock Fyne 2.7.4 plus the standard - library. - -## Part A — single fixes - -### Stage 1 — bring the window minimum under the default window size (F1, F2, F3) - -The headline finding: assembled content measures 1165.5×542.9 against a -requested 1024×660, so Fyne silently widens the window and forbids dragging it -back. Two causes, both in `settings_view.go`. - -**Changes** - -1. `src/ui/settings_view.go` — **F2**: delete `settingsControlWidth` - (line 30) and unwrap the seven controls that use it (lines 248, 252, 253, - 254, 262, 263, 264), e.g. `settingsRow("Theme", themeSelect)`. `settingsRow` - puts the control in a `Border` centre slot, which already stretches it to the - column width, so the wrapper only ever inflated `MinSize`. -2. `src/ui/settings_view.go:273` — **F3**: give the read-only config-path label - `Truncation = fyne.TextTruncateClip`, the non-deprecated form established by - commit 706aa8e. Extract it to a local so the assignment has somewhere to - live. This is what stops the tab's minimum from tracking the length of the - user's config path (1040 → 1165 → 1501 for 10-, 53- and 75-character paths). -3. `src/ui/run.go:54-55` — replace the `1024` / `660` literals with - `defaultWindowWidth` / `defaultWindowHeight` consts so the test in this stage - asserts against the same numbers the app uses, with a comment recording that - Fyne enforces the content minimum over them. - -**Tests** - -- `src/ui/mainwindow_test.go` — `TestMainViewFitsTheDefaultWindowSize`: build - `newMainView` against a store whose `Paths.ConfigPath` is a deliberately long - path, then assert `content.MinSize()` is within - `defaultWindowWidth`×`defaultWindowHeight`. This is the regression guard F1 - never had; the long path is what keeps F3 from silently regressing. -- `src/ui/settings_view_test.go` — `TestSettingsRowStretchesItsControl`: resize a - `settingsRow` to a width above its minimum and assert the control fills the - remaining width. That is the property that makes F2's deletion invisible. - -**Expected result:** Settings minimum width 1165.5 → 993.3; the binding row -becomes the *Notifications* checkbox, and the tab no longer widens with the -config path. - -### Stage 2 — stock layout, theme-derived spacing (F4, F5) - -**Changes** - -1. `src/ui/layout.go` — delete `compactVBoxLayout` (lines 40–80). Verified - identical to `layout.NewCustomPaddedVBoxLayout` at spacings −8, −6, 0 and 4, - for the three-child, hidden-middle-child, single-child and empty cases; the - stock layout additionally handles `layout.Spacer`, which the copy ignored. -2. `src/ui/jobs_view.go:139`, `src/ui/jobs_view_details.go:124`, - `src/ui/settings_view.go:323` — switch to - `container.New(layout.NewCustomPaddedVBoxLayout(rowOverlap()), …)`. Add the - `fyne.io/fyne/v2/layout` import to the latter two. -3. `src/ui/layout.go` — **F5**: add `rowOverlap()` and delete - `detailRowSpacing`, `jobRowSpacing` (`jobs_view.go:29,35`) and - `settingsRowSpacing` (`settings_view.go:36`): - - ```go - // rowOverlap is the (negative) gap that pulls stacked label rows together by - // exactly one label's vertical inner padding. Two adjacent labels each inset - // their text by theme.InnerPadding(), so the whitespace between two lines of - // text is double what a single row needs; removing one label's worth - // condenses the block without letting the text lines touch. Derived rather - // than hard-coded so it follows a theme that changes SizeNameInnerPadding. - func rowOverlap() float32 { return -theme.InnerPadding() } - ``` - - This carries over the explanation the deleted `compactVBoxLayout` comment - held, which is the part not documented upstream. - -**Decided:** Settings currently uses −6 where the other two use −8; adopting -`rowOverlap()` moves it to −8, a 2 px tightening of the Application and About -blocks, and that is the one deliberate visual change in Part A. Check it in the -running app at step 4 of *Verification*; if it reads too tight, the fallback is a -documented fraction (`rowOverlap() * 0.75`), never a reinstated literal. - -**Tests** - -- `src/ui/layout_test.go` (new) — `TestRowOverlapMatchesInnerPadding`: assert - `rowOverlap() == -theme.InnerPadding()` and that it is negative, under the - default theme and under a theme with a different inner padding. -- Existing `TestJobListViewToggleShrinksRowsAndPersists` already covers that the - job rows still shrink in compact mode through the replacement layout; no new - test needed for F4 beyond a green run. - -### Stage 3 — delete the sidebar width floor (F7) - -**Changes** - -1. `src/ui/jobs_view.go:19` — delete `minJobsSidebarWidth`; at line 365 pass - `sidebar` straight to the `Border` left slot. The toolbar row already needs - 448 against the constant's 400, so it has never bound, and a `Border` left - slot renders the child at exactly its `MinSize` width either way. -2. `src/ui/jobs_view_test.go:141` — re-anchor `jobsSidebar`, which currently - finds the sidebar by looking for a `minWidthLayout` container. Anchor it on - the pane structure instead: the `Border`'s left object is the sidebar - (`panel.Objects[1]` — `NewBorder` appends slots after the centre object). - Add the same one-line comment the row-template code uses about `NewBorder` - ordering, since this is the second place that ordering is relied on. Stage 8 - moves this anchor once more, to the split's `Leading`. - -**Tests** - -- `src/ui/jobs_view_test.go` — `TestJobsSidebarWidthIsItsContent`: assert the - sidebar's `MinSize().Width` equals the toolbar row's, i.e. that nothing else - imposes a floor. The existing `jobsList`/`jobsViewToggle` helpers exercise the - re-anchored lookup. - -### Stage 4 — redraw cost (F11, F12) - -**Changes** - -1. `src/ui/history_view.go:133-148` — **F11**: hoist the sort out of the per-cell - callback. Keep a `rows []event` snapshot beside `descending`; a `resort()` - closure refills it. Call `resort()` when the view is built, from `refresh()` - (line 179) before `table.Refresh()`, and from the header tap handler - (line 157) after flipping `descending`. **The length callback must switch - from `len(*events)` to `len(rows)`** — today it is safe only because each - cell re-derives the sorted slice from the same source, and a cache breaks - that agreement. Cells then read `rows` directly. - Drop the `label.TextStyle = fyne.TextStyle{}` assignment (the template - already carries the zero value) and the `label.Refresh()` that exists for it, - since `SetText` refreshes. -2. `src/ui/history_view.go:109-121` — hoist the `headers` slice out of - `headerText` into a package-level `var historyHeaders = [...]string{…}`; it - is currently reallocated on every header-cell update. -3. `src/ui/jobs_view.go` — **F12**: delete the `list.Refresh()` calls that - immediately precede `refreshView()` (lines 238, 256, 270, 302, 315, 347) and - the duplicate `syncFromService()` at line 314. `refreshView` already does - both. Keep the `syncFromService()` calls at 228, 253 and 333 — `folderOptions(jobs)` - reads the refreshed slice on the next line. - **Careful with line 181:** the folder-filter handler returns early when the - filter matches nothing, and that path never reaches `refreshView()`, so its - `list.Refresh()` is load-bearing. Move it into the early-return branch rather - than deleting it. - -**Tests** - -- `src/ui/history_view_test.go` — `TestHistorySortToggleKeepsRowsInSync`: build - the view over N events, flip the sort through the header tap, assert the first - and last cell text; append an event, call `refresh`, assert the row count and - the new event's placement in both orders. This is the regression test for the - cache-versus-length hazard the change introduces (severity medium → - regression test required by [STANDARDS.md](STANDARDS.md)). -- `src/ui/jobs_view_test.go` — extend an existing button test to assert the - selection and details survive a handler that lost its `list.Refresh()`. - -**Measured motivation:** 300 events in a 1200×800 window produced 126 -`UpdateCell` calls per `Refresh`, each copying and sorting the whole 300-element -slice, on every recorded run. - -### Stage 5 — content-measured History columns (F6, F14) - -The Log column already sizes itself from its content; the other five are pixel -counts with as little as 1.6 px of headroom, and three of them truncate their own -values at text size 20. - -**Changes** - -1. `src/ui/history_view.go:45-74` — generalise `logColumnWidth` into one helper: - - ```go - // textColumnWidth returns the width a table column needs to show the widest - // of the given samples in full, measured under the current theme so it - // follows text size and DPI, and clamped to [min, max]. - func textColumnWidth(samples []string, min, max float32) float32 - ``` - - with `cellPadding()` = `2 * theme.InnerPadding()` replacing the hand-tuned - `logColumnPadding = 24` (**F14**), and the min/max bounds themselves expressed - as measured text (`textWidth(strings.Repeat("0", 30))`) rather than raw - pixels. The three surviving bounds become typed `float32`, matching every - other width in the package. -2. `src/ui/history_view.go:169-174` — feed each column its samples: - - **Time** — the timestamp format itself (`2006-01-02 15:04:05` rendered), a - fixed width; no content scan needed. - - **Trigger** — the known trigger strings (`Schedule`, `Manual`, `UI`, - `Unknown`), a closed set. - - **State** — the known state strings (`Succeeded`, `Failed`, `Started`, - `Error`, `Jobs loaded`), likewise closed. - - **Job** and **Detail** — free text: measure the values actually present, - bounded like the Log column so one long row cannot dominate the table. -3. `refresh` (line 179) recomputes all content-derived columns, not just Log. - -**Tests** - -- `src/ui/history_view_test.go` — `TestHistoryColumnsFitTheirContent`: for each - column, at the default text size and at a scaled theme, assert the configured - width is at least what its widest sample measures. Table-driven, and it fails - today for Time, Trigger and State at text size 20. -- `TestTextColumnWidthClamps`: below-min, in-range and above-max samples. - -**Note:** the trigger and state strings are produced in `app`/`runner` -(`app.StatusText`, the run recorder). The samples live in `ui` next to the -column they size; a comment should point at where the real strings come from so -a new state gets added in both places. - -### Stage 6 — one caption-width helper, button row, truncation idiom (F8, F10, N1, F9 nit) - -**Changes** - -1. `src/ui/layout.go` — **F10**: one helper for both views: - - ```go - // captionColumnWidth returns the width to reserve for a column of bold - // captions: the widest of them, measured under the current theme so it tracks - // text size and DPI instead of a hand-tuned constant. - func captionColumnWidth(captions ...string) float32 - ``` - -2. `src/ui/jobs_view_details.go:119-191` — build the metadata rows and their - width from **one** list instead of two. Today `detailCaptionWidth` hard-codes - the eleven captions and `container()` writes the same eleven again thirty - lines away; a twelfth row added to one and not the other silently truncates. - - ```go - type detailRowSpec struct { - caption string - value fyne.CanvasObject - } - - func (d *detailsPanel) metadataRows() []detailRowSpec - ``` - - `container()` derives `capW := captionColumnWidth(captions(specs)...)`, then - walks the slice two at a time into `detailRowPair`, with the odd tail - (Statistics) falling through to a single `detailRow`. `detailCaptionWidth` - disappears. -3. `src/ui/settings_view.go:26-29,440-445` — delete `settingsLabelWidth` and - derive the caption box the same way. Worth 21.7 px per column on top of - Stage 1 (993.3 → 971.7), and it removes the last raw-pixel width in Settings. - `settingsRow` gains a `captionWidth float32` first parameter and `settingsView` - computes it once from the full caption list — exactly how `detailRow` already - takes the width `container()` measured, rather than re-measuring per row. -4. `src/ui/settings_view.go:299-311` — **F8**: delete both transparent - `canvas.Rectangle` spacers for - - ```go - container.New( - layout.NewCustomPaddedLayout(2*theme.Padding(), 0, theme.Padding(), 0), - container.NewHBox(saveSettings, cancelSettings, restoreDefaults, settingsStatus), - ) - ``` - - which reproduces the current 12 px top gap exactly while fixing the left - inset to the 4 px the comment promises — measured, the rectangle version puts - the first button at x = 8, because `HBox` adds its own padding *after* the - spacer. The `image/color` and `fyne.io/fyne/v2/canvas` imports go with it. -5. **N1** (not in the review) — `fyne.TextTruncate` as a `Wrapping` value is - deprecated in Fyne 2.7.4 and commit 706aa8e converted exactly one call site. - Eight remain: `settings_view.go:128,442`, - `jobs_view_details.go:67,155,187,195`, `history_view.go:88,139`. Convert them - to `Truncation = fyne.TextTruncateClip` in one pass. **`jobs_view_details.go:67` - and `:155` must change together** — the second measures a sample label that - has to stay identical to the first, which is the whole basis of - `activityRowsHeight`. -6. `src/ui/layout.go:121-136` — **F9 (nit half)**: `captionValueLayout.MinSize` - and `Layout` both return silently when `len(objects) != 2`. Document why that - is acceptable (package-private, one constructor) at the type, so the next - reader does not have to work it out. The substantive half of F9 is Stage 8. - -**Tests** - -- `src/ui/jobs_view_test.go` — `TestDetailCaptionWidthCoversEveryCaption`: for - every spec `metadataRows()` returns, the bold caption measures no wider than - `captionColumnWidth` returns. This is the guard that makes the single list - self-enforcing. -- `src/ui/settings_view_test.go` — the same assertion for the settings captions. -- `src/ui/layout_test.go` — `captionColumnWidth` over an empty list, one caption, - and at two text sizes. - -## Part B — larger, but decided - -### Stage 7 — split `settings_view.go` (F13) - -445 lines against the ~250 guideline in [ARCHITECTURE.md](ARCHITECTURE.md), and -`jobs_view.go` already demonstrates the shape. Stages 1 and 6 remove roughly 25 -lines from it, so the split should follow them, not precede them. - -**Changes** - -- `settings_view.go` — `settingsView`: field construction, save, load, validate. -- `settings_view_layout.go` — `settingsSection`, `settingsRow`, the two columns, - the button row. -- `settings_view_helpers.go` — `fyneVersion`, `mustParseURL`, - `settingsFolderPath`, `openFolder`, `chooseFile`/`chooseFolder`. -- Two composition inconsistencies to settle in the same pass rather than carry - across the split: - - *Application* and *About* use `settingsSection` (condensed); *Queue* and - *Storage* inline `container.NewVBox(header, rows…)` (theme spacing). Two - spellings of "a titled block of rows" in one function. Resolve to - `settingsSection(title, spacing, rows…)` or two named constructors. - - `chooseFile` and `chooseJSONFile` differ only by `SetFilter`. One function - taking a filter (`nil` for none) removes the copy. `chooseFile` is also - `job_dialog.go`'s, so it belongs with the helpers, not with Settings. -- `docs/ARCHITECTURE.md` — add a file-structure table for `settings_view.go` - beside the existing `jobs_view.go` one (~line 206). - -**Tests:** no new behaviour, so the existing suite is the check. Worth adding -`settings_view_test.go` coverage for the deduplicated picker's filter argument. - -### Stage 8 — draggable master/detail split (F15, F9) - -`jobs_view.go:365-366` pins the sidebar at its `MinSize` in a `Border` left slot, -so the user can never trade list width for detail width. `container.NewHSplit` -is the idiomatic Fyne answer: a draggable divider, `SetOffset` for the initial -ratio. It gives F9 a user-controlled escape — the details value column bottoms -out at 107.8 px today, kept non-empty only by the unrelated 460 px minimum on the -command-output scroll. - -**Decided: the divider position is not persisted.** No `Config` field, no -`domain`/`storage`/`app` changes, no config-compatibility tests — the stage stays -inside `src/ui` and is one commit. A restart reopens at the computed default. -If persistence is wanted later it is an additive `omitempty` field whose zero -value means "compute the default", which is exactly the pattern -[STANDARDS.md](STANDARDS.md) already requires. - -**Changes** - -1. `src/ui/jobs_view.go:365-366` — replace the `Border` with - `container.NewHSplit(sidebar, container.NewPadded(dp.container()))`. -2. Initial offset: `SetOffset` takes a ratio, but the sidebar's natural width is - absolute (448). A fixed ratio is wrong at both ends — 0.44 fits 1024 but hands - the sidebar 700 px at 1600. Compute it at build time from - `sidebar.MinSize().Width / defaultWindowWidth` (the const Stage 1 introduces), - with a comment saying why it is derived rather than a literal. -3. This retires what is left of F7's wrapper. Stage 3 still lands first and on - its own — Part A has to stand up even if this stage were deferred — which - costs one extra line in `jobsSidebar`: Stage 3 anchors it on - `panel.Objects[1]` (the `Border` left slot), this stage moves it to the - split's `Leading`. - -**Tests** - -- `src/ui/jobs_view_test.go` — `TestJobsSplitOpensAtTheSidebarWidth`: assert the - computed offset gives the leading pane at least its content minimum at the - default window width, so the toolbar is never born clipped. -- Verify manually (Verification step 7) that dragging the divider hard left - degrades the details pane instead of clipping it: `HSplit` lets either side - shrink to its content minimum, which for the details pane is the 460 px - command-output floor. - -### Stage 9 — close-out - -1. `docs/STANDARDS.md` — add the rule this review established, under *Code - quality*: **a size that must follow the theme is measured at build time, not - written as a pixel constant** — `activityRowsHeight`, `captionColumnWidth`, - `rowOverlap` and `textColumnWidth` as the examples. -2. `docs/ROADMAP.md` — remove the *GUI review — custom layouts and composition* - item outright. With Part B in scope, nothing from it carries forward. -3. `docs/CHANGELOG.md` — a `## 0.16.0` section. User-visible: the window opens - at the size it is asked for and can be dragged smaller; the Jobs pane divider - is draggable; History columns stay readable on a scaled UI; the Settings - button row is aligned as intended; the details/settings blocks shift by ~2 px. - The rest is internal and belongs in the commit messages, not here. -4. `src/app/version.go` — `0.15.0` → `0.16.0`. -5. Delete `docs/PLAN-gui-layout.md` and `docs/GUI-LAYOUT-REVIEW.md`, the way - `docs/plans/per-job-timeout.md` was removed once shipped (2ab5f07) — the - findings live on in the CHANGELOG and STANDARDS entries, and with the whole - plan implemented the review has nothing left to hold open. - -## Recommended model per stage - -Which Claude model to run each stage on. The split is by *judgement density*, -not by diff size: the stages that only move code around are the cheap ones, and -the stages that can break something silently are not. - -| Stage | Model | Why | -|---|---|---| -| 1 — window minimum (F2, F3) | Sonnet 5 | Small deletions, but the new geometry test has to assert the right thing. The plan already fixes the numbers, so there is little left to decide. | -| 2 — stock layout, `rowOverlap` (F4, F5) | Sonnet 5 | Mechanical: delete a type, swap three call sites, add two imports. The equivalence it rests on is already measured. | -| 3 — sidebar floor (F7) | Sonnet 5 | One constant and one wrapper out; the only care needed is the `NewBorder` slot ordering in the test helper, which the plan spells out. | -| **4 — redraw cost (F11, F12)** | **Opus 5** | The one stage that can break History silently. Caching the sorted slice forces the length callback off `len(*events)`, and the `list.Refresh()` at `jobs_view.go:181` is load-bearing on an early-return path. Both are easy to get subtly wrong and neither shows up as a compile error. | -| 5 — History columns (F6, F14) | Sonnet 5 | The helper's shape is specified. One cross-package check: the trigger and state sample strings must match what `app`/`runner` actually emit. | -| 6 — caption width, button row, truncation (F8, F10, N1) | Sonnet 5 | Broad but mechanical. The one trap is paired: `jobs_view_details.go:67` and `:155` must change together or `activityRowsHeight` stops mirroring the real row. | -| 7 — split `settings_view.go` (F13) | Sonnet 5 | Pure code movement plus two small dedups. Large diff, low judgement — but re-read the moved file ends for dropped functions. | -| **8 — HSplit (F15, F9)** | **Opus 5** | Behaviour change. The offset derivation and "does the pane degrade or clip when dragged hard left" are judgement calls that a headless test cannot settle. | -| **9 — close-out (docs, version)** | **Opus 5** | CHANGELOG and STANDARDS prose held to the standard the rest of `docs/` sets, which is the expensive part of this repo's doc convention. | - -Notes: - -- **Escalate on the second failure.** If a stage's tests fail twice for reasons - the plan did not anticipate, the assumption behind that stage is wrong — move - it to Opus 5 rather than iterating. -- **Verification is model-independent.** `scripts\test.bat` and the manual GUI - pass below are the gate regardless of who wrote the diff; a cheaper model does - not mean a lighter check. -- Haiku 4.5 is deliberately not recommended for any stage here: every one of - them edits GUI code whose correctness is geometric rather than textual, and - the cheapest stages are already short enough that the saving is small. -- Fable 5 is left out because its trade-offs are not characterised well enough - here to recommend it for a specific stage, not because it was judged unfit. - -## Verification - -Per stage: - -```bash -export PATH="/c/msys64/ucrt64/bin:$PATH"; export CGO_ENABLED=1; go build ./... && go vet ./... && go test -race ./... -``` - -or `scripts\test.bat` from the cgo-enabled PowerShell shell described in -[CLAUDE.md](../CLAUDE.md). - -After Part A, run the app (`go run ./cmd/gosentry`) and check the things no -headless test covers: - -1. The window opens at 1024×660 and can be dragged **narrower** than it opens — - the F1 symptom, gone. -2. Settings looks unchanged at the default window width: controls still fill - their column, captions still align, the config path is readable and clips - only when the window is genuinely narrow. -3. The Save/Cancel/Defaults row keeps its gap below the separator and sits 4 px - from the left edge, not 8. -4. Jobs rows and the details metadata block are unchanged in Detailed and in - Compact; the Application/About blocks in Settings are 2 px tighter (Stage 2's - one deliberate change). -5. History still sorts both ways from the Time header, new runs still append, - and columns hold their content. -6. Repeat 2, 4 and 5 with a scaled desktop (or a temporary theme override with a - larger `SizeNameText`): nothing that used to be a pixel constant should clip. -7. Resize the window to its minimum in both directions and confirm the details - pane degrades rather than clipping — the check the roadmap item asked for. diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 3392a2a..a48867d 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -81,50 +81,6 @@ Design notes / open questions: Service exposes import/export operations; the UI only picks the file and shows the outcome. -### GUI review — custom layouts and composition - -**The review has been carried out — its findings are in -[GUI-LAYOUT-REVIEW.md](GUI-LAYOUT-REVIEW.md).** This item stays open until they -are applied; F9, F13 and F15 there are larger than a single fix and come back -here once the rest has landed. The agenda below is what the pass was asked to -answer. - -The `ui` package has accumulated hand-written layouts and tuned constants that -work but have never been reviewed as a whole: -[`layout.go`](../src/ui/layout.go) holds four custom `fyne.Layout` -implementations (`minWidthLayout`, `compactVBoxLayout`, `fixedHeightLayout`, -`captionValueLayout`), and the views drive them with negative spacings -(`detailRowSpacing = -8`, `jobRowSpacing = -8`, `settingsRowSpacing = -6`) that -cancel out the built-in padding of Fyne widgets. - -Do a focused pass over composition only — not a general code review — and -answer, per layout and per constant: is it still needed, is it the smallest -thing that works, and does it hold up at different window sizes and theme -scales. - -What to look for: - -- *Negative spacing as a workaround.* Pulling rows together to overlap label - padding is a workaround for widget metrics, not a layout decision. Check - whether a `widget.Form`, a grid, or a custom text row would express the same - result without depending on the padding a future Fyne release may change. -- *Hard-coded pixel constants.* Widths and heights expressed in raw pixels - (`logColumnMinWidth`, `minJobsSidebarWidth`, `settingsControlWidth`) do not - follow `theme.Padding()` / text size, so they behave differently under a - scaled UI. `activityRowsHeight` already derives its height from the theme — - decide which of the rest should do the same. -- *Layouts with one call site.* `fixedHeightLayout` is used once. If a stock - container expresses the same intent, deleting the type is a net win — the - complexity rule in [REVIEW.md](REVIEW.md) §2 applies to layouts too. -- *File size.* `settings_view.go` is well past the ~250-line guideline in - [ARCHITECTURE.md](ARCHITECTURE.md) and should be split the way `jobs_view.go` - was. -- *Behaviour at small sizes.* The details pane was condensed to fit 720p; verify - the current composition still degrades sensibly when the window is narrow or - short, instead of clipping. - -Findings that are single fixes go straight in; anything larger comes back here. - ### Window size persistence *(frozen)* Window size is currently **not** saved on quit or close. Saving was disabled diff --git a/docs/STANDARDS.md b/docs/STANDARDS.md index 84fa3ee..ec37b63 100644 --- a/docs/STANDARDS.md +++ b/docs/STANDARDS.md @@ -12,6 +12,16 @@ what a whole-project review looks at, in [REVIEW.md](REVIEW.md). - Fixes with severity ≥ medium → regression test. - Documented intentional behavior → section below, not a backlog bug. - UI view constructors accept `*app.Service`; call `app.Open()` only from `run.go`. +- A size that must follow the theme is **measured at build time, not written as + a pixel constant.** `theme.Padding()` and text metrics depend on the running + app's theme, text size, and DPI, so a hand-tuned number is only correct for + the one theme it was tuned against and clips under any other. Measure the real + widget, or derive the value from the theme, in a named helper: `rowOverlap` + (theme padding), `captionColumnWidth` and `textColumnWidth` (the widest of the + actual strings), `activityRowsHeight` (the list's own row template). The same + applies to a ratio computed from an absolute width — see `initialSplitOffset`. + A raw pixel literal is left only where nothing about it tracks the theme, and + says so in a comment. ## Config file compatibility diff --git a/src/app/version.go b/src/app/version.go index ae525ed..260adf4 100644 --- a/src/app/version.go +++ b/src/app/version.go @@ -3,4 +3,4 @@ package app // Version is the application version shown in the GUI and used by build // scripts in artifact names. It is a var rather than a const so release builds // can override it with Go ldflags when CI tags a build. -var Version = "0.15.0" +var Version = "0.16.0"