mix/gosentry
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs: GUI layout review — findings for the composition pass

Browse Source 
Carries out the "GUI review — custom layouts and composition" roadmap item and
records the result in docs/GUI-LAYOUT-REVIEW.md. Composition only; no code was
changed. Every number was measured with a throwaway headless Fyne probe at text
size 14 and 20, not estimated.

Headline finding: the Settings tab sets a minimum content width of 1165.5 px for
a typical install — wider than the 1024x660 window run.go asks for — so Fyne
silently widens the window on open and the user cannot drag it narrower. It also
grows with the length of the config-file path (1501 px for a 75-character one).
Two independent causes: the seven settingsControlWidth wrappers, which measure
identically to bare controls at every reachable width and only inflate MinSize,
and the Config JSON path label, the one value label in the tab without
truncation. Fixing both takes the floor to 993.3.

Also found: compactVBoxLayout is a byte-for-byte re-implementation of stock
layout.NewCustomPaddedVBoxLayout (identical geometry at every spacing tested);
minJobsSidebarWidth (400) never binds because the toolbar row already needs 448;
the negative row spacings are theme.InnerPadding() written as a magic number;
the History column widths truncate their own content on a scaled UI; and the
History table re-sorts its whole event slice once per cell — 126 sorts of a
300-element copy for one redraw.

Fifteen findings in all, each with a disposition (single fix or roadmap) and a
suggested order. fixedHeightLayout, which the roadmap singled out, is kept: no
stock layout forces an exact height, and the alternative depends on the parent
staying a Border.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
mixeme
2026-07-27 03:03:50 +03:00
parent 721b049100
commit 70aa4cbc4d
2 changed files with 392 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/GUI-LAYOUT-REVIEW.md
+386
View File
@@ -0,0 +1,386 @@
# 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).
docs/ROADMAP.md
+6
View File
@@ -83,6 +83,12 @@ Design notes / open questions:
### 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`