ykcheungab
eb70ac7638
ui-layout.md named the arrangement's owners but nothing knew which elements exist, so a view switch could silently take a running session's tracker reset away: docs/ui-elements.json now states all 195 by region, pane and view, ui/elements.py loads it, and tools/ui_elements_dump.py drafts it. Verified: apps/studio 393 items (13 new) and ui_check 600/0, autocal 358, shared 390 with its 2 known wall-clock failures, lint and doc gates clean; a removed element, a renamed objectName and a new named widget each fail the suite.
163 lines
9.9 KiB
Markdown
163 lines
9.9 KiB
Markdown
# Studio UI layout contract
|
|
|
|
Rules for the Studio window's layout, and for the tests that prove it. The colour equivalent is
|
|
[`ui-palette.md`](ui-palette.md); this file is the layout half.
|
|
|
|
Scope: `apps/studio`. The same rules apply to `apps/autocal` where it has panes of its own.
|
|
|
|
## 1. Ownership
|
|
|
|
| Concern | Owner |
|
|
|---|---|
|
|
| Top-level panel arrangement, presets, splitter sizes, which panes are visible | `view_layout.py` (`ViewLayoutController`) |
|
|
| A panel's internal layout, its own minimums, how it degrades when narrow | that panel's module |
|
|
| Colour and appearance | theme tokens + `build_qss` (see `ui-palette.md`) |
|
|
| Shared spacing and minimum sizes | the token module named in §4 |
|
|
|
|
A panel MUST NOT set the size of another panel, and MUST NOT reach into the splitter. If a panel
|
|
needs more room, it raises its own minimum and lets the controller allocate.
|
|
|
|
**One owner per region.** A region's geometry is decided in exactly one place. The `app_window.py`
|
|
extractions (`ViewLayoutController`, `DisplayPrefsController`) are how this is achieved; new layout
|
|
behaviour belongs in a controller, not in `app_window.py`.
|
|
|
|
## 2. View transitions
|
|
|
|
The three top-level views are arrangements of the *same* window. Switching between them is a single
|
|
operation with a fixed set of effects.
|
|
|
|
**Every path that changes the view MUST go through `apply_preset()`.** It is the only place that
|
|
applies the complete set, and `apply_preset` deliberately performs all of:
|
|
|
|
1. sets `view_preset` and the persisted preference;
|
|
2. sets `show_cal` from the preset;
|
|
3. sets the rail's source view (`replay` vs `live`);
|
|
4. sets the run-button role (`playback` vs `acquire`);
|
|
5. applies panel visibility;
|
|
6. applies the preset's splitter sizes;
|
|
7. switches the chart/replay source and clears replay-only state when leaving replay;
|
|
8. updates the brand bar, view checks and replay actions.
|
|
|
|
**MUST NOT** write `_view_preset`, the `view_preset` preference, or the splitter sizes directly.
|
|
Doing so produces a state that looks correct in the nav and wrong everywhere else — stale chart
|
|
source, wrong run role, sizes from the previous view.
|
|
|
|
### The transition that broke
|
|
|
|
`ViewLayoutController.on_cal_panel_toggled(False)` set `_view_preset = "live"` and persisted it
|
|
without calling `apply_preset("live")`. The sequence **replay → calibration → calibration off**
|
|
therefore ended with `view_preset == "live"` while the rail still reported the replay source and no
|
|
live splitter sizes had been requested. The live branch (`on_cal_panel_toggled(True)`) did call
|
|
`apply_preset`, which is what made the asymmetry easy to miss.
|
|
|
|
This transition MUST be covered by a regression test that asserts the whole effect set of §2, in the
|
|
controller and again through a real `StudioMainWindow`. Asserting only `view_preset`, `show_cal` and
|
|
the brand bar — as the existing toggle test does — is what let it through.
|
|
|
|
## 3. Per-view arrangement
|
|
|
|
Each view is described by what it is *for*, so priority disputes have a reference.
|
|
|
|
| View | Primary pane (gets width first) | Companion panes | Degradation as width shrinks, in order |
|
|
|---|---|---|---|
|
|
| `live` | measurement panes (2D/3D) | charts, settings rail | charts narrow to their minimum, then settings rail collapses, then charts scroll |
|
|
| `replay` | charts (history browsing is the point) | 2D pane, rail | 2D pane narrows, then hides where the layout allows |
|
|
| `calibration` | `cal_panel` — its six-column table and action row need width | rest | the table scrolls horizontally rather than compressing columns below readability |
|
|
|
|
The 3D pane is a peer of the 2D pane, never a replacement for it. Where both are visible they share
|
|
the measurement region; where only one is, it takes the region.
|
|
|
|
**Squeeze is a designed state, not an accident.** For each pane, name what happens at minimum width.
|
|
"Everything gets smaller" is not a degradation order.
|
|
|
|
## 4. Numeric geometry
|
|
|
|
Not every numeric argument is a defect; blanket replacement of numbers with names is churn. The rule
|
|
distinguishes by *reason*:
|
|
|
|
| Kind | Rule |
|
|
|---|---|
|
|
| **Structural constants** — 1 px separator, 1 px rule, a colour-bar's height, a native handle that must stay at zero size | Keep, with a named constant and a comment stating why the number is the value |
|
|
| **Shared spacing** — margins, gaps, padding shared by more than one panel | MUST come from the shared token module |
|
|
| **Content-driven width** — a field, a spin box, a label that holds text | MUST derive from `sizeHint()`, `fontMetrics()`, or a token for its role. A literal that must hold the longest locale string MUST be justified in a comment |
|
|
| **A flexible panel's own minimum** | MUST be a named constant with the reason recorded |
|
|
|
|
The token module (shared spacing and minimum sizes) does not exist yet; creating it is a follow-up.
|
|
Until it does, new shared spacing MUST NOT be introduced as a literal in more than one place.
|
|
|
|
**Text must fit.** Any control whose text can change with locale MUST hold the longest supported
|
|
string without clipping.
|
|
|
|
## 5. Enforcement gaps to close
|
|
|
|
These are stated because the contract is not currently enforced where it claims to be.
|
|
|
|
1. **The palette prohibition is not mechanically enforced.** `ui-palette.md` states that hex
|
|
literals are not allowed in widget code, but `ui_check.audit_palette()` reads the theme JSON and
|
|
checks values and contrast only — it never scans source. Hex literals exist in widget modules
|
|
today. Either add the source scan or correct the claim in `ui-palette.md`.
|
|
2. **The containment check is position-blind.** `test_ui_smoke.test_children_stay_inside_their_parent`
|
|
compares `child.width()/height()` against the parent's, and never looks at `x`/`y`. Since
|
|
`QWidget.geometry()` is parent-relative, a child shifted outside its parent — or overlapping a
|
|
sibling — passes as long as it is no larger. It MUST compare the full rectangle.
|
|
3. **Rendered 3D pixels are tested but conditionally.** `test_scene3d_gl_smoke.py` asserts on pixels
|
|
through `grabFramebuffer`, and skips without a real GL context. That is the right design; the gap
|
|
is that the skip is invisible in an ordinary run. Where the GL gate is required, a skip MUST be
|
|
reported as a failure of the run, not counted as a pass.
|
|
|
|
## 6. Tests
|
|
|
|
The existing suites stay: state assertions are cheap and catch real regressions. What is added is
|
|
assertions about the **visible result**.
|
|
|
|
| Tier | What it asserts | Where |
|
|
|---|---|---|
|
|
| **Transition** | the full effect set of §2, for every path that changes the view, including the breaking sequence | `test_view_layout.py` (controller) and a window-level test through `StudioMainWindow` |
|
|
| **Geometry** | full child rectangles inside parents, no sibling overlap among laid-out peers, required text fits | extend `test_ui_smoke.py` |
|
|
| **Matrix** | the above under combinations that historically broke: minimum window size, longest locale, both themes, sensor type, pane state | parametrised in `test_ui_smoke.py` |
|
|
| **Rendered** | a few pinned 2D and toolbar snapshots for human review | `ui_snapshot.py` output, reviewed not asserted |
|
|
| **GL pixels** | rendered 3D, in the real-context suite only | `test_scene3d_gl_smoke.py` |
|
|
|
|
**On waiting:** `processEvents()` dispatches pending paint events, so it is sufficient to let a
|
|
layout settle. It does not assert what was painted, and it does not guarantee that later
|
|
asynchronous work has finished. Tests that depend on completion MUST wait for the signal or
|
|
condition, not for a fixed number of `processEvents()` calls. `pytest-qt` makes that easier and is
|
|
not a prerequisite.
|
|
|
|
**Golden images, if adopted:** pin the window size, scale factor, font and theme, or they will flake
|
|
and be disabled. A flaky render gate is worse than none.
|
|
|
|
## 7. The element manifest
|
|
|
|
[`ui-elements.json`](ui-elements.json) is the machine-readable half of this contract. The rules
|
|
above say who owns the arrangement; the manifest says **which elements are supposed to exist**, per
|
|
region, per pane and per view. Each entry carries a stable `id`, the Qt `objectName` (recorded only
|
|
when it is a stable identity — the rail's transport carries a stylesheet token instead, so it is
|
|
found by path), the class, a one-line purpose, and the views it appears in. Anything that exists
|
|
only in some views, or only while a condition holds, declares `views` or a named `condition`
|
|
rather than leaving it ambiguous.
|
|
|
|
[`ui/elements.py`](../src/orisys_studio/ui/elements.py) loads it and answers the questions a test
|
|
needs: `missing()`, `wrong_class()`, `unlisted_named_widgets()` and `validate()`.
|
|
[`tests/test_ui_elements.py`](../tests/test_ui_elements.py) runs them against a real
|
|
`StudioMainWindow` in each view, and asserts the declared panes are the visible ones.
|
|
[`tools/ui_elements_dump.py`](../tools/ui_elements_dump.py) drafts the file from a live window.
|
|
|
|
**The manifest is authored, not generated.** A generated file accepts whatever the code happens to
|
|
contain, which is the drift it exists to catch; the dump is a drafting aid and its output must be
|
|
curated by hand. **An element is listed before a test relies on it** — that is what makes the file a
|
|
spec rather than a description of what someone found. The drift report (visible named widgets the
|
|
manifest accounts for neither as an element identity nor as a declared stylesheet token) is
|
|
compared with the record in each view, so new interface names fail until the manifest says what
|
|
they are.
|
|
|
|
Whoever changes the set of elements — adding, renaming or removing a widget the manifest covers —
|
|
updates the manifest in the same change, before the test is asked to rely on it.
|
|
|
|
This is the **element** tier of §6 and nothing more. It does not assert geometry or containment, it
|
|
does not run the locale/theme/minimum-size matrix, and it does not compare rendered pixels. Those
|
|
extend it rather than replace it: the geometry tier would add the rectangle a listed element is
|
|
allowed to occupy, the matrix tier would vary the window states the same list is checked under, and
|
|
the rendered tier would pin a reference image for the panes it names. None of that is attempted
|
|
here.
|