Skip to content

Commit a8ad908

Browse files
Yaraslautclaude
andcommitted
[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 <noreply@anthropic.com>
1 parent e489c79 commit a8ad908

8 files changed

Lines changed: 124 additions & 10 deletions

File tree

‎docs/spec/client/client.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,9 @@ route the history holds at start, a move back or forward) signs it in the same w
8585
shell's own fact, not read from the source: a local source's signed-out principal is its `--user` name and its
8686
signed-out catalog lists the screens, so neither tells signed in from signed out. The navigator lists the catalog's
8787
screens either way. The top bar's title and the navigator's current entry follow the active tab: each change of the
88-
workspace sets the title to the tab's caption and `Navigator::setCurrent` to its screen.
88+
workspace sets the title to the tab's caption and `Navigator::setCurrent` to its screen. The top bar's head slot
89+
follows the same way, but only re-mounts on a genuine active-tab change (compared by the model's own tab id, not the
90+
bar's index), since the mounted row is itself reactive to its own content.
8991

9092
## Sign-in mapping
9193

‎docs/spec/ui/shell_frame.md‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,16 @@ and the page the workspace's tabs sit on. It is built from backend widgets, so e
1919
| a row, stretch × stretch | `shell` | the whole frame |
2020
| 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 |
2121
| a column, stretch × stretch | — | the main column |
22-
| 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`) |
22+
| 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`) |
2323
| 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 |
2424

2525
- `page()` is the content container, where the client places the workspace.
2626
- `navigator()` is the navigator, for its pins and search.
27+
- `headSlot()` is the container a client mounts the active tab's own head row into — `Workspace::
28+
headRowForActive()` builds it — beside the title. Its own `stretch` is independent of whether the mounted row is
29+
currently visible (a screen with no head mounts an invisible row), so the slot keeps claiming its share of the
30+
bar's width either way; `_title` keeps its own separate stretch, so the two split the bar's spare width, and the
31+
mounted row renders at its own natural width within the slot rather than reaching the slot's own trailing edge.
2732
- `setTitle` and `setPrincipal` set the top bar's texts. The bar centres its children across it, so the principal's
2833
line sits level with the sign-out button's label rather than on the bar's top edge.
2934
- The sign-out button calls `ShellFrameOptions::onSignOut`; with none, it is hidden.

‎docs/spec/ui/workspace.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,13 @@ mounted after the screen, since it reads the handle, and moved above it. It read
114114
tracked read of `tabs()`, so a row mounted while the model is still opening the tab reads the head once the model
115115
holds it, and one whose handle has gone reads nothing.
116116

117+
`Workspace::headRowForActive()` builds the same row for whichever tab is active, as a public `Node` a host can mount
118+
somewhere else entirely (a client's own shell chrome, outside the page the in-body row above already sits in) —
119+
`morph::client::Shell` mounts it into `ShellFrame::headSlot()`, beside the title in the top bar. It reads
120+
`WorkspaceModel::active()` directly, not the public `Workspace::active()` index, since the index is a position in
121+
the tab bar, not an identity, and is unsafe to compare across a close. With no tab open it returns a defined,
122+
invisible row — never a null `Node` — so a caller mounting it need not handle a null case itself.
123+
117124
## Captions
118125

119126
A tab's caption is its screen's `title`, followed by ` •` while `dirty` is true. Each tab of the model has an effect

‎include/morph/ui/shell_frame.hpp‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,9 @@ struct ShellFrameOptions {
3636
///
3737
/// A row filling the window: the sidebar (surface `sidebar`, `kShellSidebarWidth` units wide), holding the
3838
/// application's name and a `Navigator`, and the main column holding the top bar (surface `toolbar`: the shown
39-
/// screen's title, the principal and a sign-out button) above the page (surface `page`), where the client places the
40-
/// workspace. Signed out, the sidebar and the top bar are hidden and the page is all there is. Neither copyable nor
41-
/// movable; owned by the owner thread of its backend.
39+
/// screen's title, a slot for its active tab's head row, the principal and a sign-out button) above the page
40+
/// (surface `page`), where the client places the workspace. Signed out, the sidebar and the top bar are hidden and
41+
/// the page is all there is. Neither copyable nor movable; owned by the owner thread of its backend.
4242
class ShellFrame {
4343
public:
4444
/// @brief The frame, appended to @p parent.
@@ -59,6 +59,7 @@ class ShellFrame {
5959
_topBar{backend.createStack(_main.get(), Axis::Horizontal)},
6060
_topStart{backend.createSpacer(_topBar.get())},
6161
_title{backend.createText(_topBar.get())},
62+
_headSlot{backend.createStack(_topBar.get(), Axis::Horizontal)},
6263
_principal{backend.createText(_topBar.get())},
6364
_signOut{backend.createButton(_topBar.get())},
6465
_topEnd{backend.createSpacer(_topBar.get())},
@@ -97,6 +98,11 @@ class ShellFrame {
9798
_title->setRole(TextRole::Heading);
9899
_title->setLayout({.width = Sizing::stretch(), .height = {}});
99100
_title->setTestId("shell_title");
101+
// Its own stretch, independent of whether the row mounted inside it is currently visible: the mounted
102+
// content's own visibility (hidden while a screen has no head) must not collapse the slot's width, since
103+
// MorphStack.qml reads the *slot's* widthKind to decide whether the bar as a whole stretches or packs.
104+
_headSlot->setLayout({.width = Sizing::stretch(), .height = {}});
105+
_headSlot->setTestId("shell_head");
100106
_principal->setRole(TextRole::Muted);
101107
_principal->setTestId("shell_principal");
102108
_signOut->setLabel("Sign out");
@@ -131,6 +137,10 @@ class ShellFrame {
131137
/// @return It.
132138
[[nodiscard]] Navigator& navigator() noexcept { return *_navigator; }
133139

140+
/// @brief The container in the top bar a client mounts the active screen's head row into, beside the title.
141+
/// @return It.
142+
[[nodiscard]] ContainerWidget& headSlot() noexcept { return *_headSlot; }
143+
134144
/// @brief Shows the shown screen's title in the top bar.
135145
/// @param title The title; empty shows none.
136146
void setTitle(std::string_view title) { _title->setText(title); }
@@ -175,6 +185,7 @@ class ShellFrame {
175185
std::unique_ptr<StackWidget> _topBar;
176186
std::unique_ptr<SpacerWidget> _topStart;
177187
std::unique_ptr<TextWidget> _title;
188+
std::unique_ptr<StackWidget> _headSlot;
178189
std::unique_ptr<TextWidget> _principal;
179190
std::unique_ptr<ButtonWidget> _signOut;
180191
std::unique_ptr<SpacerWidget> _topEnd;

‎include/morph/ui/workspace.hpp‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,22 @@ class Workspace {
215215
return shown ? handleOf(*shown) : nullptr;
216216
}
217217

218+
/// @brief The active tab's head row, the same reactive content `headRow` builds for the body-embedded rendering,
219+
/// for a host that mounts it elsewhere (a shell's own top bar, say). Never null: an invisible, empty row
220+
/// stands in for "no active tab" rather than a null `Node`, since a caller mounting this may not expect to
221+
/// handle one.
222+
/// @return The row, reactive to the active tab changing and to its head's own content changing.
223+
[[nodiscard]] Node headRowForActive() const {
224+
std::optional<WorkspaceTabId> const tab = _model->active().peek();
225+
if (tab) {
226+
return ui::row(buildHeadRow(*tab));
227+
}
228+
Row empty{.children = {}, .gap = 0, .common = {}, .tone = std::nullopt};
229+
empty.common.testId = "head";
230+
empty.common.visible = false;
231+
return ui::row(std::move(empty));
232+
}
233+
218234
/// @brief Closes every tab without asking, newest first: what signing out does, since no close guard may keep a
219235
/// screen of the principal who is leaving.
220236
void forceCloseAll() { _model->forceCloseAll(); }
@@ -327,9 +343,12 @@ class Workspace {
327343
return handle == nullptr ? nullptr : handle->head();
328344
}
329345

330-
// The head row above a tab's screen: the subtitle (muted), the badges, a spacer and the actions as buttons. It is
331-
// hidden while the head has none of them, so a screen without a head looks as it would without the row.
332-
[[nodiscard]] Node headRow(WorkspaceTabId tab) const {
346+
// The head row's own content, built but not yet wrapped into an immutable Node: the subtitle (muted), the
347+
// badges, a spacer and the actions as buttons. Hidden while the head has none of them, so a screen without a
348+
// head looks as it would without the row. Factored out of headRow() so a caller needing to set additional
349+
// layout fields on the row -- which a Node, once built, can no longer be mutated to carry -- can do so on the
350+
// still-mutable Row this returns, before wrapping it itself.
351+
[[nodiscard]] Row buildHeadRow(WorkspaceTabId tab) const {
333352
auto const subtitle = [this, tab] {
334353
ScreenHead const* const head = headOf(tab);
335354
return head == nullptr ? std::string{} : head->subtitle().get();
@@ -387,9 +406,12 @@ class Workspace {
387406
return ui::button(std::move(button));
388407
},
389408
Axis::Horizontal, 1));
390-
return ui::row(std::move(row));
409+
return row;
391410
}
392411

412+
// The head row above a tab's screen, exactly as buildHeadRow() builds it, wrapped into a Node.
413+
[[nodiscard]] Node headRow(WorkspaceTabId tab) const { return ui::row(buildHeadRow(tab)); }
414+
393415
// The model changed: the bar shows its tabs in order, with their captions, and the active tab's page.
394416
void showTabs() {
395417
if (_model == nullptr) {

‎src/client/shell.cpp‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,9 @@ class Shell final : public ui::MountedShell {
109109

110110
~Shell() override {
111111
*_alive = false;
112-
// The tabs first, then the frame they sit in.
112+
// The head row's own mount first, since it is built from the Workspace's data and lives in the ShellFrame;
113+
// then the tabs, then the frame they sit in.
114+
resetHead();
113115
_workspace.reset();
114116
_frame.reset();
115117
}
@@ -293,6 +295,7 @@ class Shell final : public ui::MountedShell {
293295
// The frame and its workspace for the catalog the source holds now: the navigator lists its entries, so a new
294296
// session's catalog gets a new frame. Every tab of the old one is closed.
295297
void buildFrame() {
298+
resetHead();
296299
_workspace.reset();
297300
_frame.reset();
298301
ui::CatalogReply const* const catalog = _source->catalogReply();
@@ -377,6 +380,29 @@ class Shell final : public ui::MountedShell {
377380
auto const tabs = _workspace->tabs();
378381
_frame->setTitle(shown && *shown < tabs.size() ? tabs.at(*shown).caption : std::string{});
379382
_frame->navigator().setCurrent(activeScreen());
383+
showActiveHead();
384+
}
385+
386+
// The top bar's head slot follows the active tab, but only re-mounts on a genuine tab change: onChanged also
387+
// fires on title/dirty/identity changes, and the mounted row is itself reactive to a head's own
388+
// subtitle/status/actions, so those update in place with no re-mount needed. Comparing the tab's id, not its
389+
// index: an index is a position in the tab bar, not an identity, and is unsafe to compare across a close (two
390+
// different tabs can share the same index at different times, and the same tab can shift index without
391+
// changing).
392+
void showActiveHead() {
393+
std::optional<ui::WorkspaceTabId> const active = _workspace->model().active().peek();
394+
if (active == _headTab) {
395+
return;
396+
}
397+
_headTab = active;
398+
_head.emplace(_context->runtime(), *_backend, _workspace->headRowForActive(), &_frame->headSlot());
399+
}
400+
401+
// The head row's own mount and its comparison key, always reset together: resetting one without the other would
402+
// leave the guard above comparing against a tab id from a Workspace instance that no longer exists.
403+
void resetHead() {
404+
_head.reset();
405+
_headTab.reset();
380406
}
381407

382408
// The screen id of the active tab; empty with none.
@@ -607,6 +633,11 @@ class Shell final : public ui::MountedShell {
607633
std::unique_ptr<ui::ShellFrame> _frame;
608634
std::unique_ptr<ui::Workspace> _workspace;
609635
std::optional<ui::Mounted> _message;
636+
// The active tab's head row, mounted into the frame's top-bar slot; reset together with its own comparison key
637+
// (never one without the other) ahead of _workspace, since both are built from the Workspace's own data and
638+
// mounted into the ShellFrame.
639+
std::optional<ui::Mounted> _head;
640+
std::optional<ui::WorkspaceTabId> _headTab;
610641
// Whether a sign-in or sign-out has reloaded the catalog, and whether the session is now signed in.
611642
bool _changed = false;
612643
bool _signedIn = false;

‎tests/test_ui_shell_frame.cpp‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,11 @@ TEST_CASE("shell frame: a sidebar with the navigator, a top bar and the page, on
4949
CHECK(backend.prop(backend.find("Text", "testId", "shell_principal").value(), "text") == "alice");
5050
CHECK(backend.prop(backend.find("Text", "testId", "shell_application").value(), "text") == "Morph Bookmarks");
5151

52+
// The head slot (#1027): its own stretch, independent of whatever is mounted inside it, so it keeps claiming
53+
// room in the top bar even while an active tab's head row is hidden.
54+
int const headSlot = backend.find("Row", "testId", "shell_head").value();
55+
CHECK(backend.prop(headSlot, "layout") == "stretch(1)/content");
56+
5257
backend.click(backend.find("Button", "testId", "nav_bookmarks.tags").value());
5358
CHECK(opened == std::vector<std::string>{"bookmarks.tags"});
5459
backend.click(backend.find("Button", "testId", "shell_signOut").value());

‎tests/test_ui_workspace.cpp‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -445,6 +445,37 @@ TEST_CASE("workspace: a screen without head parts leaves the head row hidden", "
445445
CHECK(harness.backend.prop(harness.byTestId("Row", "head"), "visible") == "false");
446446
}
447447

448+
// Mutation: build headRowForActive() from Workspace::active()'s index rather than the model's own tab id (would
449+
// desync across a close that reindexes a later tab). Also covers: no tab open gives a defined, invisible row, never
450+
// a null Node (headRowForActive() must never throw inside Mounted's own null check).
451+
TEST_CASE("workspace: headRowForActive mounts the active tab's own head row, separately from the in-body one",
452+
"[ui][workspace]") {
453+
using morph::ui::Mounted;
454+
Harness harness;
455+
Host host{harness, std::nullopt};
456+
Workspace shell{harness.runtime, harness.backend, host, catalog()};
457+
458+
// No tab open: a defined, invisible row, not a null Node.
459+
{
460+
Mounted const separate{harness.runtime, harness.backend, shell.headRowForActive()};
461+
CHECK(harness.backend.prop(harness.byTestId("Row", "head"), "visible") == "false");
462+
}
463+
464+
REQUIRE(shell.open("headed", {}) == 0U);
465+
harness.settle();
466+
Mounted const separate{harness.runtime, harness.backend, shell.headRowForActive()};
467+
harness.settle();
468+
// Two renderings of the same tab's head, both present -- confirming headRowForActive() reads the live active
469+
// tab's own data, the same way the in-body headRow() does, not a stale snapshot.
470+
std::string const dump = harness.backend.dump();
471+
std::size_t rows = 0;
472+
for (std::size_t at = dump.find("head_subtitle"); at != std::string::npos;
473+
at = dump.find("head_subtitle", at + 1)) {
474+
++rows;
475+
}
476+
CHECK(rows == 2);
477+
}
478+
448479
namespace {
449480

450481
using morph::ui::WorkspaceModel;

0 commit comments

Comments
 (0)