feat(sidebar): copy a row's name from the context menu - #385
Conversation
Reading a name off the sidebar is the other half of setting one, and there was
no way to do it. Renaming a session to something you want to reuse elsewhere —
a branch name, a worktree directory, a host — meant retyping it, and the
sidebar label is the one string in the app you cannot select with the mouse.
Copy Name sits directly under Rename on both row kinds, since the two act on
the same thing: the row's identity. With several sessions selected it becomes
Copy Names and copies one per line, matching how Flag, Close and Move already
treat a multi-selection.
It copies displayName, so it agrees with what the row shows: the manual rename
if there is one, else the pane's terminal title, else the cwd basename. Copying
customName instead would yield an empty string for every session that was never
renamed, which is most of them.
A keymap custom command cannot cover this. {AGT_SESSION_NAME} and
{AGT_WORKSPACE_NAME} resolve the ACTIVE row, so they answer a different
question than 'the row I just right-clicked' — and for the workspace half there
is no GUI surface at all today.
Names resolve through the sidebar's window-local store, like Close, Flag and
Duplicate, so a background window copies its own row rather than the frontmost
window's. A row closed between the right-click and the menu choice copies
nothing and leaves the pasteboard untouched, rather than clearing whatever the
user had on it.
No control command. The 'adding a user-facing action' rule keeps the toolbar,
menu bar and socket from drifting, and this is on none of the first two; the
data is already fully exposed, since tree reports every session's and
workspace's name. Writing the system pasteboard would also contradict a
decision the API has already made: session copy returns the selection and
explicitly does not touch the clipboard, while session paste reads it. A script
that wants this today pipes tree through pbcopy.
The SidebarNode arm handled any node, resting on workspaceName returning nil for a session id. That is true, but it turns a wiring mistake into a silent no-op indistinguishable from 'the row went away'. menu(forRow:) only ever attaches a node for a workspace row, so the pattern now says that.
…steboard
workspaceName returned Optional("") for a blank name, which passed the
non-empty guard and cleared the clipboard to write nothing — the opposite of
what the guard is for. Blank is unreachable through renameWorkspace, which
rejects it, but AppStore+PendingClose rebuilds a Workspace straight from a
snapshot with no such check. Nil is now the answer for blank as well as gone,
decided in core rather than at the call site.
Comments cut to the brevity rule: the placement rationale belonged in the PR,
not the source, and the window-local-store note is already made four times in
this file. What is left is the non-obvious part — that the newline join depends
on TerminalText.sanitized.
Docs corrected: Rename is single-target, so Copy Names is the FIRST item on a
multi-selection menu, not under Rename. .claude/rules/sidebar.md records the
action alongside the other batch items.
Dropping sessionDisplayNames/workspaceName from AppStore removes the literal trigger of the 'adding a user-facing action' rule, which asks for a control command, CLI and e2e for anything added there. dda1354 shipped exactly that for Duplicate Session, a row action of the same shape, so claiming an exemption while adding to AppStore was arguing both sides. The resolution is two lines at the call site instead; tree already exposes both names for any script. Coverage moves with it. The six core tests exercised two getters that no longer exist, and never touched the parts that can be wrong. SidebarCopyNameTests drives a real outline view and asserts what the menu actually produces: the derived name for an un-renamed session (customName is nil there, so copying it would yield ""), the plural title and newline join in sidebar order, and the two cases that must leave the clipboard untouched — a blank workspace name, and a row closed between building the menu and choosing from it. Also drops the Rename anchor from the comment, sidebar.md and docs.html. The code shows the placement, and README is the one surface a reader needs it on.
|
your self-review covers the control-API half. The keymap half of the description still stands as written, and it doesn't hold for session rows.
right-click does select the row. so one keymap line already does the session case: what survives is narrower than the description argues:
restate the case on those three and I'll take another look. two things from the rework itself:
|
Reverts the un-hoist. CLAUDE.md keeps derivation in agtermCore and the app target a side-effect adapter, continuing the umputun#78 series; owing a control command is not a reason to push a derivation into a view. sessionDisplayNames and workspaceName are back, with the blank-name rule in core where the other name gates live, and the menu handler is two lines again. testMultiSelection...InSidebarOrder passed for the wrong reason: it fed the ids in sidebar order, so raw selection order satisfied it too and the normalization it names was never exercised. Feeding them reversed pins AppStore+SidebarSelection's visible-order filter. Adds the composition test the newline join actually rests on. The three sanitize sites are each covered alone; nothing covered them together, so a fourth name source or a dropped sanitized() call would silently split a copied block. README corrected again: on a multi-selection Copy Names is not the first item — Clear Statuses and a separator sit above it whenever a selected session is non-idle.
It covered the rename and cwd-basename paths, not the OSC title — that one is sanitized on a live surface, and setting oscTitle directly on the store bypasses it, so a host-free test there would pass without proving anything. The old name claimed every source; the same overclaim the ordering test had.
|
Sorry, deleted my earlier comment while tidying without realising you were mid-review — folded it into the description instead. |
clearContents() destroys every representation, so putting back only .string loses an image, file URL or RTF the developer had, and leaves test text behind when the clipboard held no string at all. ControlAPITestCase.seedPasteboard already documents this for the UI-test target; same deep copy here, on addTeardownBlock so it also runs when an assertion fails.
|
Thanks — that was a useful correction, and all four points are addressed. Out of draft now. The keymap argument. You're right, and I'd verified it the wrong way round: I'd read The un-hoist. Reverted. README. Fixed — Clear Statuses and a separator sit above Copy Names whenever a selected session is non-idle, so it isn't the first item there either. Two more I found while rechecking my own claims: I'd cited Also tightened the tests: the ordering one passed for the wrong reason (it fed ids already in sidebar order, so raw order satisfied it too), and there's now one covering the composition the
|
|
go ahead with it, in the single-target form only. drop Copy Names. Nothing needs a list of session names, and matching how Flag/Close/Move treat a batch is not a reason on its own. Gate it on keep both row kinds. The workspace half is the only part not reachable today, so it earns the item; the session half is there for reach, since a no control command, though not on the capability argument. The socket does not write the user's clipboard: dropping the batch should also take out the newline join and what hangs off it, the sanitize composition test, the one process thing. This was hard to follow from my side: opened ready for review, then pushed changes while I was reading it, then moved to draft, then kept discussing inside the draft. I could not tell whether draft meant work in progress and I should stay out of it, or that you still wanted my opinion. pls keep it in draft while you iterate and mark it ready when you want eyes on it. I'll do the full pass then. |
|
Perfect, thank you! Moved to draft until ready for review. |
Nothing needs a list of session names, and matching how Flag/Close/Move treat a batch was the only reason offered. The item is now gated on sessionCount == 1 beside Rename, Duplicate Session and Reveal in Finder, so it is absent under a multi-selection rather than acting on one row while three are selected. That takes the newline join with it, and everything the join needed: the sanitize-composition test existed to prove the separator was unambiguous, and with no separator a name carrying a newline is just a name. sessionDisplayNames goes too — a single row reads session(withID:)?.displayName, which is the same public core accessor the Clear Status item on this menu already uses. workspaceName stays, since the blank-to-nil gate is a real derivation. sidebar.md is normative, so its entry now describes what ships: single-target, no join, and the gating decision stated so the next person does not have to re-derive it. README and docs.html lose the plural title, the one-per-line semantics and the Clear Statuses placement note. Tests drop to six. The two that went were the batch's; the one that arrived pins the gating, since an untested decision drifts back.
Stated as a rule the next person can apply rather than an argument re-made per PR, and in control-api.md rather than sidebar.md because it is about the socket, not the sidebar. The reason is the socket's existing shape, not whether the data is reachable: session copy returns the selection in the response precisely so it does not touch the clipboard, and session paste reads the clipboard as input. A command writing NSPasteboard.general would be the first to break that, and one returning the name would duplicate a field tree already carries.
The control-api rule ran five lines where the file's habit is two to three, and half of it restated the same point twice. The provenance sentence went with it — git carries that. The README entry ran three sentences against one for Reveal in Finder and one for Duplicate Session beside it. A reader wants what the item does; that an un-renamed session copies its derived name rather than a blank is behaviour they will never notice, and it is pinned by a test.
testTheItemIsAbsentUnderAMultiSelection asserted only an absence, so it held whenever sessionCount was anything but 1 — including when the selection never formed at all. It now asserts 'Close 2 Sessions' is present first, which is the premise the absence depends on. Third test in this file to claim more than it pinned; the other two were caught the same way. README said 'the name the row shows', which is false in flagged mode — a row there renders 'session : workspace' while Copy Name copies displayName alone. That phrasing was already corrected once and the trim reintroduced it. sidebar.md stated the single-target contract twice, once incompletely. Copy Name joins the existing Rename / Duplicate Session / Reveal in Finder list and the duplicate clause is gone. Dropped 'like Rename' with it: Rename's gate is <= 1 where this is == 1, so it named the one neighbour that does not share the guard.
The span wrapped mid-name, which renders but breaks highlighting in an editor. Rewrapped at the clause instead; all three lines stay inside the file's 109.
|
Addressed.
|
AppKit substitutes NSUserKeyEquivalents by menu-item TITLE as soon as the item joins a menu, including a detached one never installed in the menu bar. Both files build items titled after real system commands -- "Paste and Match Style", "Zoom", "Close" -- so a developer who rebound any of them in System Settings got a key equivalent replaced before the assertion ran, and the test failed on his machine alone. Reported on #385 as an unexplained macOS-version failure of testUppercaseKeyEquivalentReportsImpliedShift; it is neither version-specific nor environmental. Verified by probe: adding an item titled "Merge All Windows" to a detached menu on a machine with that shortcut bound turns keyEquivalent "V" into "m", and usesUserKeyEquivalents = false suppresses it. The titles stay as they are -- CloseSessionChordTests needs the real "Close" to test chord ownership against agterm's own item -- so the substitution is suppressed for the duration of each test class instead.
Upstream's umputun#385 for the GTK popovers: Copy Name under Rename on a session row and on a workspace row, single-target only, beside the other items gated the same way. It copies `Session.displayName`, so it agrees with the row, and `AppStore.workspaceName`, whose blank-to-nil gate already lives in core. A row gone between the right-click and the choice leaves the clipboard as the user had it rather than clearing it to write nothing. The target is the display clipboard, not the primary selection: this is an explicit copy, not a selection. No control command, for the reason `control-api.md` now records upstream.
2 participants
What is the problem?
A workspace's name cannot be got out of the app, and a session's cannot be got out of the sidebar. The row's
NSTextFieldis not selectable outside rename mode, so reading a name to reuse elsewhere means retyping it, or entering rename mode and copying out of the field — which risks committing an edit to a name you only wanted to read.For session rows a
keymap.confline already does it, and an earlier version of this description was wrong to claim otherwise:{AGT_SESSION_NAME}isSession.displayName(CustomCommandRunner.swift:265), the property the row's name comes from, and right-clicking a session row outside the selection selects it (WorkspaceSidebar.swift:775), so the clicked session is active by the time an action runs.For workspace rows there is no equivalent. That same guard is
node.kind == .session, so right-clicking a workspace row changes no selection, and{AGT_WORKSPACE_NAME}resolves the active session's workspace (CustomCommandRunner.swift:268, viastore.workspace(forSession:)) — which can be a different one. A workspace name can only be read by eye.How does this solve it?
Copy Name under Rename, on session and workspace rows. Single-target, gated on
sessionCount == 1like Duplicate Session and Reveal in Finder, so it is absent under a multi-selection.The workspace half is the part not reachable today, so it carries the feature. The session half is there for reach: a
keymap.confline only helps people who read the keymap docs.It copies
Session.displayName, so an un-renamed session yields its title or cwd basename rather than the emptycustomName. Names resolve through the sidebar's window-local store, like Close/Flag/Duplicate, so a background window copies its own row. A blank workspace name and a row closed between the right-click and the choice both leave the pasteboard untouched rather than clearing it.workspaceNamesits inagtermCorebecause the blank-to-nil gate is a model policy; the session case is the existingsession(withID:)plus the publisheddisplayName, with the app target doing only the pasteboard write.Control API
None, on your ground rather than mine — the socket never writes the user's clipboard.
session copyreturns the selection so it does not have to,session pastereads it as input, and a form that returned the name would duplicate whattreecarries. Written into.claude/rules/control-api.mdas a rule rather than an argument to re-make: an action whose only effect is a system-pasteboard write gets no control command.sidebar.mdcross-references it.Docs
README.md,site/docs.html,.claude/rules/sidebar.md,.claude/rules/control-api.md.CHANGELOG.mdleft alone.plugins/agterm/skills/agterm/andsite/commands.htmlunchanged — both document control commands only, and there is no command here.Verification
make buildsucceeds ·make test2377 passed ·make lintclean ·make test-app168 tests, one failure:LiveMenuKeyEquivalentsTests.testUppercaseKeyEquivalentReportsImpliedShift(), pre-existing and reproduced on unmodifiedmaster(macOS 26.5.2 / Xcode 26.6).Six tests in
agtermTests/SidebarCopyNameTests.swiftdrive a real outline view and Coordinator: the renamed and un-renamed session names, the workspace name, absence under a multi-selection, and the two cases that must leave the clipboard untouched — a blank workspace name, and a row closed between building the menu and choosing from it.