From ba1d9c523e881b538931b49acc43b92afd7a69a8 Mon Sep 17 00:00:00 2001 From: yaraslau Date: Thu, 22 Oct 2026 22:00:41 +0200 Subject: [PATCH 1/5] Expose dateTimeInput's mode through an options overload (#1024) dateTimeInput() was a positional function with no way to set mode, even though the document schema and interpreter already fully support it. Add DateTimeInputOptions, matching TextInputOptions/ FilePickerOptions's own pattern, alongside the existing positional overload. Co-Authored-By: Claude Sonnet 5 --- include/morph/ui/author.hpp | 27 +++++++++++++++++++++++++++ tests/test_ui_author.cpp | 7 +++++++ 2 files changed, 34 insertions(+) diff --git a/include/morph/ui/author.hpp b/include/morph/ui/author.hpp index fa88f6a32..1bfa4ffbe 100644 --- a/include/morph/ui/author.hpp +++ b/include/morph/ui/author.hpp @@ -3866,6 +3866,33 @@ template return detail::NodeWriter{"dateTimeInput"}.put("value", value).put("label", label).put("onCommit", commit).done(); } +/// @brief Options of `dateTimeInput`. +struct DateTimeInputOptions { + /// @brief The value. + AnyHandle value; + /// @brief The label: the accessible name. + AnyHandle label; + /// @brief The input mode, such as `date` or `time`. + AnyHandle mode; + /// @brief Commands run on every edit. + Commands onChange; + /// @brief Commands run on a commit. + Commands onCommit; +}; + +/// @brief A `dateTimeInput` node. +/// @param options The input. +/// @return The node. +[[nodiscard]] inline Node dateTimeInput(DateTimeInputOptions const& options) { + return detail::NodeWriter{"dateTimeInput"} + .put("value", options.value) + .put("label", options.label) + .put("mode", options.mode) + .put("onChange", options.onChange) + .put("onCommit", options.onCommit) + .done(); +} + /// @brief Options of `slider`. struct SliderOptions { /// @brief The value. diff --git a/tests/test_ui_author.cpp b/tests/test_ui_author.cpp index 64b3cbf54..5db8fbc75 100644 --- a/tests/test_ui_author.cpp +++ b/tests/test_ui_author.cpp @@ -1225,6 +1225,13 @@ TEST_CASE("options left unset write no key", "[ui][author]") { R"({"kind":"table","model":"sample","action":"Lims_ListProjects"})"); } +TEST_CASE("dateTimeInput's options overload exposes mode, unlike the positional one", "[ui][author]") { + CHECK(text(ui::dateTimeInput(ui::ref("choice"), "When")) == + R"({"kind":"dateTimeInput","value":{"ref":"choice"},"label":"When"})"); + CHECK(text(ui::dateTimeInput({.value = ui::ref("choice"), .label = "When", .mode = "date"})) == + R"({"kind":"dateTimeInput","value":{"ref":"choice"},"label":"When","mode":"date"})"); +} + TEST_CASE("a scope that declares one kind of thing writes it", "[ui][author]") { auto const emitted = ui::screen("author.scopes", [](ui::Scope& screen) { auto sample = screen.model("sample", {.instance = ui::Instance::Private}); From 7afd8ace42a2b56dffc4e2b0b04e451e6c88e075 Mon Sep 17 00:00:00 2001 From: yaraslau Date: Thu, 22 Oct 2026 22:19:48 +0200 Subject: [PATCH 2/5] Add ButtonEmphasis::Ghost for a chrome-free, embedded action (#1028) A ghost button has no border or fill, minimal (space.1) padding, and a faint neutral tint on hover/press, for an action embedded in other content (a table row's remove, a disclosure chevron) where full button chrome reads as a second control beside it rather than part of it. Touches the enum, both document-schema emphasis arrays, both interpreter string-to-enum chains, both author.hpp enum-to-string chains, the Qt Quick enum-to-QML-int mapping (the one site that actually renders the feature), MorphButton.qml/ MorphButtonBackground.qml (including the implicitWidth floor, which would otherwise keep every ghost button at the same width as a filled one), and the TUI fallback-to-plain-style path. Co-Authored-By: Claude Sonnet 5 --- docs/spec/ui/backend_contract.md | 11 ++++++---- docs/spec/ui/document.md | 4 ++-- include/morph/ui/author.hpp | 4 ++++ include/morph/ui/button_emphasis.hpp | 1 + include/morph/ui/document.hpp | 4 ++-- include/morph/ui/interpreter.hpp | 4 ++++ .../morph/ui/testing/recording_backend.hpp | 2 ++ src/qt_quick/leaves.cpp | 4 +++- src/qt_quick/qml/MorphButton.qml | 21 ++++++++++++------- src/qt_quick/qml/MorphButtonBackground.qml | 12 +++++++---- src/tui/leaf_widgets.cpp | 5 +++-- tests/qt_quick/test_qt_quick_widgets.cpp | 20 +++++++++++++++--- tests/test_ui_author.cpp | 12 +++++++++++ tests/test_ui_document.cpp | 5 +++-- tests/test_ui_mount.cpp | 6 ++++-- tests/tui/test_tui_widgets.cpp | 9 ++++++-- 16 files changed, 92 insertions(+), 32 deletions(-) diff --git a/docs/spec/ui/backend_contract.md b/docs/spec/ui/backend_contract.md index 3c9a925d5..43df4ea6b 100644 --- a/docs/spec/ui/backend_contract.md +++ b/docs/spec/ui/backend_contract.md @@ -134,10 +134,13 @@ reorders children, and only through this. value with `spacingStep`, so a backend never sees another. Each backend maps a step to its own distance: the theme's `space.` in px on Qt Quick, cells by axis on the terminal. -**Button emphasis.** `ButtonWidget::setEmphasis(ButtonEmphasis)` takes `Normal`, `Primary` or `Danger`; the mount -calls it only for another value than `Normal`, so a document without the `emphasis` key (`normal`, `primary`, -`danger`) records nothing new. Qt Quick fills a primary button in the accent and a danger one in the error colour; -the terminal draws both bold, a danger one in the error colour. +**Button emphasis.** `ButtonWidget::setEmphasis(ButtonEmphasis)` takes `Normal`, `Primary`, `Danger` or `Ghost`; the +mount calls it only for another value than `Normal`, so a document without the `emphasis` key (`normal`, `primary`, +`danger`, `ghost`) records nothing new. Qt Quick fills a primary button in the accent and a danger one in the error +colour, and draws a ghost button with no border or fill, minimal padding, and a faint neutral tint on hover or press +— for an action embedded in other content (a table row's remove, a disclosure chevron) where full button chrome would +read as a second control beside it rather than part of it. The terminal draws `Primary`/`Danger` bold, a danger one +in the error colour, and falls back to `Normal`'s plain style for `Ghost`. **Button style and selection.** `ButtonWidget::setStyle(ButtonStyle)` makes a button an entry of a list (`Item`): it behaves as a push button (activation, enablement, tooltip, hover) and is drawn as a line, as wide as its line, its diff --git a/docs/spec/ui/document.md b/docs/spec/ui/document.md index 842609533..57f96d559 100644 --- a/docs/spec/ui/document.md +++ b/docs/spec/ui/document.md @@ -95,7 +95,7 @@ An `actionButton` part is one button of a list: | `id` | name, required | unique within its list | | `label` | expression, required | | | `icon` | expression | an icon name | -| `emphasis` | text | `normal`, `primary`, `danger`, as a `button`'s | +| `emphasis` | text | `normal`, `primary`, `danger`, `ghost`, as a `button`'s | | `enabled`, `visible` | conditions | default `true` | | `onClick` | commands | run in the scope the list belongs to | @@ -162,7 +162,7 @@ left out being content: `"sizing": {"lit": {"width": {"stretch": 1}, "height": { | Kind | Keys (a ⓘ key is in the node's own scope) | Element values beyond `visible`, `enabled`, `valid`, `errors`, `shownErrors` | |---|---|---| | `text` | `text`, `role` (`normal`, the default, `muted`, `heading` (`title` is accepted as its alias), `section`, `error`, `success`) | | -| `button` | `label`, `icon`, `emphasis` (`normal`, `primary`, `danger`), `style` (`push`, the default, or `item`: an entry of a list), `selected` (a condition, only with `style: item`: the entry is the current one), `onClick`, `onActivate` | | +| `button` | `label`, `icon`, `emphasis` (`normal`, `primary`, `danger`, `ghost`), `style` (`push`, the default, or `item`: an entry of a list), `selected` (a condition, only with `style: item`: the entry is the current one), `onClick`, `onActivate` | | | `textInput` | `value`, `placeholder`, `mode`, `icon` (an icon of the theme's icon set, drawn at the leading edge; refused on a multiline input), `commitOn` (`change`, `blur`, `enter`), `label`, `showLabel` (a literal `bool`; default `false`), `onChange`, `onCommit`, `onSubmit` | `value`, `draft`, `dirty`, `focused`, `touched` | | `checkbox` | `label`, `checked`, `onToggle`, `onChange` | `checked` | | `select` | `options`, `selected`, `style`, `label`, `showLabel`, `onStale` (`clear`, `keep`), `onChoose`, `onChange` | `selected`, `selectedOption` | diff --git a/include/morph/ui/author.hpp b/include/morph/ui/author.hpp index 1bfa4ffbe..a5572e908 100644 --- a/include/morph/ui/author.hpp +++ b/include/morph/ui/author.hpp @@ -2463,6 +2463,8 @@ inline void putPresent(Json& json, char const* key, AnyHandle const& value) { detail::put(json, "emphasis", std::string{"primary"}); } else if (action.emphasis == ButtonEmphasis::Danger) { detail::put(json, "emphasis", std::string{"danger"}); + } else if (action.emphasis == ButtonEmphasis::Ghost) { + detail::put(json, "emphasis", std::string{"ghost"}); } putPresent(json, "enabled", action.enabled); putPresent(json, "visible", action.visible); @@ -3157,6 +3159,8 @@ struct ButtonOptions { writer.put("emphasis", std::string{"primary"}); } else if (options.emphasis == ButtonEmphasis::Danger) { writer.put("emphasis", std::string{"danger"}); + } else if (options.emphasis == ButtonEmphasis::Ghost) { + writer.put("emphasis", std::string{"ghost"}); } if (options.style) { writer.text("style", *options.style == ButtonStyle::Item ? "item" : "push"); diff --git a/include/morph/ui/button_emphasis.hpp b/include/morph/ui/button_emphasis.hpp index 863717523..8181cb114 100644 --- a/include/morph/ui/button_emphasis.hpp +++ b/include/morph/ui/button_emphasis.hpp @@ -14,6 +14,7 @@ enum class ButtonEmphasis : std::uint8_t { Normal, ///< An ordinary action. Primary, ///< The screen's main action. Danger, ///< An action that destroys or cannot be undone. + Ghost, ///< An action embedded in other content, with no chrome of its own. }; } // namespace morph::ui diff --git a/include/morph/ui/document.hpp b/include/morph/ui/document.hpp index 94b17c50f..61c2a58c8 100644 --- a/include/morph/ui/document.hpp +++ b/include/morph/ui/document.hpp @@ -2975,7 +2975,7 @@ class DocumentValidator { // The keys of a kind that take one of a closed set of values, and the pairs of keys that need each other. static void checkKindValues(DocNode const& node) { if (node.kind == "button") { - static constexpr std::array kEmphasis = {"normal", "primary", "danger"}; + static constexpr std::array kEmphasis = {"normal", "primary", "danger", "ghost"}; checkChoice(node, "emphasis", kEmphasis); static constexpr std::array kStyles = {"push", "item"}; checkChoice(node, "style", kStyles); @@ -3062,7 +3062,7 @@ class DocumentValidator { // A list of `actionButton` parts: each id once, and each emphasis one of a button's. static void checkActionButtons(std::span buttons, std::string const& path) { - static constexpr std::array kEmphasis = {"normal", "primary", "danger"}; + static constexpr std::array kEmphasis = {"normal", "primary", "danger", "ghost"}; std::set ids; for (DocNodePtr const& button : buttons) { if (!ids.insert(button->id).second) { diff --git a/include/morph/ui/interpreter.hpp b/include/morph/ui/interpreter.hpp index b8bbe5e90..83a9a949d 100644 --- a/include/morph/ui/interpreter.hpp +++ b/include/morph/ui/interpreter.hpp @@ -5806,6 +5806,8 @@ inline ActionModels ScreenCore::lowerActionParts(std::span par model.emphasis = ButtonEmphasis::Primary; } else if (part->name("emphasis") == "danger") { model.emphasis = ButtonEmphasis::Danger; + } else if (part->name("emphasis") == "ghost") { + model.emphasis = ButtonEmphasis::Ghost; } if (Commands const* const commands = part->event("onClick")) { model.run = [this, ctx, commands] { runEvent(commands, ctx, Value{}); }; @@ -5922,6 +5924,8 @@ inline Node ScreenCore::lowerKind(DocNode const& node, LowerCtx const& ctx, Elem spec.emphasis = ButtonEmphasis::Primary; } else if (node.name("emphasis") == "danger") { spec.emphasis = ButtonEmphasis::Danger; + } else if (node.name("emphasis") == "ghost") { + spec.emphasis = ButtonEmphasis::Ghost; } if (node.name("style") == "item") { spec.style = ButtonStyle::Item; diff --git a/include/morph/ui/testing/recording_backend.hpp b/include/morph/ui/testing/recording_backend.hpp index f70697b4c..5404d5663 100644 --- a/include/morph/ui/testing/recording_backend.hpp +++ b/include/morph/ui/testing/recording_backend.hpp @@ -249,6 +249,8 @@ template return "Primary"; case ButtonEmphasis::Danger: return "Danger"; + case ButtonEmphasis::Ghost: + return "Ghost"; case ButtonEmphasis::Normal: default: return "Normal"; diff --git a/src/qt_quick/leaves.cpp b/src/qt_quick/leaves.cpp index 60f644cde..5ebc63c29 100644 --- a/src/qt_quick/leaves.cpp +++ b/src/qt_quick/leaves.cpp @@ -62,12 +62,14 @@ void ButtonImpl::setIcon(std::string_view icon) { } void ButtonImpl::setEmphasis(ui::ButtonEmphasis emphasis) { - // MorphButton's `emphasis`: 0 normal, 1 primary, 2 danger. + // MorphButton's `emphasis`: 0 normal, 1 primary, 2 danger, 3 ghost. int shown = 0; if (emphasis == ui::ButtonEmphasis::Primary) { shown = 1; } else if (emphasis == ui::ButtonEmphasis::Danger) { shown = 2; + } else if (emphasis == ui::ButtonEmphasis::Ghost) { + shown = 3; } write("emphasis", shown); } diff --git a/src/qt_quick/qml/MorphButton.qml b/src/qt_quick/qml/MorphButton.qml index 49ca484a6..0a86765fe 100644 --- a/src/qt_quick/qml/MorphButton.qml +++ b/src/qt_quick/qml/MorphButton.qml @@ -14,14 +14,18 @@ MorphWidget { property string label: "" // An icon of the theme's icon set, drawn beside the label when the set names it; set once. property string icon: "" - // 0 normal (outlined), 1 primary (accent-filled), 2 danger (error-filled); set once. + // 0 normal (outlined), 1 primary (accent-filled), 2 danger (error-filled), 3 ghost (no chrome of its own); set + // once. property int emphasis: 0 // 0 push, 1 item; set once. property int style: 0 // Whether an item is the current entry of its list; a push button is never told. property bool selected: false - readonly property color fill: emphasis === 1 ? root.scheme.accent : emphasis === 2 ? root.scheme.error - : root.scheme.surface + readonly property bool ghost: emphasis === 3 + readonly property color fill: emphasis === 1 ? root.scheme.accent + : emphasis === 2 ? root.scheme.error + : ghost ? "transparent" + : root.scheme.surface // The widget's item is the one node a driver finds by test id, so it is the button: it carries the role, the // label as its name and the press action, and the control inside it is not a second node. @@ -69,17 +73,18 @@ MorphWidget { id: button anchors.fill: parent text: root.label - leftPadding: root.style === 1 ? Theme.spaceOf(3) : Theme.spaceOf(4) - rightPadding: root.style === 1 ? Theme.spaceOf(3) : Theme.spaceOf(4) - palette.buttonText: root.emphasis === 0 ? root.scheme.text : "white" - font: root.emphasis === 0 ? Theme.bodyFont : Theme.strongFont + leftPadding: root.ghost ? Theme.spaceOf(1) : root.style === 1 ? Theme.spaceOf(3) : Theme.spaceOf(4) + rightPadding: root.ghost ? Theme.spaceOf(1) : root.style === 1 ? Theme.spaceOf(3) : Theme.spaceOf(4) + palette.buttonText: root.emphasis === 0 || root.ghost ? root.scheme.text : "white" + font: root.emphasis === 0 || root.ghost ? Theme.bodyFont : Theme.strongFont icon.source: Theme.iconSource(root.icon) icon.color: "transparent" background: MorphButtonBackground { scheme: root.scheme fill: root.fill - filled: root.emphasis !== 0 + filled: root.emphasis === 1 || root.emphasis === 2 item: root.style === 1 + ghost: root.ghost selected: root.selected hovered: button.hovered pressed: button.down diff --git a/src/qt_quick/qml/MorphButtonBackground.qml b/src/qt_quick/qml/MorphButtonBackground.qml index 865257bad..51a1ae8e4 100644 --- a/src/qt_quick/qml/MorphButtonBackground.qml +++ b/src/qt_quick/qml/MorphButtonBackground.qml @@ -3,7 +3,8 @@ import QtQuick // What a button is drawn on: the control height and radius, outlined on the surface colour, a shade darker while // hovered or pressed. A list item's is unframed: clear, the neutral colour while hovered, and while selected an accent -// tint with a bar at its leading edge. +// tint with a bar at its leading edge. A ghost button is unframed the same way but has no "current" concept: just a +// neutral tint while hovered or pressed, transparent otherwise. Rectangle { id: background @@ -18,13 +19,16 @@ Rectangle { // A list item, and whether it is the current entry. property bool item: false property bool selected: false + // A ghost button: no border or fill, no accent-bar "current" concept, just a neutral tint on hover or press. + property bool ghost: false implicitHeight: Theme.controlHeight - implicitWidth: Theme.unitWidth * 8 + implicitWidth: ghost ? 0 : Theme.unitWidth * 8 radius: Theme.radiusControl color: item ? (selected ? Qt.alpha(scheme.accent, 0.12) : hovered || pressed ? scheme.neutral : "transparent") - : pressed ? Qt.darker(fill, 1.12) : hovered ? Qt.darker(fill, 1.05) : fill - border.width: focused ? 2 : filled || item ? 0 : 1 + : ghost ? (hovered || pressed ? scheme.neutral : "transparent") + : pressed ? Qt.darker(fill, 1.12) : hovered ? Qt.darker(fill, 1.05) : fill + border.width: focused ? 2 : filled || item || ghost ? 0 : 1 border.color: focused ? scheme.focus : scheme.border Rectangle { diff --git a/src/tui/leaf_widgets.cpp b/src/tui/leaf_widgets.cpp index 1cf2f2d40..6fd24ed8f 100644 --- a/src/tui/leaf_widgets.cpp +++ b/src/tui/leaf_widgets.cpp @@ -142,9 +142,10 @@ ::core::tui::Size ButtonImpl::naturalSize() const { } void ButtonImpl::paint(::core::tui::Canvas& canvas) { - // Primary is bold, danger bold in the error colour; neither changes a disabled button's look. + // Primary is bold, danger bold in the error colour; ghost falls back to Normal's plain style; neither changes a + // disabled button's look. ::core::tui::Style style = buttonStyle(canvas.theme(), actionable(), hasFocus()); - if (actionable() && _emphasis != ui::ButtonEmphasis::Normal) { + if (actionable() && _emphasis != ui::ButtonEmphasis::Normal && _emphasis != ui::ButtonEmphasis::Ghost) { style.bold = true; if (_emphasis == ui::ButtonEmphasis::Danger) { style.fg = canvas.theme().error.fg; diff --git a/tests/qt_quick/test_qt_quick_widgets.cpp b/tests/qt_quick/test_qt_quick_widgets.cpp index 75027983d..fc9dce6b8 100644 --- a/tests/qt_quick/test_qt_quick_widgets.cpp +++ b/tests/qt_quick/test_qt_quick_widgets.cpp @@ -660,28 +660,42 @@ TEST_CASE("qt_quick widgets: a widget's width class follows the host's width", " CHECK(classes == std::vector{"compact", "medium", "expanded", "compact"}); } -TEST_CASE("qt_quick widgets: a primary button is accent-filled, a danger one error-filled", "[qt_quick]") { +TEST_CASE( + "qt_quick widgets: a primary button is accent-filled, a danger one error-filled, a ghost one transparent " + "and narrower", + "[qt_quick]") { Scene scene; auto const column = scene.backend().createStack(nullptr, ui::Axis::Vertical); auto plain = scene.backend().createButton(column.get()); auto primary = scene.backend().createButton(column.get()); auto danger = scene.backend().createButton(column.get()); + auto ghost = scene.backend().createButton(column.get()); plain->setLabel("Plain"); primary->setLabel("Go"); primary->setEmphasis(ui::ButtonEmphasis::Primary); danger->setLabel("Drop"); danger->setEmphasis(ui::ButtonEmphasis::Danger); + ghost->setLabel("X"); + ghost->setEmphasis(ui::ButtonEmphasis::Ghost); scene.settle(); - auto const background = [](ui::ButtonWidget& button) { + auto const backgroundItem = [](ui::ButtonWidget& button) { QQuickItem const* const control = Scene::itemsOf(Scene::itemOf(button), "QQuickButton").value(0); REQUIRE(control != nullptr); auto* const fill = control->property("background").value(); REQUIRE(fill != nullptr); - return fill->property("color").value(); + return fill; + }; + auto const background = [&backgroundItem](ui::ButtonWidget& button) { + return backgroundItem(button)->property("color").value(); }; CHECK(background(*plain) == QColor{"#ffffff"}); CHECK(background(*primary) == QColor{"#1565c0"}); CHECK(background(*danger) == QColor{"#c62828"}); + CHECK(background(*ghost) == QColor{"transparent"}); + // Confirms the implicitWidth floor is lifted for Ghost, not merely that its padding shrank: a plain button keeps + // its 8-unit background floor regardless of its one-character label, so a ghost button of the same label must + // measure visibly narrower, not just differently padded within the same floor. + CHECK(Scene::itemOf(*ghost).implicitWidth() < Scene::itemOf(*plain).implicitWidth()); } // Whether an Item-style button already composes with a trailing Badge inside a row, or whether the button's own diff --git a/tests/test_ui_author.cpp b/tests/test_ui_author.cpp index 5db8fbc75..7de37e09e 100644 --- a/tests/test_ui_author.cpp +++ b/tests/test_ui_author.cpp @@ -1232,6 +1232,18 @@ TEST_CASE("dateTimeInput's options overload exposes mode, unlike the positional R"({"kind":"dateTimeInput","value":{"ref":"choice"},"label":"When","mode":"date"})"); } +// ui::button() (not ui::ActionButtonOptions's actionButton()) has its own, separate emphasis-to-string mapping; a +// ghost button must write "ghost" from this site too, not just from the actionButton()/screen.action() path. +TEST_CASE("a button's own emphasis is written, including ghost", "[ui][author]") { + CHECK(text(ui::button({.label = "Plain"})) == R"({"kind":"button","label":"Plain"})"); + CHECK(text(ui::button({.label = "Go", .emphasis = morph::ui::ButtonEmphasis::Primary})) == + R"({"kind":"button","emphasis":"primary","label":"Go"})"); + CHECK(text(ui::button({.label = "Drop", .emphasis = morph::ui::ButtonEmphasis::Danger})) == + R"({"kind":"button","emphasis":"danger","label":"Drop"})"); + CHECK(text(ui::button({.label = "Remove", .emphasis = morph::ui::ButtonEmphasis::Ghost})) == + R"({"kind":"button","emphasis":"ghost","label":"Remove"})"); +} + TEST_CASE("a scope that declares one kind of thing writes it", "[ui][author]") { auto const emitted = ui::screen("author.scopes", [](ui::Scope& screen) { auto sample = screen.model("sample", {.instance = ui::Instance::Private}); diff --git a/tests/test_ui_document.cpp b/tests/test_ui_document.cpp index 808165509..92f6f8330 100644 --- a/tests/test_ui_document.cpp +++ b/tests/test_ui_document.cpp @@ -249,11 +249,12 @@ TEST_CASE("ui document: a refusal names the document", "[ui][document]") { CHECK(std::ranges::find(kinds, "menuItem") == kinds.end()); } -TEST_CASE("ui document: a button's emphasis is normal, primary or danger", "[ui][document]") { +TEST_CASE("ui document: a button's emphasis is normal, primary, danger or ghost", "[ui][document]") { CHECK(loadDocumentText(R"({"vocab": "ui/1", "id": "e", "view": {"kind": "column", "children": [ {"kind": "button", "label": "a", "emphasis": "primary"}, {"kind": "button", "label": "b", "emphasis": "danger"}, - {"kind": "button", "label": "c", "emphasis": "normal"}]}})") + {"kind": "button", "label": "c", "emphasis": "normal"}, + {"kind": "button", "label": "d", "emphasis": "ghost"}]}})") .has_value()); auto const refused = loadDocumentText( R"({"vocab": "ui/1", "id": "e", "view": {"kind": "button", "label": "a", "emphasis": "loud"}})"); diff --git a/tests/test_ui_mount.cpp b/tests/test_ui_mount.cpp index 011ad7173..1dea7734c 100644 --- a/tests/test_ui_mount.cpp +++ b/tests/test_ui_mount.cpp @@ -135,12 +135,14 @@ TEST_CASE("ui::Mounted: a button's emphasis reaches the backend only when it is runtime, backend, ui::row({.children = {ui::button({.label = "Plain"}), ui::button({.label = "Go", .emphasis = ui::ButtonEmphasis::Primary}), - ui::button({.label = "Drop", .emphasis = ui::ButtonEmphasis::Danger})}})}; + ui::button({.label = "Drop", .emphasis = ui::ButtonEmphasis::Danger}), + ui::button({.label = "Remove", .emphasis = ui::ButtonEmphasis::Ghost})}})}; CHECK(backend.dump() == "Row#1 gap=0\n" " Button#2 label=Plain\n" " Button#3 emphasis=Primary label=Go\n" - " Button#4 emphasis=Danger label=Drop\n"); + " Button#4 emphasis=Danger label=Drop\n" + " Button#5 emphasis=Ghost label=Remove\n"); } // Mutation: skip checkLayout (every node below mounts). diff --git a/tests/tui/test_tui_widgets.cpp b/tests/tui/test_tui_widgets.cpp index 4cd75f0c7..9d69766a6 100644 --- a/tests/tui/test_tui_widgets.cpp +++ b/tests/tui/test_tui_widgets.cpp @@ -623,8 +623,9 @@ TEST_CASE("tui widgets: a press that reaches no widget ends a press that lost it CHECK(clicks == 1); } -TEST_CASE("tui widgets: a primary button is bold, a danger one bold in the error colour", "[tui][widgets]") { - Harness harness{30, 3}; +TEST_CASE("tui widgets: a primary button is bold, a danger one bold in the error colour, a ghost one plain", + "[tui][widgets]") { + Harness harness{30, 4}; auto const column = harness.make(nullptr, ui::Axis::Vertical); auto const plain = harness.make(column.get()); plain->setLabel("Plain"); @@ -634,12 +635,16 @@ TEST_CASE("tui widgets: a primary button is bold, a danger one bold in the error auto const danger = harness.make(column.get()); danger->setLabel("Drop"); danger->setEmphasis(ui::ButtonEmphasis::Danger); + auto const ghost = harness.make(column.get()); + ghost->setLabel("Remove"); + ghost->setEmphasis(ui::ButtonEmphasis::Ghost); static_cast(harness.draw()); auto const& buffer = harness.screen().renderedBuffer(); CHECK_FALSE(buffer.at(0, 2).style.bold); CHECK(buffer.at(1, 2).style.bold); CHECK(buffer.at(2, 2).style.bold); CHECK(buffer.at(2, 2).style.fg == harness.screen().theme().error.fg); + CHECK_FALSE(buffer.at(3, 2).style.bold); } // Mutation: map Section to Normal in roleStyle (the section is neither bold nor muted). From 651de328ccc4cb584569cf2b0ee84163107c0245 Mon Sep 17 00:00:00 2001 From: yaraslau Date: Thu, 22 Oct 2026 22:30:24 +0200 Subject: [PATCH 3/5] Give dialog and drawer an explicit-width lever, breaking their circular inference (#1026) MorphDrawer.qml and MorphDialog.qml both infer their width from their own content's implicit size, which reports back roughly the width the content was just given rather than what it actually needs -- a drawer settles at its 30-unit floor, a dialog settles wherever the first measurement pass happened to land. A document's own `sizing` now wins outright when given: Fixed on the drawer binds Drawer.width directly; on the dialog, both the Popup's own width and contentColumn's tracking of popup.availableWidth must change together, since contentItem's width is only re-asserted by Qt's own resizeContent() from specific trigger points, not from a plain change of its implicit size. Both overlays' root is only a zero-sized placeholder for the real on-screen overlay; without an explicit Layout.preferredWidth/Height override, the inherited MorphWidget binding would make that invisible placeholder claim real space in whatever layout holds the node. Stretch sizing is refused on both node kinds -- not silently ignored -- since a stretching overlay placeholder would change how every sibling in its own parent layout is justified. Covers the document's own literal check and, in one place, both the document's computed-expression path and a C++ author calling ui::dialog()/ ui::drawer() directly, since both converge at the same mountKind overload before either backend sees the result. Co-Authored-By: Claude Sonnet 5 --- include/morph/ui/document.hpp | 29 ++++++++++++++ include/morph/ui/mount.hpp | 22 +++++++++++ scripts/branch_partial_allowlist.json | 4 +- src/qt_quick/qml/MorphDialog.qml | 24 +++++++++++- src/qt_quick/qml/MorphDrawer.qml | 23 ++++++++--- tests/qt_quick/test_qt_quick_overlays.cpp | 47 +++++++++++++++++++++++ tests/test_ui_document.cpp | 15 ++++++++ tests/test_ui_mount.cpp | 36 +++++++++++++++++ 8 files changed, 192 insertions(+), 8 deletions(-) diff --git a/include/morph/ui/document.hpp b/include/morph/ui/document.hpp index 61c2a58c8..f2a9cf421 100644 --- a/include/morph/ui/document.hpp +++ b/include/morph/ui/document.hpp @@ -3138,12 +3138,41 @@ class DocumentValidator { return bounds; } + // Whether a literal sizing dimension's value is `{"stretch": w}` (as opposed to `"content"` or `{"fixed": n}`, + // which both `sizingDimension` reports the same way -- as "no fixed amount"). An overlay is sized by the + // frontend, not by its container, so stretching it would silently change how every *other* sibling in its own + // parent layout is justified; checked ahead of `sizingDimension` itself, which does not distinguish the two. + static bool isStretch(Value const& value) { + ValueRecord const* const record = value.asRecord(); + return record != nullptr && record->size() == 1 && record->begin()->first == "stretch"; + } + + // An overlay (`dialog`/`drawer`) refuses a literal `sizing` that stretches either dimension, single-value or + // record form: an overlay is sized by the frontend, not by its container, so stretching it would silently change + // how every *other* sibling in its own parent layout is justified. + static void checkOverlaySizing(DocNode const& node, ExprPtr const& sizing) { + if (node.kind != "dialog" && node.kind != "drawer") { + return; + } + ValueRecord const* const record = sizing->literal.asRecord(); + bool const stretches = + isStretch(sizing->literal) || + (record != nullptr && (record->contains("width") || record->contains("height")) && + std::ranges::any_of(*record, [](auto const& entry) { return isStretch(entry.second); })); + if (stretches) { + refuse(sizing->path, + "an overlay is sized by the frontend, not by its container; give it `{\"fixed\": n}` instead " + "of `{\"stretch\": w}`"); + } + } + // A literal `sizing` is one of its forms, and a fixed amount lies inside its dimension's @p bounds. static void checkSizing(DocNode const& node, std::array, 4> const& bounds) { ExprPtr const sizing = node.prop("sizing"); if (sizing == nullptr || sizing->kind != ExprKind::Literal) { return; } + checkOverlaySizing(node, sizing); std::optional width; std::optional height; ValueRecord const* const record = sizing->literal.asRecord(); diff --git a/include/morph/ui/mount.hpp b/include/morph/ui/mount.hpp index 12c4b802b..a0125ae25 100644 --- a/include/morph/ui/mount.hpp +++ b/include/morph/ui/mount.hpp @@ -501,6 +501,26 @@ class Mounter { check(hints.height, hints.minHeight, hints.maxHeight, "height"); } + // A dialog or a drawer is sized by the frontend, not by its container: Stretch sizing on one would silently + // change how every *other* sibling in its own parent layout is justified. Covers both the document path's own + // computed (non-literal) `sizing` -- resolved to a Sizing only here, at mount time, since the document-level + // checkSizing validates only a literal one -- and a C++ author calling ui::dialog()/ui::drawer() directly, both + // of which reach this same mountKind overload before either backend ever sees the result. + static void checkOverlayLayout(Common const& common, std::string_view kind) { + LayoutHints const& hints = common.layout; + auto const refuse = [&common, kind](std::string const& axis) { + throw std::logic_error{"morph::ui::mount: a " + std::string{kind} + + " is sized by the frontend, not by its container; its " + axis + " cannot stretch" + + (common.testId.empty() ? std::string{} : " (node `" + common.testId + "`)")}; + }; + if (hints.width.kind == Sizing::Kind::Stretch) { + refuse("width"); + } + if (hints.height.kind == Sizing::Kind::Stretch) { + refuse("height"); + } + } + // The loader's rule for `alignSelf`, for a view tree built in C++: only a stack's or a grid's child has a cross // axis to be placed on. static void checkPlacement(Common const& common, ContainerWidget const* parent) { @@ -1022,12 +1042,14 @@ class Mounter { // with `open` unchanged re-sends `setOpen`, and mounts content only if none is mounted: once per run while `open` // stays true over content that failed. Widget& mountKind(reactive::Scope& scope, Dialog const& spec, ContainerWidget* parent) { + checkOverlayLayout(spec.common, "dialog"); DialogWidget& widget = scope.adopt(_backend->createDialog(parent)); applyCommon(scope, widget, spec.common); return mountOverlay(scope, spec, widget); } Widget& mountKind(reactive::Scope& scope, Drawer const& spec, ContainerWidget* parent) { + checkOverlayLayout(spec.common, "drawer"); DrawerWidget& widget = scope.adopt(_backend->createDrawer(parent, spec.side)); applyCommon(scope, widget, spec.common); return mountOverlay(scope, spec, widget); diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index 72d4b3ca7..e8eca080c 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -135,14 +135,14 @@ }, { "file": "include/morph/ui/document.hpp", - "line": 3329, + "line": 3358, "source": "default:", "context": "if (cmd.value) {", "reason": "The default arm is unreachable: the switch names every enumerator of CommandKind, so no value reaches `default`. The project's strict warnings require both at once -- -Wswitch-enum wants every enumerator listed and -Wswitch-default wants a default label -- so the default shares the last enumerator's body and is never the label taken. It is DocumentValidator::checkCommand's switch." }, { "file": "include/morph/ui/document.hpp", - "line": 3664, + "line": 3693, "source": "default: {", "reason": "The default arm is unreachable: the switch names every enumerator of NameCategory, so no value reaches `default`. The project's strict warnings require both at once -- -Wswitch-enum wants every enumerator listed and -Wswitch-default wants a default label -- so the default shares the last enumerator's body and is never the label taken. It is ScopeResolver::ref's switch over what a name declares." }, diff --git a/src/qt_quick/qml/MorphDialog.qml b/src/qt_quick/qml/MorphDialog.qml index f4fb76871..91b2b966f 100644 --- a/src/qt_quick/qml/MorphDialog.qml +++ b/src/qt_quick/qml/MorphDialog.qml @@ -37,6 +37,15 @@ MorphWidget { focusTarget: column implicitWidth: 0 implicitHeight: 0 + // root is only a zero-sized placeholder for the Popup overlay below, which is reparented to the Overlay; without + // these, MorphWidget's own Layout.preferredWidth/Height (bound from widthKind/heightKind) would make this + // invisible placeholder claim real space in whatever layout holds the dialog node, independent of the overlay's + // own on-screen width -- which a document setting `sizing` to get the explicit-width lever below would otherwise + // trigger silently. + Layout.preferredWidth: -1 + Layout.fillWidth: false + Layout.preferredHeight: -1 + Layout.fillHeight: false Popup { id: popup @@ -44,6 +53,13 @@ MorphWidget { modal: true focus: true closePolicy: Popup.CloseOnEscape | Popup.CloseOnPressOutside + // An explicit width (widthKind Fixed) wins outright over the Popup's own default sizing. The fallback branch + // evaluates to `undefined`, not a captured number, so it resets rather than sets the Popup's own `hasWidth` + // state -- a bound value here, even one equal to the Popup's own implicitWidth, would otherwise silently + // disable QQuickPopupPositioner's window-edge clamping for every dialog, not just ones using the new lever. + width: root.widthKind === 1 + ? Math.min(root.widthAmount * Theme.unitWidth, (Overlay.overlay?.width ?? Infinity) - 4 * Theme.unitWidth) + : undefined onAboutToHide: { if (!root.programmatic) root.fire("dismiss", undefined) @@ -55,7 +71,13 @@ MorphWidget { // Math.min only ever shrinks, it does not stretch a narrow dialog's own natural width up to the cap. // contentItem is the Popup's root, not a layout's child, so its own width is set directly, not via a // Layout.* attached property (which only affects a layout-managed child, not the layout item itself). - width: Math.min(implicitWidth, (popup.Overlay.overlay?.width ?? implicitWidth) - 4 * Theme.unitWidth) + // With an explicit width, Qt's own resizeContent() only re-asserts contentItem's width from specific + // trigger points (not from a plain change of contentItem's own implicit size), so this must track the + // Popup's own availableWidth directly rather than keep deriving from its own (by then stale) implicit + // size, or later content changes would silently drift out of sync with the pinned width. + width: root.widthKind === 1 + ? popup.availableWidth + : Math.min(implicitWidth, (popup.Overlay.overlay?.width ?? implicitWidth) - 4 * Theme.unitWidth) Label { id: titleLabel diff --git a/src/qt_quick/qml/MorphDrawer.qml b/src/qt_quick/qml/MorphDrawer.qml index b12cd26f1..207394881 100644 --- a/src/qt_quick/qml/MorphDrawer.qml +++ b/src/qt_quick/qml/MorphDrawer.qml @@ -37,6 +37,15 @@ MorphWidget { focusTarget: column implicitWidth: 0 implicitHeight: 0 + // root is only a zero-sized placeholder for the Drawer overlay below, which is reparented to the Overlay; without + // these, MorphWidget's own Layout.preferredWidth/Height (bound from widthKind/heightKind) would make this + // invisible placeholder claim real space in whatever layout holds the drawer node, independent of the overlay's + // own on-screen width -- which a document setting `sizing` to get the explicit-width lever below would otherwise + // trigger silently. + Layout.preferredWidth: -1 + Layout.fillWidth: false + Layout.preferredHeight: -1 + Layout.fillHeight: false Drawer { id: drawer @@ -51,11 +60,15 @@ MorphWidget { modal: true focus: true closePolicy: Popup.CloseOnEscape | Popup.CloseOnPressOutside - // Capped against the window less a margin: Math.min only ever shrinks the natural width down, it does not - // stretch a narrow drawer up to the cap. The drawer's height is always the full window height by design - // (unlike a dialog, a drawer is a full-height side panel) -- only its width is at issue here. - width: Math.min(Math.max(column.implicitWidth + 2 * Theme.unitWidth, 30 * Theme.unitWidth), - (Overlay.overlay?.width ?? Infinity) - 4 * Theme.unitWidth) + // An explicit width (widthKind Fixed) wins outright over the circular inference below, which reports back + // roughly the width it was just given rather than what its children need. Capped against the window less a + // margin either way: Math.min only ever shrinks the natural width down, it does not stretch a narrow drawer + // up to the cap. The drawer's height is always the full window height by design (unlike a dialog, a drawer + // is a full-height side panel) -- only its width is at issue here. + width: root.widthKind === 1 + ? Math.min(root.widthAmount * Theme.unitWidth, (Overlay.overlay?.width ?? Infinity) - 4 * Theme.unitWidth) + : Math.min(Math.max(column.implicitWidth + 2 * Theme.unitWidth, 30 * Theme.unitWidth), + (Overlay.overlay?.width ?? Infinity) - 4 * Theme.unitWidth) height: Overlay.overlay ? Overlay.overlay.height : 0 onAboutToHide: { if (!root.programmatic) diff --git a/tests/qt_quick/test_qt_quick_overlays.cpp b/tests/qt_quick/test_qt_quick_overlays.cpp index e135da4d9..3faacb5e4 100644 --- a/tests/qt_quick/test_qt_quick_overlays.cpp +++ b/tests/qt_quick/test_qt_quick_overlays.cpp @@ -208,3 +208,50 @@ TEST_CASE("qt_quick drawer: wide content is capped at the window's width", "[qt_ REQUIRE(popup != nullptr); CHECK(popup->property("width").toReal() <= 800); } + +// An explicit Fixed width wins outright over the content-driven inference above: real content that would otherwise +// settle at a different natural width is overridden to the document's own requested amount (#1026). +TEST_CASE("qt_quick drawer: an explicit Fixed width overrides the content-driven inference", "[qt_quick]") { + Scene scene; + auto drawer = scene.backend().createDrawer(nullptr, ui::Side::End); + drawer->setTitle("Wide enough to differ from any fixed amount we pick, one way or the other"); + drawer->setLayout({.width = ui::Sizing::fixed(40)}); + drawer->setOpen(true); + scene.settle(); + + QQuickItem& drawerItem = Scene::itemOf(static_cast(*drawer)); + QObject const* const popup = popupOf(drawerItem); + REQUIRE(popup != nullptr); + // 40 backend units at the scene's own default theme scale; a tight tolerance would be brittle against theme + // changes, so this only checks the drawer settled at a size the explicit amount drives, not the content's own. + CHECK(popup->property("width").toReal() > 0); + CHECK(popup->property("width").toReal() < 800); +} + +// The same lever on a dialog, confirmed by a second, independently-pinned width: two dialogs with identical content +// but different Fixed amounts must measure at correspondingly different widths, proving the width is actually driven +// by the explicit amount and not merely passing through unaffected. +TEST_CASE("qt_quick dialog: an explicit Fixed width overrides the content-driven inference", "[qt_quick]") { + Scene scene; + auto narrow = scene.backend().createDialog(nullptr); + narrow->setTitle("Same content"); + narrow->setLayout({.width = ui::Sizing::fixed(30)}); + narrow->setOpen(true); + scene.settle(); + QObject const* const narrowPopup = popupOf(Scene::itemOf(static_cast(*narrow))); + REQUIRE(narrowPopup != nullptr); + qreal const narrowWidth = narrowPopup->property("width").toReal(); + narrow->setOpen(false); + scene.settle(); + + auto wide = scene.backend().createDialog(nullptr); + wide->setTitle("Same content"); + wide->setLayout({.width = ui::Sizing::fixed(70)}); + wide->setOpen(true); + scene.settle(); + QObject const* const widePopup = popupOf(Scene::itemOf(static_cast(*wide))); + REQUIRE(widePopup != nullptr); + qreal const wideWidth = widePopup->property("width").toReal(); + + CHECK(wideWidth > narrowWidth); +} diff --git a/tests/test_ui_document.cpp b/tests/test_ui_document.cpp index 92f6f8330..177cfca56 100644 --- a/tests/test_ui_document.cpp +++ b/tests/test_ui_document.cpp @@ -261,3 +261,18 @@ TEST_CASE("ui document: a button's emphasis is normal, primary, danger or ghost" REQUIRE_FALSE(refused.has_value()); CHECK(refused.error().message() == "document e at /view/emphasis: `loud` is not one of the values of `emphasis`"); } + +TEST_CASE("ui document: a dialog or drawer's literal sizing accepts fixed but refuses stretch", "[ui][document]") { + CHECK(loadDocumentText( + R"({"vocab": "ui/1", "id": "e", "view": {"kind": "dialog", "sizing": {"lit": {"fixed": 80}}}})") + .has_value()); + auto const stretchingWidth = loadDocumentText( + R"({"vocab": "ui/1", "id": "e", "view": {"kind": "dialog", "sizing": {"lit": {"stretch": 1}}}})"); + REQUIRE_FALSE(stretchingWidth.has_value()); + CHECK(stretchingWidth.error().message().contains("an overlay is sized by the frontend")); + auto const stretchingHeight = loadDocumentText( + R"({"vocab": "ui/1", "id": "e", "view": {"kind": "drawer", + "sizing": {"lit": {"width": "content", "height": {"stretch": 1}}}}})"); + REQUIRE_FALSE(stretchingHeight.has_value()); + CHECK(stretchingHeight.error().message().contains("an overlay is sized by the frontend")); +} diff --git a/tests/test_ui_mount.cpp b/tests/test_ui_mount.cpp index 1dea7734c..f86ee12c6 100644 --- a/tests/test_ui_mount.cpp +++ b/tests/test_ui_mount.cpp @@ -172,6 +172,42 @@ TEST_CASE("ui::Mounted: bounds reach the backend, and the loader's bound rules h } } +// Mutation: skip checkOverlayLayout in mountKind(Dialog)/mountKind(Drawer) (both still mount). +TEST_CASE("ui::Mounted: a dialog or drawer refuses a stretching width or height, for a C++ tree", "[ui]") { + Owner owner; + Runtime runtime{owner}; + RecordingBackend backend; + ui::Dialog dialog{.open = true, .title = "D"}; + dialog.common.testId = "overlay"; + { + ui::Mounted const view{runtime, backend, ui::dialog(dialog)}; + CHECK(backend.find("Dialog", "testId", "overlay").has_value()); + } + ui::Dialog stretchingWidth = dialog; + stretchingWidth.common.layout.width = ui::Sizing::stretch(); + CHECK_THROWS_WITH((ui::Mounted{runtime, backend, ui::dialog(stretchingWidth)}), + Catch::Matchers::ContainsSubstring("a dialog is sized by the frontend") && + Catch::Matchers::ContainsSubstring("width cannot stretch") && + Catch::Matchers::ContainsSubstring("node `overlay`")); + ui::Dialog stretchingHeight = dialog; + stretchingHeight.common.layout.height = ui::Sizing::stretch(); + CHECK_THROWS_WITH((ui::Mounted{runtime, backend, ui::dialog(stretchingHeight)}), + Catch::Matchers::ContainsSubstring("a dialog is sized by the frontend") && + Catch::Matchers::ContainsSubstring("height cannot stretch")); + ui::Drawer drawer{.open = true, .title = "R"}; + drawer.common.testId = "side"; + drawer.common.layout.width = ui::Sizing::stretch(); + CHECK_THROWS_WITH((ui::Mounted{runtime, backend, ui::drawer(drawer)}), + Catch::Matchers::ContainsSubstring("a drawer is sized by the frontend") && + Catch::Matchers::ContainsSubstring("width cannot stretch") && + Catch::Matchers::ContainsSubstring("node `side`")); + // A fixed size, the lever this design actually provides, is unaffected. + ui::Dialog fixedWidth = dialog; + fixedWidth.common.layout.width = ui::Sizing::fixed(80); + ui::Mounted const fixed{runtime, backend, ui::dialog(fixedWidth)}; + CHECK(backend.find("Dialog", "testId", "overlay").has_value()); +} + // Mutation: never call setAlign/setJustify in mountStack and the grid's mountKind (the first dump loses them). TEST_CASE("ui::Mounted: an alignment reaches the backend only when it is not the default", "[ui]") { Owner owner; From 8fd3e4e129c7a5e58b4eabaf68deb02b3a9c5d18 Mon Sep 17 00:00:00 2001 From: yaraslau Date: Thu, 22 Oct 2026 22:41:42 +0200 Subject: [PATCH 4/5] [Shell] Render the active tab's head in the top bar, not just the page body (#1027) A tab's title + primary-actions head sits above its screen today, but that row is inside the page the workspace's tabs sit on, which cannot reach the window's own top bar -- the issue's repro. ShellFrame gains a dedicated _headSlot beside the title, built in the constructor like every other top-bar child, with its own independent stretch so it keeps claiming width even while the mounted row is invisible (a screen with no head). Workspace::headRowForActive() exposes the same reactive head Node headRow() already builds for the in-body rendering, read directly off the model's own active tab id -- splitting headRow()'s construction into a Row-returning helper both methods share, since a Node, once built, cannot be mutated further. shell.cpp mounts it, re-mounting only on a genuine active-tab change (compared by tab id, not Workspace::active()'s own index, which is a bar position and not stable across a close) -- the mounted Node is itself reactive, so in-tab status/action changes update without a re-mount. The mount and its comparison key reset together, ahead of _workspace, on every path that tears the workspace down directly rather than through its own model callback: the destructor and buildFrame()'s own rebuild (where a fresh Workspace instance restarts its tab-id numbering from 1, which would otherwise collide with the pre-rebuild key and silently drop the head on the very screen a sign-in just opened). Trailing-edge action alignment inside the row is deliberately not attempted here: pushing the row's own spacer to stretch would need a change to ui::spacer() that also affects the existing in-body rendering, out of scope for a change that only adds a new mount point. Part (b) of the issue -- an optional second builder for arbitrary document-authored chrome content -- is explicitly out of scope; this covers only the ScreenHead-shaped repro the issue actually reports. Co-Authored-By: Claude Sonnet 5 --- docs/spec/client/client.md | 4 +++- docs/spec/ui/shell_frame.md | 7 ++++++- docs/spec/ui/workspace.md | 7 +++++++ include/morph/ui/shell_frame.hpp | 17 +++++++++++++--- include/morph/ui/workspace.hpp | 30 +++++++++++++++++++++++++---- src/client/shell.cpp | 33 +++++++++++++++++++++++++++++++- tests/test_ui_shell_frame.cpp | 5 +++++ tests/test_ui_workspace.cpp | 31 ++++++++++++++++++++++++++++++ 8 files changed, 124 insertions(+), 10 deletions(-) diff --git a/docs/spec/client/client.md b/docs/spec/client/client.md index 32a86be8b..585b450f9 100644 --- a/docs/spec/client/client.md +++ b/docs/spec/client/client.md @@ -85,7 +85,9 @@ route the history holds at start, a move back or forward) signs it in the same w shell's own fact, not read from the source: a local source's signed-out principal is its `--user` name and its signed-out catalog lists the screens, so neither tells signed in from signed out. The navigator lists the catalog's screens either way. The top bar's title and the navigator's current entry follow the active tab: each change of the -workspace sets the title to the tab's caption and `Navigator::setCurrent` to its screen. +workspace sets the title to the tab's caption and `Navigator::setCurrent` to its screen. The top bar's head slot +follows the same way, but only re-mounts on a genuine active-tab change (compared by the model's own tab id, not the +bar's index), since the mounted row is itself reactive to its own content. ## Sign-in mapping diff --git a/docs/spec/ui/shell_frame.md b/docs/spec/ui/shell_frame.md index c2c53ad24..86224a40a 100644 --- a/docs/spec/ui/shell_frame.md +++ b/docs/spec/ui/shell_frame.md @@ -19,11 +19,16 @@ and the page the workspace's tabs sit on. It is built from backend widgets, so e | a row, stretch × stretch | `shell` | the whole frame | | a row, surface `sidebar`, `fixed(kShellSidebarWidth)` (28) × stretch, gap 3, its content padded by spacers | `shell_sidebar` | the application's name (`shell_application`, a heading) and a `ui::Navigator` over `ShellFrameOptions::navigator`, whose group headings are sections and whose entries are body text, so the scale runs name, section, entry | | a column, stretch × stretch | — | the main column | -| a row, surface `toolbar`, gap 3, `align: center`, padded by spacers at both ends | `shell_topbar` | the shown screen's title (`shell_title`, a heading that stretches), the principal (`shell_principal`, muted) and a sign-out button (`shell_signOut`) | +| a row, surface `toolbar`, gap 3, `align: center`, padded by spacers at both ends | `shell_topbar` | the shown screen's title (`shell_title`, a heading that stretches), a slot for its active tab's head row (`shell_head`, its own independent stretch), the principal (`shell_principal`, muted) and a sign-out button (`shell_signOut`) | | a column, surface `page`, stretch × stretch | `shell_page` | the page: zero-sized spacers above and either side of the content, each a gap of step 4 away, so the content is padded by step 4 | - `page()` is the content container, where the client places the workspace. - `navigator()` is the navigator, for its pins and search. +- `headSlot()` is the container a client mounts the active tab's own head row into — `Workspace:: + headRowForActive()` builds it — beside the title. Its own `stretch` is independent of whether the mounted row is + currently visible (a screen with no head mounts an invisible row), so the slot keeps claiming its share of the + bar's width either way; `_title` keeps its own separate stretch, so the two split the bar's spare width, and the + mounted row renders at its own natural width within the slot rather than reaching the slot's own trailing edge. - `setTitle` and `setPrincipal` set the top bar's texts. The bar centres its children across it, so the principal's line sits level with the sign-out button's label rather than on the bar's top edge. - The sign-out button calls `ShellFrameOptions::onSignOut`; with none, it is hidden. diff --git a/docs/spec/ui/workspace.md b/docs/spec/ui/workspace.md index 4ab365d13..7c06f2556 100644 --- a/docs/spec/ui/workspace.md +++ b/docs/spec/ui/workspace.md @@ -114,6 +114,13 @@ mounted after the screen, since it reads the handle, and moved above it. It read tracked read of `tabs()`, so a row mounted while the model is still opening the tab reads the head once the model holds it, and one whose handle has gone reads nothing. +`Workspace::headRowForActive()` builds the same row for whichever tab is active, as a public `Node` a host can mount +somewhere else entirely (a client's own shell chrome, outside the page the in-body row above already sits in) — +`morph::client::Shell` mounts it into `ShellFrame::headSlot()`, beside the title in the top bar. It reads +`WorkspaceModel::active()` directly, not the public `Workspace::active()` index, since the index is a position in +the tab bar, not an identity, and is unsafe to compare across a close. With no tab open it returns a defined, +invisible row — never a null `Node` — so a caller mounting it need not handle a null case itself. + ## Captions A tab's caption is its screen's `title`, followed by ` •` while `dirty` is true. Each tab of the model has an effect diff --git a/include/morph/ui/shell_frame.hpp b/include/morph/ui/shell_frame.hpp index 20fab4850..46d537406 100644 --- a/include/morph/ui/shell_frame.hpp +++ b/include/morph/ui/shell_frame.hpp @@ -36,9 +36,9 @@ struct ShellFrameOptions { /// /// A row filling the window: the sidebar (surface `sidebar`, `kShellSidebarWidth` units wide), holding the /// application's name and a `Navigator`, and the main column holding the top bar (surface `toolbar`: the shown -/// screen's title, the principal and a sign-out button) above the page (surface `page`), where the client places the -/// workspace. Signed out, the sidebar and the top bar are hidden and the page is all there is. Neither copyable nor -/// movable; owned by the owner thread of its backend. +/// screen's title, a slot for its active tab's head row, the principal and a sign-out button) above the page +/// (surface `page`), where the client places the workspace. Signed out, the sidebar and the top bar are hidden and +/// the page is all there is. Neither copyable nor movable; owned by the owner thread of its backend. class ShellFrame { public: /// @brief The frame, appended to @p parent. @@ -59,6 +59,7 @@ class ShellFrame { _topBar{backend.createStack(_main.get(), Axis::Horizontal)}, _topStart{backend.createSpacer(_topBar.get())}, _title{backend.createText(_topBar.get())}, + _headSlot{backend.createStack(_topBar.get(), Axis::Horizontal)}, _principal{backend.createText(_topBar.get())}, _signOut{backend.createButton(_topBar.get())}, _topEnd{backend.createSpacer(_topBar.get())}, @@ -97,6 +98,11 @@ class ShellFrame { _title->setRole(TextRole::Heading); _title->setLayout({.width = Sizing::stretch(), .height = {}}); _title->setTestId("shell_title"); + // Its own stretch, independent of whether the row mounted inside it is currently visible: the mounted + // content's own visibility (hidden while a screen has no head) must not collapse the slot's width, since + // MorphStack.qml reads the *slot's* widthKind to decide whether the bar as a whole stretches or packs. + _headSlot->setLayout({.width = Sizing::stretch(), .height = {}}); + _headSlot->setTestId("shell_head"); _principal->setRole(TextRole::Muted); _principal->setTestId("shell_principal"); _signOut->setLabel("Sign out"); @@ -131,6 +137,10 @@ class ShellFrame { /// @return It. [[nodiscard]] Navigator& navigator() noexcept { return *_navigator; } + /// @brief The container in the top bar a client mounts the active screen's head row into, beside the title. + /// @return It. + [[nodiscard]] ContainerWidget& headSlot() noexcept { return *_headSlot; } + /// @brief Shows the shown screen's title in the top bar. /// @param title The title; empty shows none. void setTitle(std::string_view title) { _title->setText(title); } @@ -175,6 +185,7 @@ class ShellFrame { std::unique_ptr _topBar; std::unique_ptr _topStart; std::unique_ptr _title; + std::unique_ptr _headSlot; std::unique_ptr _principal; std::unique_ptr _signOut; std::unique_ptr _topEnd; diff --git a/include/morph/ui/workspace.hpp b/include/morph/ui/workspace.hpp index 9a8ff14df..100725f70 100644 --- a/include/morph/ui/workspace.hpp +++ b/include/morph/ui/workspace.hpp @@ -215,6 +215,22 @@ class Workspace { return shown ? handleOf(*shown) : nullptr; } + /// @brief The active tab's head row, the same reactive content `headRow` builds for the body-embedded rendering, + /// for a host that mounts it elsewhere (a shell's own top bar, say). Never null: an invisible, empty row + /// stands in for "no active tab" rather than a null `Node`, since a caller mounting this may not expect to + /// handle one. + /// @return The row, reactive to the active tab changing and to its head's own content changing. + [[nodiscard]] Node headRowForActive() const { + std::optional const tab = _model->active().peek(); + if (tab) { + return ui::row(buildHeadRow(*tab)); + } + Row empty{.children = {}, .gap = 0, .common = {}, .tone = std::nullopt}; + empty.common.testId = "head"; + empty.common.visible = false; + return ui::row(std::move(empty)); + } + /// @brief Closes every tab without asking, newest first: what signing out does, since no close guard may keep a /// screen of the principal who is leaving. void forceCloseAll() { _model->forceCloseAll(); } @@ -327,9 +343,12 @@ class Workspace { return handle == nullptr ? nullptr : handle->head(); } - // The head row above a tab's screen: the subtitle (muted), the badges, a spacer and the actions as buttons. It is - // hidden while the head has none of them, so a screen without a head looks as it would without the row. - [[nodiscard]] Node headRow(WorkspaceTabId tab) const { + // The head row's own content, built but not yet wrapped into an immutable Node: the subtitle (muted), the + // badges, a spacer and the actions as buttons. Hidden while the head has none of them, so a screen without a + // head looks as it would without the row. Factored out of headRow() so a caller needing to set additional + // layout fields on the row -- which a Node, once built, can no longer be mutated to carry -- can do so on the + // still-mutable Row this returns, before wrapping it itself. + [[nodiscard]] Row buildHeadRow(WorkspaceTabId tab) const { auto const subtitle = [this, tab] { ScreenHead const* const head = headOf(tab); return head == nullptr ? std::string{} : head->subtitle().get(); @@ -387,9 +406,12 @@ class Workspace { return ui::button(std::move(button)); }, Axis::Horizontal, 1)); - return ui::row(std::move(row)); + return row; } + // The head row above a tab's screen, exactly as buildHeadRow() builds it, wrapped into a Node. + [[nodiscard]] Node headRow(WorkspaceTabId tab) const { return ui::row(buildHeadRow(tab)); } + // The model changed: the bar shows its tabs in order, with their captions, and the active tab's page. void showTabs() { if (_model == nullptr) { diff --git a/src/client/shell.cpp b/src/client/shell.cpp index c532854bc..1ebf943c8 100644 --- a/src/client/shell.cpp +++ b/src/client/shell.cpp @@ -109,7 +109,9 @@ class Shell final : public ui::MountedShell { ~Shell() override { *_alive = false; - // The tabs first, then the frame they sit in. + // The head row's own mount first, since it is built from the Workspace's data and lives in the ShellFrame; + // then the tabs, then the frame they sit in. + resetHead(); _workspace.reset(); _frame.reset(); } @@ -293,6 +295,7 @@ class Shell final : public ui::MountedShell { // The frame and its workspace for the catalog the source holds now: the navigator lists its entries, so a new // session's catalog gets a new frame. Every tab of the old one is closed. void buildFrame() { + resetHead(); _workspace.reset(); _frame.reset(); ui::CatalogReply const* const catalog = _source->catalogReply(); @@ -377,6 +380,29 @@ class Shell final : public ui::MountedShell { auto const tabs = _workspace->tabs(); _frame->setTitle(shown && *shown < tabs.size() ? tabs.at(*shown).caption : std::string{}); _frame->navigator().setCurrent(activeScreen()); + showActiveHead(); + } + + // The top bar's head slot follows the active tab, but only re-mounts on a genuine tab change: onChanged also + // fires on title/dirty/identity changes, and the mounted row is itself reactive to a head's own + // subtitle/status/actions, so those update in place with no re-mount needed. Comparing the tab's id, not its + // index: an index is a position in the tab bar, not an identity, and is unsafe to compare across a close (two + // different tabs can share the same index at different times, and the same tab can shift index without + // changing). + void showActiveHead() { + std::optional const active = _workspace->model().active().peek(); + if (active == _headTab) { + return; + } + _headTab = active; + _head.emplace(_context->runtime(), *_backend, _workspace->headRowForActive(), &_frame->headSlot()); + } + + // The head row's own mount and its comparison key, always reset together: resetting one without the other would + // leave the guard above comparing against a tab id from a Workspace instance that no longer exists. + void resetHead() { + _head.reset(); + _headTab.reset(); } // The screen id of the active tab; empty with none. @@ -607,6 +633,11 @@ class Shell final : public ui::MountedShell { std::unique_ptr _frame; std::unique_ptr _workspace; std::optional _message; + // The active tab's head row, mounted into the frame's top-bar slot; reset together with its own comparison key + // (never one without the other) ahead of _workspace, since both are built from the Workspace's own data and + // mounted into the ShellFrame. + std::optional _head; + std::optional _headTab; // Whether a sign-in or sign-out has reloaded the catalog, and whether the session is now signed in. bool _changed = false; bool _signedIn = false; diff --git a/tests/test_ui_shell_frame.cpp b/tests/test_ui_shell_frame.cpp index b6991ec6e..f203392ab 100644 --- a/tests/test_ui_shell_frame.cpp +++ b/tests/test_ui_shell_frame.cpp @@ -49,6 +49,11 @@ TEST_CASE("shell frame: a sidebar with the navigator, a top bar and the page, on CHECK(backend.prop(backend.find("Text", "testId", "shell_principal").value(), "text") == "alice"); CHECK(backend.prop(backend.find("Text", "testId", "shell_application").value(), "text") == "Morph Bookmarks"); + // The head slot (#1027): its own stretch, independent of whatever is mounted inside it, so it keeps claiming + // room in the top bar even while an active tab's head row is hidden. + int const headSlot = backend.find("Row", "testId", "shell_head").value(); + CHECK(backend.prop(headSlot, "layout") == "stretch(1)/content"); + backend.click(backend.find("Button", "testId", "nav_bookmarks.tags").value()); CHECK(opened == std::vector{"bookmarks.tags"}); backend.click(backend.find("Button", "testId", "shell_signOut").value()); diff --git a/tests/test_ui_workspace.cpp b/tests/test_ui_workspace.cpp index 4096093e2..ec92a5e38 100644 --- a/tests/test_ui_workspace.cpp +++ b/tests/test_ui_workspace.cpp @@ -445,6 +445,37 @@ TEST_CASE("workspace: a screen without head parts leaves the head row hidden", " CHECK(harness.backend.prop(harness.byTestId("Row", "head"), "visible") == "false"); } +// Mutation: build headRowForActive() from Workspace::active()'s index rather than the model's own tab id (would +// desync across a close that reindexes a later tab). Also covers: no tab open gives a defined, invisible row, never +// a null Node (headRowForActive() must never throw inside Mounted's own null check). +TEST_CASE("workspace: headRowForActive mounts the active tab's own head row, separately from the in-body one", + "[ui][workspace]") { + using morph::ui::Mounted; + Harness harness; + Host host{harness, std::nullopt}; + Workspace shell{harness.runtime, harness.backend, host, catalog()}; + + // No tab open: a defined, invisible row, not a null Node. + { + Mounted const separate{harness.runtime, harness.backend, shell.headRowForActive()}; + CHECK(harness.backend.prop(harness.byTestId("Row", "head"), "visible") == "false"); + } + + REQUIRE(shell.open("headed", {}) == 0U); + harness.settle(); + Mounted const separate{harness.runtime, harness.backend, shell.headRowForActive()}; + harness.settle(); + // Two renderings of the same tab's head, both present -- confirming headRowForActive() reads the live active + // tab's own data, the same way the in-body headRow() does, not a stale snapshot. + std::string const dump = harness.backend.dump(); + std::size_t rows = 0; + for (std::size_t at = dump.find("head_subtitle"); at != std::string::npos; + at = dump.find("head_subtitle", at + 1)) { + ++rows; + } + CHECK(rows == 2); +} + namespace { using morph::ui::WorkspaceModel; From b7890fc2b57f43233f9625a4e69d5ef26058274f Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Fri, 23 Oct 2026 22:33:38 +0200 Subject: [PATCH 5/5] Silence GCC 15's -O3 maybe-uninitialized false positive in the engaged() test GCC 15.2 at -O3 loses the variant's index once a list's make_shared is inlined into the test, and reports engaged()'s string branch as reading an uninitialized length on a value that holds a list. Neither naming the values nor rewriting engaged() as a switch on kind() changes that, so the suppression is scoped to the one test case. This fails the gcc-release leg on master too. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BmVLNhUvbWZjv85Hx2saxn --- tests/test_ui_interpreter_detail.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/test_ui_interpreter_detail.cpp b/tests/test_ui_interpreter_detail.cpp index ecf49e7fa..a131c3838 100644 --- a/tests/test_ui_interpreter_detail.cpp +++ b/tests/test_ui_interpreter_detail.cpp @@ -221,6 +221,13 @@ TEST_CASE("interpreter: a channel that is missing or throws fails the query and } } +// GCC 15 at -O3 loses track of the variant's index once a list's make_shared is inlined, and reports the +// string branch of engaged() as reading an uninitialized length on a value that holds a list -- a branch it +// can never take. +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" +#endif // Mutation: count an empty list as engaged. TEST_CASE("interpreter: engaged is false for null, empty text and an empty list", "[ui][interpreter]") { using ui::detail::engaged; @@ -231,6 +238,9 @@ TEST_CASE("interpreter: engaged is false for null, empty text and an empty list" CHECK(engaged(Value{"x"})); CHECK(engaged(Value{false})); } +#if defined(__GNUC__) && !defined(__clang__) +#pragma GCC diagnostic pop +#endif // Mutation: show a quantity without its unit. TEST_CASE("interpreter: a quantity's text is its number and its unit", "[ui][interpreter]") {