Repository navigation
fix(focus): a reopened window's focused terminal is presented first, and keys typed before its surface wait for it (REOPEN-F4) - #31707
Conversation
…nd gets the keys typed before its surface shows (REOPEN-F4, red) Three tests, red on the feat tip: - FocusedPaneShowsFirstTests: panes that come on screen together report the focused pane first, then layout order (the layout reported a Set's hash order). - FocusedPanePresentedFirstTests: the queued focused pane gets the next frame's one new surface, the others follow one per frame as before. - ColdFocusedTerminalTypeAheadTests (real daemon): a line typed in the turn a hibernated workspace shows cold, with the frame's surface budget spent, runs whole and in order in its focused terminal and never in the other one. On the feat tip, 10 of 15 Cmd-Q keep + reopen cycles on cmux21 lost the first key: the focus named the restored terminal, but its surface came 100-200 ms later, after another pane's.
…and keys typed before its surface wait for it (REOPEN-F4) On the feat tip the first key typed after a Cmd-Q keep + reopen was lost in 10 of 15 cycles on cmux21 (STATUS 4618). The focus named the restored terminal, but the panes got their surfaces one per frame in a Set's hash order, so the focused terminal's surface came 100-200 ms after the key, and the key reached no view. - LayoutRootView reports panes that come on screen together with the focused pane first, then layout order (was hash order). - ContentPresentationScheduler gives the queued focused pane the next frame's surface; the others keep their order and pace (one new surface per frame). - CreationInputCoordinator holds a key while the focus names a live terminal whose surface is not shown yet (the 1193 lane hold): the keys go to that terminal, in order, once its view has the keyboard. Bound: holdLimit (2 s), then they go to that terminal's view or, with no view, to its daemon terminal by id; a closed terminal takes them with it (logged). Never to another view. Lawrence's rule (cmux-ad 02:35Z): a restored window gives the keyboard to the restored focused terminal, and any key before that is held for it.
…App to CmuxNextFocusUI The Focus row of the destination map (architecture-refactor-DESIGN.md section 4). It names no App type; its users (InputVerificationService, debug.focus) import CmuxNextFocusUI. Pays for the REOPEN-F4 hold's App lines (arch ratchet app_lines).
…public import AppKit (InternalImportsByDefault)
|
Found 6 test failures on Blacksmith runners: Failures
|
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
web and size checks: green Shipped in:
|
…controller (LayoutPaneID is not in the test's scope)
|
Review (reopen-f4-reviewer-pr2): SHIP at 2adbcf9 Independent correctness review of What I checked
Non-blocking notes (follow-ups)
|
…aits for its focused terminal (review of #31707, red) An empty surface hold let a Ctrl/Cmd chord through (the creation-hold rule for held Ctrl-Tab), so a first Ctrl-L, Ctrl-C or Ctrl-R reached no view and was lost.
…ves a second cold terminal its own lane (review of #31707) - A Ctrl/Cmd chord no app shortcut takes (Ctrl-C, Ctrl-L, Ctrl-R) is the awaited terminal's key: it waits with the others, also as the first key of an empty hold (the creation-hold rule let it through, and it reached no view). App shortcuts (Cmd-T, Cmd-W, Cmd-Q) still act at once. - The focus moves to another terminal whose surface is not shown yet while the hold waits: that terminal gets its own lane, so its keys go to it, also at the hold's bound.
|
Review (reopen-f4-reviewer-pr2): SHIP at 785c45d This is a delta review of What I checked
Non-blocking notes
|
#31621 added it to the protocol)
|
Review (reopen-f4-reviewer-pr2): SHIP at f40f3c6 This is a delta review of What I checked
No new findings. My notes from the earlier receipts are still valid. |
…d or moved (#31870) * swift-logic-classes: classify the declarations #31795 and #31707 added or moved #31795 (Chief info Computer card shows the Cloud machine's screen) added CloudDesktopSurfaceServer, CloudDesktopSurfaceSources, CloudDesktopFrames, the phone directory rules and MobileHostCloudLinks with no table rows, so the default rule counted them in full as logic (+505 on the swift-logic tip row). Each is now read and classified: the loopback listener, the CVPixelBuffer and VideoToolbox parts and the loggers are PLATFORM; the sf/1 session rules, the grant and ownership rules, the VNC session lifecycle and the moved-chat ledger stay LOGIC; the hand-written need_cloud_links answer type is MIRROR. #31707 moved NoActivateKeyboardGuard to CmuxNextFocusUI and split AppKitKeyboardGuardHost into its own file; their PLATFORM rows follow them. Five stale rows (files moved or an extension removed) are removed. At e88c4a5-based HEAD: swift_logic_lines 415848 -> 415524 (-324), mirror_lines 70561 -> 70590 (+29). This is a classification of new code, not a code change; the logic parts stay counted and are paid by backlog E2 and F14. --missing: none; table_rows_invalid: 0; no unlisted CmuxNext declaration. * swift-logic-classes: the phone Cloud desktop access rule in AppServices+Cloud counts as logic (review of #31870) The reviewer found that #31795 added phoneCloudDesktop (the access rule: setting gate, one owned link, same account, running machine, live check; a Swift copy of hostd's cloud_desktop_access, rule L3) and phoneCloudLinks to the extension AppServices #1 row, which stayed PLATFORM 0. It is now MIXED 60. The CloudDesktopSurfaceSources reason now names what the class holds: the tab-key parse (a copy of hostd's cloud_desktop_of) and the refusal codes. swift_logic_lines at head: 415524 -> 415563 (+39).
|
Key-dispatch A/B for this PR (agent reopen-f4; follow-up the lead asked for: each key-down now does one more check, Host: cmux21, 120 Hz virtual display (
Values are the median over rounds of each round's p50 and p95, and the max over all rounds, in ms.
Verdict: no p95 is over budget. Dispatch p95 is at most 0.67 ms (key-dispatch budget 0.5 ms is for Not measured: the light load phase and light rounds 2-3 gave no probe rows in BOTH arms (hangs 100-415 with 20 busy terminals and 2 streaming chats in the light session). That is a bench gap and not this change. The STRESS session (60 workspaces) measured all its rows. |
|
From dogfood-ports, for the REOPEN-F4 owner: two key-routing defects where #31865 (a scripted new terminal never takes a typing person's keys) meets this PR's surface hold. Found by the merge review of #31865 (#31865 (comment)). Full steps are in STATUS 5995. (1) A key typed for a person's new workspace goes to a cold terminal whose surface hold is open in the same window: |
…ring a surface hold; a replayed key held for a terminal reaches it Review of #31865 (its merge with #31707): 1. CreationInputBuffer.capture: an adopted lane (a cold terminal's surface hold, keys carried from another hold) gives way to the person's new-workspace ticket. Keys typed after they asked for the workspace are its terminal's; keys typed before stay the cold terminal's. 2. CreationInputCoordinator: a replayed key that was held for a terminal (it runs again as typed because that terminal has the keyboard) goes to it; the person's keyboard claim keeps only replayed keys typed for no terminal. Before, a hold that ended at its limit while the new workspace was hidden lost the focused terminal's held line. CmuxNextApp lines: +0 (the replay flag carries the lane in the same lines). Regression tests: the two tests of the parent commit (red there).
…rminal; a held line for the focused terminal is never lost (#32098) * test(focus): two key-routing gaps where the person's new-workspace hold meets the #31707 surface hold (red) Review of #31865 (its merge with #31707, comment 6572422551): 1. CreationInputHoldTests.keysForAPersonsNewWorkspaceTypedDuringASurfaceHoldGoToTheNewTerminal: a key typed into a cold terminal opens its surface hold (an adopted lane); the person then asks for a new workspace. CreationInputBuffer.capture prefers the focused (adopted) lane, so the keys typed for the new workspace go to the cold terminal. 2. SecondCreationKeysTests.keysHeldForTheFocusedTerminalReachItWhileAPersonsNewWorkspaceClaimHolds: keys held for the terminal that has the keyboard run again as typed when the hold ends at its limit; while the person's claim holds (the new workspace never showed) the claim keeps every such key that runs no binding, so the line is lost. Both fail on this commit and pass with the fix. * fix(focus): the person's new workspace takes the keys typed for it during a surface hold; a replayed key held for a terminal reaches it Review of #31865 (its merge with #31707): 1. CreationInputBuffer.capture: an adopted lane (a cold terminal's surface hold, keys carried from another hold) gives way to the person's new-workspace ticket. Keys typed after they asked for the workspace are its terminal's; keys typed before stay the cold terminal's. 2. CreationInputCoordinator: a replayed key that was held for a terminal (it runs again as typed because that terminal has the keyboard) goes to it; the person's keyboard claim keeps only replayed keys typed for no terminal. Before, a hold that ended at its limit while the new workspace was hidden lost the focused terminal's held line. CmuxNextApp lines: +0 (the replay flag carries the lane in the same lines). Regression tests: the two tests of the parent commit (red there).
…l keeps its keys and a later creation gets its own Review of #32148 (comment 6575210178), two lost-key blockers: 1. passesOn skipped the REOPEN-F4 surface hold (#31707): with the left workspace's hold still open, a cold terminal that the person clicked to got no surface lane, and the keys typed at once went on to a window where no terminal view had them. Now a hold whose every creation the person left (CreationInputBuffer.personLeftEveryCreation) takes the focused terminal's surface lane as no hold would, and a key passes on only while the focused view takes keys (responderTakesKeys; a remount waits). 2. A creation asked for after the click (Cmd-T's non-optimistic path, no tab before its reply) set the hold's generation again, so capture gave its keys the left workspace's lane (workspaceTickets.max()), and they were dropped when that workspace never showed. capture now never picks a workspace the person left (workspaceTickets.subtracting(movedOnFrom)). #32030's browser page hold, once it lands, needs the same rule (a page still to show keeps the key); its owner is told. Regression tests (red on the parent commit): ColdFocusedTerminalTypeAheadTests.aLineTypedAsAColdWorkspaceShowsAfterTheClickThatLeftANewWorkspaceReachesItsTerminal, PersonTerminalKeyboardClaimTests.keysTypedForACreationAfterTheClickNeverGoToTheLeftWorkspace.
…; keys typed after it reach that pane (#32148) * test(focus): two key-routing gaps where the person's new-workspace hold meets the #31707 surface hold (red) Review of #31865 (its merge with #31707, comment 6572422551): 1. CreationInputHoldTests.keysForAPersonsNewWorkspaceTypedDuringASurfaceHoldGoToTheNewTerminal: a key typed into a cold terminal opens its surface hold (an adopted lane); the person then asks for a new workspace. CreationInputBuffer.capture prefers the focused (adopted) lane, so the keys typed for the new workspace go to the cold terminal. 2. SecondCreationKeysTests.keysHeldForTheFocusedTerminalReachItWhileAPersonsNewWorkspaceClaimHolds: keys held for the terminal that has the keyboard run again as typed when the hold ends at its limit; while the person's claim holds (the new workspace never showed) the claim keeps every such key that runs no binding, so the line is lost. Both fail on this commit and pass with the fix. * fix(focus): the person's new workspace takes the keys typed for it during a surface hold; a replayed key held for a terminal reaches it Review of #31865 (its merge with #31707): 1. CreationInputBuffer.capture: an adopted lane (a cold terminal's surface hold, keys carried from another hold) gives way to the person's new-workspace ticket. Keys typed after they asked for the workspace are its terminal's; keys typed before stay the cold terminal's. 2. CreationInputCoordinator: a replayed key that was held for a terminal (it runs again as typed because that terminal has the keyboard) goes to it; the person's keyboard claim keeps only replayed keys typed for no terminal. Before, a hold that ended at its limit while the new workspace was hidden lost the focused terminal's held line. CmuxNextApp lines: +0 (the replay flag carries the lane in the same lines). Regression tests: the two tests of the parent commit (red there). * test(focus): keys typed after a click on the focused pane reach that pane, not the new workspace (red) Review of #32098 (finding 1): the person asks for a new workspace, then clicks the pane that already has the focus. The click ends their keyboard claim but moves no focus generation (no responder change), so the keys typed after it still take the new workspace's lane through `buffer.newestGeneration == generation`, and they are dropped when the workspace never shows. Fails on this commit: b and c are held and lost. * fix(focus): a click on the focused pane ends the new workspace's lane; the keys typed after it reach that pane Review of #32098 (finding 1): the person asks for a new workspace, then clicks the pane that already has the focus. The click ended their keyboard claim, but it moves no focus generation (the responder does not change), so the keys typed after it still took the new workspace's lane through `buffer.newestGeneration == generation`, and were dropped when the workspace never showed. When the person's action ends their claim (personActed: the app's one mouse-down subscription, or one of their actions), the window's hold records that they moved on from its new workspaces (CreationInputBuffer.personMovedOn): the generation that kept the lane is cleared, and a key typed now passes on to the focused view at once when every creation of the hold is such a workspace, the key is for no creation's or surface's terminal, and no key that runs as typed waits before it. Keys typed before the click still wait for the new workspace. The rule is in CmuxNextFocusUI; CmuxNextApp lines do not rise. Regression test: PersonTerminalKeyboardClaimTests.keysTypedAfterAClickOnTheFocusedPaneReachThatPane (red on the parent commit). * test(focus): keys after the click that left a new workspace wait for a cold terminal and go to a later creation (red) Review of #32148 (comment 6575210178), two lost-key blockers: (a) ColdFocusedTerminalTypeAheadTests.aLineTypedAsAColdWorkspaceShowsAfterTheClickThatLeftANewWorkspaceReachesItsTerminal: the person's new workspace's hold is open and their click ended its claim as another workspace shows cold; a line typed at once must wait for the focused terminal's surface and run there. With passesOn the keys go on to the window, where no terminal view has them. (b) PersonTerminalKeyboardClaimTests.keysTypedForACreationAfterTheClickNeverGoToTheLeftWorkspace: after the click, a creation with no tab before its reply (Cmd-T's non-optimistic path) takes the next keys; capture gave them the left workspace's lane (workspaceTickets.max()), and they were dropped when it never showed. * fix(focus): after the click that left a new workspace, a cold terminal keeps its keys and a later creation gets its own Review of #32148 (comment 6575210178), two lost-key blockers: 1. passesOn skipped the REOPEN-F4 surface hold (#31707): with the left workspace's hold still open, a cold terminal that the person clicked to got no surface lane, and the keys typed at once went on to a window where no terminal view had them. Now a hold whose every creation the person left (CreationInputBuffer.personLeftEveryCreation) takes the focused terminal's surface lane as no hold would, and a key passes on only while the focused view takes keys (responderTakesKeys; a remount waits). 2. A creation asked for after the click (Cmd-T's non-optimistic path, no tab before its reply) set the hold's generation again, so capture gave its keys the left workspace's lane (workspaceTickets.max()), and they were dropped when that workspace never showed. capture now never picks a workspace the person left (workspaceTickets.subtracting(movedOnFrom)). #32030's browser page hold, once it lands, needs the same rule (a page still to show keeps the key); its owner is told. Regression tests (red on the parent commit): ColdFocusedTerminalTypeAheadTests.aLineTypedAsAColdWorkspaceShowsAfterTheClickThatLeftANewWorkspaceReachesItsTerminal, PersonTerminalKeyboardClaimTests.keysTypedForACreationAfterTheClickNeverGoToTheLeftWorkspace. --------- Co-authored-by: cmux-bot[bot] <348531757+cmux-bot[bot]@users.noreply.github.com>
… releases the wait for a left workspace (#32352) * test(focus): two key-routing gaps where the person's new-workspace hold meets the #31707 surface hold (red) Review of #31865 (its merge with #31707, comment 6572422551): 1. CreationInputHoldTests.keysForAPersonsNewWorkspaceTypedDuringASurfaceHoldGoToTheNewTerminal: a key typed into a cold terminal opens its surface hold (an adopted lane); the person then asks for a new workspace. CreationInputBuffer.capture prefers the focused (adopted) lane, so the keys typed for the new workspace go to the cold terminal. 2. SecondCreationKeysTests.keysHeldForTheFocusedTerminalReachItWhileAPersonsNewWorkspaceClaimHolds: keys held for the terminal that has the keyboard run again as typed when the hold ends at its limit; while the person's claim holds (the new workspace never showed) the claim keeps every such key that runs no binding, so the line is lost. Both fail on this commit and pass with the fix. * fix(focus): the person's new workspace takes the keys typed for it during a surface hold; a replayed key held for a terminal reaches it Review of #31865 (its merge with #31707): 1. CreationInputBuffer.capture: an adopted lane (a cold terminal's surface hold, keys carried from another hold) gives way to the person's new-workspace ticket. Keys typed after they asked for the workspace are its terminal's; keys typed before stay the cold terminal's. 2. CreationInputCoordinator: a replayed key that was held for a terminal (it runs again as typed because that terminal has the keyboard) goes to it; the person's keyboard claim keeps only replayed keys typed for no terminal. Before, a hold that ended at its limit while the new workspace was hidden lost the focused terminal's held line. CmuxNextApp lines: +0 (the replay flag carries the lane in the same lines). Regression tests: the two tests of the parent commit (red there). * test(focus): keys typed after a click on the focused pane reach that pane, not the new workspace (red) Review of #32098 (finding 1): the person asks for a new workspace, then clicks the pane that already has the focus. The click ends their keyboard claim but moves no focus generation (no responder change), so the keys typed after it still take the new workspace's lane through `buffer.newestGeneration == generation`, and they are dropped when the workspace never shows. Fails on this commit: b and c are held and lost. * fix(focus): a click on the focused pane ends the new workspace's lane; the keys typed after it reach that pane Review of #32098 (finding 1): the person asks for a new workspace, then clicks the pane that already has the focus. The click ended their keyboard claim, but it moves no focus generation (the responder does not change), so the keys typed after it still took the new workspace's lane through `buffer.newestGeneration == generation`, and were dropped when the workspace never showed. When the person's action ends their claim (personActed: the app's one mouse-down subscription, or one of their actions), the window's hold records that they moved on from its new workspaces (CreationInputBuffer.personMovedOn): the generation that kept the lane is cleared, and a key typed now passes on to the focused view at once when every creation of the hold is such a workspace, the key is for no creation's or surface's terminal, and no key that runs as typed waits before it. Keys typed before the click still wait for the new workspace. The rule is in CmuxNextFocusUI; CmuxNextApp lines do not rise. Regression test: PersonTerminalKeyboardClaimTests.keysTypedAfterAClickOnTheFocusedPaneReachThatPane (red on the parent commit). * test(focus): keys after the click that left a new workspace wait for a cold terminal and go to a later creation (red) Review of #32148 (comment 6575210178), two lost-key blockers: (a) ColdFocusedTerminalTypeAheadTests.aLineTypedAsAColdWorkspaceShowsAfterTheClickThatLeftANewWorkspaceReachesItsTerminal: the person's new workspace's hold is open and their click ended its claim as another workspace shows cold; a line typed at once must wait for the focused terminal's surface and run there. With passesOn the keys go on to the window, where no terminal view has them. (b) PersonTerminalKeyboardClaimTests.keysTypedForACreationAfterTheClickNeverGoToTheLeftWorkspace: after the click, a creation with no tab before its reply (Cmd-T's non-optimistic path) takes the next keys; capture gave them the left workspace's lane (workspaceTickets.max()), and they were dropped when it never showed. * fix(focus): after the click that left a new workspace, a cold terminal keeps its keys and a later creation gets its own Review of #32148 (comment 6575210178), two lost-key blockers: 1. passesOn skipped the REOPEN-F4 surface hold (#31707): with the left workspace's hold still open, a cold terminal that the person clicked to got no surface lane, and the keys typed at once went on to a window where no terminal view had them. Now a hold whose every creation the person left (CreationInputBuffer.personLeftEveryCreation) takes the focused terminal's surface lane as no hold would, and a key passes on only while the focused view takes keys (responderTakesKeys; a remount waits). 2. A creation asked for after the click (Cmd-T's non-optimistic path, no tab before its reply) set the hold's generation again, so capture gave its keys the left workspace's lane (workspaceTickets.max()), and they were dropped when that workspace never showed. capture now never picks a workspace the person left (workspaceTickets.subtracting(movedOnFrom)). #32030's browser page hold, once it lands, needs the same rule (a page still to show keeps the key); its owner is told. Regression tests (red on the parent commit): ColdFocusedTerminalTypeAheadTests.aLineTypedAsAColdWorkspaceShowsAfterTheClickThatLeftANewWorkspaceReachesItsTerminal, PersonTerminalKeyboardClaimTests.keysTypedForACreationAfterTheClickNeverGoToTheLeftWorkspace. * test(focus): after a failed new workspace and a click, keys go on to the clicked pane (red) Review of #32148 (comment 6576859579), note 2: two new workspaces in one hold, the first one's create fails, then the person clicks the focused pane. The failed ticket is no longer a workspace ticket but still counts in nextID, so personLeftEveryCreation stays false: the keys typed after the click wait without a lane and run as typed when the hold ends, into the second workspace if it shows. Fails on this commit: b and c are held. * fix(focus): a failed new workspace releases its ticket at once, so the keys after the click go on to the clicked pane Review of #32148 (comment 6576859579), note 2: a new-workspace creation whose create failed left its id in the hold's count but not in its workspace tickets, so after the person's click the hold never counted as left (personLeftEveryCreation), and the keys typed after the click waited without a lane, to run as typed when the hold ended (into a second new workspace if that one showed). A failed creation is now recorded as over (failedWorkspaces) and counts as released; the person's action is still needed (their claim keeps the keys until then). Regression test: PersonTerminalKeyboardClaimTests.keysTypedAfterTheClickPassOnWhenAnEarlierNewWorkspaceFailed (red on the parent commit). * test(focus): a left workspace that never shows does not delay a later creation's keys (red) Review of #32148 (comment 6576859579), note 1: the person's new workspace replied but the window never shows it; the person clicks, asks for another creation and types. awaitedWorkspace still names the left workspace, so the flush waits until the hold's limit (2 s by default; 8 s in the test) before the creation's keys run. Fails on this commit: the keys do not run within 2 s. * fix(focus): a click releases the hold's wait for a left workspace that never shows Review of #32148 (comment 6576859579), note 1: the hold's flush waited for the window to show the newest new workspace (awaitedWorkspace) even after the person left it with a click. For a workspace that replied and never shows, the keys of a later creation (Cmd-T), and a cold terminal's lane keys, waited until the hold's limit (2 s). The flush now waits only for a workspace the person did not leave (shownWorkspaceToWaitFor). The keys typed for the left workspace before the click go with it when the hold ends: they run there if it shows by then, else they are dropped, as at the limit before. Regression test: PersonTerminalKeyboardClaimTests.aLeftWorkspaceThatNeverShowsDoesNotDelayALaterCreationsKeys (red on the parent commit). * test(focus): the failed-workspace rule (note 2) is tested where it lives, in CreationInputBuffer The App-level test could not tell the fix from the bug: the test window's focused terminal has no surface, so after the click its keys take that terminal's surface lane (REOPEN-F4) and replay at the hold's end, which is the right target, and before the fix they also replayed (as typed). The rule itself is CreationInputBuffer's: CreationInputHoldTests.aFailedNewWorkspaceCountsAsLeftOnceThePersonActs checks personLeftEveryCreation and passesOn. Red on the base before the fix (red ref branch fix/creation-hold-left-workspace-release-red, never merge). * test(focus): a line typed for a new workspace before the click runs there when it shows a frame after the reply (red) Review of #32352 (comment 6580371831), blocker: Cmd-N, type, click, then the create's reply while the window does not show the workspace yet; the store applies daemon events once per frame, so it shows one frame later. The flush no longer waits for a workspace the person left (shownWorkspaceToWaitFor), so in the reply's turn it dropped the line (dropKeys(ofWorkspacesOtherThan:)). Fails on this commit: the line never runs in the new workspace's terminal. * fix(focus): a left new workspace's own keys wait for it to show with its terminal; the other lanes' keys go now Review of #32352 (comment 6580371831), blocker: the flush no longer waited for a new workspace the person left (note 1), so the keys typed for it before the click were dropped in the create reply's turn. The store applies daemon events once per frame, so the window usually shows the new workspace a frame after the reply. Now a left workspace that replied and still holds keys typed for it (CreationInputBuffer.leftWorkspaceWithKeys) keeps only those keys waiting: until the window shows it with its terminal (then they are bound to its first terminal and run there), or until the hold's limit (then they are dropped, as before). The keys of every other lane run at once (take(focused:keeping:)), so note 1's gain stays: a left workspace with no keys of its own keeps nothing waiting. Docs of open(...) and personMovedOn() and shownWorkspaceToWaitFor say so. Regression test: ColdFocusedTerminalTypeAheadTests.aLineTypedForANewWorkspaceBeforeTheClickRunsThereWhenItShowsAFrameAfterTheReply (red on the parent commit). --------- Co-authored-by: cmux-bot[bot] <348531757+cmux-bot[bot]@users.noreply.github.com>
…'s keys (#32478) * test(focus): two key-routing gaps where the person's new-workspace hold meets the #31707 surface hold (red) Review of #31865 (its merge with #31707, comment 6572422551): 1. CreationInputHoldTests.keysForAPersonsNewWorkspaceTypedDuringASurfaceHoldGoToTheNewTerminal: a key typed into a cold terminal opens its surface hold (an adopted lane); the person then asks for a new workspace. CreationInputBuffer.capture prefers the focused (adopted) lane, so the keys typed for the new workspace go to the cold terminal. 2. SecondCreationKeysTests.keysHeldForTheFocusedTerminalReachItWhileAPersonsNewWorkspaceClaimHolds: keys held for the terminal that has the keyboard run again as typed when the hold ends at its limit; while the person's claim holds (the new workspace never showed) the claim keeps every such key that runs no binding, so the line is lost. Both fail on this commit and pass with the fix. * fix(focus): the person's new workspace takes the keys typed for it during a surface hold; a replayed key held for a terminal reaches it Review of #31865 (its merge with #31707): 1. CreationInputBuffer.capture: an adopted lane (a cold terminal's surface hold, keys carried from another hold) gives way to the person's new-workspace ticket. Keys typed after they asked for the workspace are its terminal's; keys typed before stay the cold terminal's. 2. CreationInputCoordinator: a replayed key that was held for a terminal (it runs again as typed because that terminal has the keyboard) goes to it; the person's keyboard claim keeps only replayed keys typed for no terminal. Before, a hold that ended at its limit while the new workspace was hidden lost the focused terminal's held line. CmuxNextApp lines: +0 (the replay flag carries the lane in the same lines). Regression tests: the two tests of the parent commit (red there). * test(focus): keys typed after a click on the focused pane reach that pane, not the new workspace (red) Review of #32098 (finding 1): the person asks for a new workspace, then clicks the pane that already has the focus. The click ends their keyboard claim but moves no focus generation (no responder change), so the keys typed after it still take the new workspace's lane through `buffer.newestGeneration == generation`, and they are dropped when the workspace never shows. Fails on this commit: b and c are held and lost. * fix(focus): a click on the focused pane ends the new workspace's lane; the keys typed after it reach that pane Review of #32098 (finding 1): the person asks for a new workspace, then clicks the pane that already has the focus. The click ended their keyboard claim, but it moves no focus generation (the responder does not change), so the keys typed after it still took the new workspace's lane through `buffer.newestGeneration == generation`, and were dropped when the workspace never showed. When the person's action ends their claim (personActed: the app's one mouse-down subscription, or one of their actions), the window's hold records that they moved on from its new workspaces (CreationInputBuffer.personMovedOn): the generation that kept the lane is cleared, and a key typed now passes on to the focused view at once when every creation of the hold is such a workspace, the key is for no creation's or surface's terminal, and no key that runs as typed waits before it. Keys typed before the click still wait for the new workspace. The rule is in CmuxNextFocusUI; CmuxNextApp lines do not rise. Regression test: PersonTerminalKeyboardClaimTests.keysTypedAfterAClickOnTheFocusedPaneReachThatPane (red on the parent commit). * test(focus): keys after the click that left a new workspace wait for a cold terminal and go to a later creation (red) Review of #32148 (comment 6575210178), two lost-key blockers: (a) ColdFocusedTerminalTypeAheadTests.aLineTypedAsAColdWorkspaceShowsAfterTheClickThatLeftANewWorkspaceReachesItsTerminal: the person's new workspace's hold is open and their click ended its claim as another workspace shows cold; a line typed at once must wait for the focused terminal's surface and run there. With passesOn the keys go on to the window, where no terminal view has them. (b) PersonTerminalKeyboardClaimTests.keysTypedForACreationAfterTheClickNeverGoToTheLeftWorkspace: after the click, a creation with no tab before its reply (Cmd-T's non-optimistic path) takes the next keys; capture gave them the left workspace's lane (workspaceTickets.max()), and they were dropped when it never showed. * fix(focus): after the click that left a new workspace, a cold terminal keeps its keys and a later creation gets its own Review of #32148 (comment 6575210178), two lost-key blockers: 1. passesOn skipped the REOPEN-F4 surface hold (#31707): with the left workspace's hold still open, a cold terminal that the person clicked to got no surface lane, and the keys typed at once went on to a window where no terminal view had them. Now a hold whose every creation the person left (CreationInputBuffer.personLeftEveryCreation) takes the focused terminal's surface lane as no hold would, and a key passes on only while the focused view takes keys (responderTakesKeys; a remount waits). 2. A creation asked for after the click (Cmd-T's non-optimistic path, no tab before its reply) set the hold's generation again, so capture gave its keys the left workspace's lane (workspaceTickets.max()), and they were dropped when that workspace never showed. capture now never picks a workspace the person left (workspaceTickets.subtracting(movedOnFrom)). #32030's browser page hold, once it lands, needs the same rule (a page still to show keeps the key); its owner is told. Regression tests (red on the parent commit): ColdFocusedTerminalTypeAheadTests.aLineTypedAsAColdWorkspaceShowsAfterTheClickThatLeftANewWorkspaceReachesItsTerminal, PersonTerminalKeyboardClaimTests.keysTypedForACreationAfterTheClickNeverGoToTheLeftWorkspace. * test(focus): after a failed new workspace and a click, keys go on to the clicked pane (red) Review of #32148 (comment 6576859579), note 2: two new workspaces in one hold, the first one's create fails, then the person clicks the focused pane. The failed ticket is no longer a workspace ticket but still counts in nextID, so personLeftEveryCreation stays false: the keys typed after the click wait without a lane and run as typed when the hold ends, into the second workspace if it shows. Fails on this commit: b and c are held. * fix(focus): a failed new workspace releases its ticket at once, so the keys after the click go on to the clicked pane Review of #32148 (comment 6576859579), note 2: a new-workspace creation whose create failed left its id in the hold's count but not in its workspace tickets, so after the person's click the hold never counted as left (personLeftEveryCreation), and the keys typed after the click waited without a lane, to run as typed when the hold ended (into a second new workspace if that one showed). A failed creation is now recorded as over (failedWorkspaces) and counts as released; the person's action is still needed (their claim keeps the keys until then). Regression test: PersonTerminalKeyboardClaimTests.keysTypedAfterTheClickPassOnWhenAnEarlierNewWorkspaceFailed (red on the parent commit). * test(focus): a left workspace that never shows does not delay a later creation's keys (red) Review of #32148 (comment 6576859579), note 1: the person's new workspace replied but the window never shows it; the person clicks, asks for another creation and types. awaitedWorkspace still names the left workspace, so the flush waits until the hold's limit (2 s by default; 8 s in the test) before the creation's keys run. Fails on this commit: the keys do not run within 2 s. * fix(focus): a click releases the hold's wait for a left workspace that never shows Review of #32148 (comment 6576859579), note 1: the hold's flush waited for the window to show the newest new workspace (awaitedWorkspace) even after the person left it with a click. For a workspace that replied and never shows, the keys of a later creation (Cmd-T), and a cold terminal's lane keys, waited until the hold's limit (2 s). The flush now waits only for a workspace the person did not leave (shownWorkspaceToWaitFor). The keys typed for the left workspace before the click go with it when the hold ends: they run there if it shows by then, else they are dropped, as at the limit before. Regression test: PersonTerminalKeyboardClaimTests.aLeftWorkspaceThatNeverShowsDoesNotDelayALaterCreationsKeys (red on the parent commit). * test(focus): the failed-workspace rule (note 2) is tested where it lives, in CreationInputBuffer The App-level test could not tell the fix from the bug: the test window's focused terminal has no surface, so after the click its keys take that terminal's surface lane (REOPEN-F4) and replay at the hold's end, which is the right target, and before the fix they also replayed (as typed). The rule itself is CreationInputBuffer's: CreationInputHoldTests.aFailedNewWorkspaceCountsAsLeftOnceThePersonActs checks personLeftEveryCreation and passesOn. Red on the base before the fix (red ref branch fix/creation-hold-left-workspace-release-red, never merge). * test(focus): a line typed for a new workspace before the click runs there when it shows a frame after the reply (red) Review of #32352 (comment 6580371831), blocker: Cmd-N, type, click, then the create's reply while the window does not show the workspace yet; the store applies daemon events once per frame, so it shows one frame later. The flush no longer waits for a workspace the person left (shownWorkspaceToWaitFor), so in the reply's turn it dropped the line (dropKeys(ofWorkspacesOtherThan:)). Fails on this commit: the line never runs in the new workspace's terminal. * fix(focus): a left new workspace's own keys wait for it to show with its terminal; the other lanes' keys go now Review of #32352 (comment 6580371831), blocker: the flush no longer waited for a new workspace the person left (note 1), so the keys typed for it before the click were dropped in the create reply's turn. The store applies daemon events once per frame, so the window usually shows the new workspace a frame after the reply. Now a left workspace that replied and still holds keys typed for it (CreationInputBuffer.leftWorkspaceWithKeys) keeps only those keys waiting: until the window shows it with its terminal (then they are bound to its first terminal and run there), or until the hold's limit (then they are dropped, as before). The keys of every other lane run at once (take(focused:keeping:)), so note 1's gain stays: a left workspace with no keys of its own keeps nothing waiting. Docs of open(...) and personMovedOn() and shownWorkspaceToWaitFor say so. Regression test: ColdFocusedTerminalTypeAheadTests.aLineTypedForANewWorkspaceBeforeTheClickRunsThereWhenItShowsAFrameAfterTheReply (red on the parent commit). * test(focus): a shown second new workspace ends the wait for a left one's keys (red) Review of #32352 (comment 6581603361), note 2: Cmd-N (W1), type, click, Cmd-N (W2). When the window shows W2 with its terminal, W1's kept keys (keysKeptForLeftWorkspace) still kept the hold open until its limit, so keys typed into W2's shown terminal waited up to 2 s; before #32352 the hold ended there and W1's keys were dropped. Fails on this commit. * fix(focus): a shown second new workspace ends the wait for a left one's keys Review of #32352 (comment 6581603361), note 2: Cmd-N (W1), type, click, Cmd-N (W2). Once the window shows W2 (the workspace the person did not leave), keysKeptForLeftWorkspace returns nil: the hold ends there as it did before #32352, so the keys typed into W2 go on at once, and W1's kept keys are dropped with the hold (its workspace is not the one shown), as before. While W2 has not shown, W1's own keys still wait for W1. Regression test: CreationInputHoldTests.aShownSecondWorkspaceEndsTheWaitForALeftOnesKeys (red on the parent commit).
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Summary
The first key a person types into a reopened window's focused terminal can arrive before that terminal's surface exists, and then it reaches no view. On the feat tip (d3feb066), 10 of 15 Cmd-Q keep + reopen cycles on cmux21 lost that key (REOPEN-F4, STATUS 4618). The 15 input journals show the rule: the key was kept if and only if the focused pane was the first pane the window showed.
Why it happens: a reopened window shows its panes one new surface per frame (
ContentPresentationScheduler), andLayoutRootView.updateVisibilityreported the panes that came on screen in aSet's hash order. So the focused terminal got the first frame in about 1 of 3 launches; in the others its surface came 100-200 ms later, after the key. Nothing held a key for a focused terminal whose surface was still to come (the hold existed only for a selection change,PaneSelectionPresenter).Lawrence's rule (cmux-ad 02:35Z): a restored window gives the keyboard to the restored focused terminal, and any key that arrives before that is held for that terminal (the 1193 lane design) and delivered in order, never dropped.
What changes:
LayoutRootViewreports panes that come on screen together with the focused pane first, then in layout order (never hash order).ContentPresentationSchedulergives a queued focused pane the next frame's surface. The other panes keep their order and their pace (one new surface per frame); nothing else waits longer.CreationInputCoordinatorholds a key-down while the focus names a live terminal of the shown workspace whose surface is not shown yet. The held keys are that terminal's lane (CreationInputBuffer.awaitSurface(of:)), and they run again in order once its view has the keyboard.holdLimit(2 s, as every creation hold). Then the keys go to that terminal's view if it has one, else to its daemon terminal by id (send-key, in order). A terminal that closed takes its keys with it, with a log line. They never go to another view. I chose delivery over a drop: the keys are the person's input for a terminal that exists; the daemon accepts them even with no view, as the 1193 lanes do.app_lines):NoActivateKeyboardGuard(AppKit only, no App types) moves toCmuxNextFocusUI(the Focus row of the destination map). Its AppKit host and the waivedNoActivateApplicationReferencestay in the App. Net CmuxNextApp −94 lines.Culprit (lead question, STATUS 4872): the dogfood line passes because it has its own fix of the sidebar half, 99c70f7 (
ShellWindow.refusesOrderInKeyView, never ported to feat); feat's equivalent #30356 never reached the app because of the stale CCmuxAppFFI pin, which #31684 fixes. This PR reverts nothing: it covers the other half, a key that comes before the focused terminal's surface exists.Review follow-up (SHIP at 2adbcf9, findings 1 and 2, fixed in 785c45d): a Ctrl/Cmd chord that no app shortcut takes (Ctrl-C, Ctrl-L, Ctrl-R) waits too, also as the first key of an empty hold; app shortcuts (Cmd-T, Cmd-W, Cmd-Q) still act at once. A second cold terminal the focus moves to meanwhile gets its own lane.
The other half of REOPEN-F4 (the focus itself went to the sidebar list that AppKit picks as the window comes forward, because the app's CCmuxAppFFI pin predates #30356) is #31684. The two PRs are independent; both are needed for the journey.
Owner note:
LayoutRootViewis PL-motion-core's (OWNERS.md); the change is only the order of the presence reports.Testing
Regression tests (committed first, 19119b7, red on the base):
FocusedPaneShowsFirstTests(layout): six panes come on screen together, the focused one last in layout order; the reports must be the focused pane, then layout order.FocusedPanePresentedFirstTests(scheduler, manual frames): the queued focused pane gets the next frame's surface; the others follow one per frame in their order.ColdFocusedTerminalTypeAheadTests(real daemon): a workspace with two terminals, the right one focused, hibernates; with the frame's surface budget spent, the window shows it cold, and a line is typed in that same turn. The line must run whole and in order in the focused terminal and never reach the other one.Fleet runs, 5 runs each (
package-test-lane.sh suite, each suite named 5 times, throughproof-step.py). Every ref carries the cmux-tui/Cargo.lock of base-red fix #31654 on top (proof only: the stale lock of #31462 stops the same-tree daemon the real-daemon tests need):[p2, p5, p3, p6, p4, p1]). Scheduler:["first", "a", "b", "focused"]. Cold workspace: the typed line never ran in the focused terminal (ran?.matched → nil). Chord: the screen was not cleared.PresentablePane.showWhileWaiting; the test fakes got an empty one, f40f3c6): head f40f3c6, compile job 9db120984fcfe247833308c9 passed, job 1eac526d941c59400e4ea144: all 4 tests pass 5 of 5.Observed / Not run
/work/tmp/reopen-video/rounds/r2-feat-marks-quit-keep-hid-light/(kitcp/reopen-video-kit@d76e5b72c7a): kept iff the focused pane showed first (15 of 15).prepush-gate.py: no new violations; app_lines −94.awaitedSurface; its common case is two dictionary reads and a tab compare).Changelog
Fixed: The first keys typed into a reopened window reach its focused terminal instead of being lost
Proof
No capture: input routing with no visual change; the proof is the test runs and the reopen journey.