Timeline GoTo targets the mission, not the hidden Recordings tab - #1401
Open
vl3c wants to merge 2 commits into
Open
Timeline GoTo targets the mission, not the hidden Recordings tab#1401vl3c wants to merge 2 commits into
vl3c wants to merge 2 commits into
Conversation
The GoTo buttons on Timeline rows navigated to the raw Recordings tab, which Basic UI mode hides, so design 4.1a hid the buttons too. That left a Basic player with no way at all to get from a timeline row to the flight it belongs to - a core-loop action, not an advanced one. GoTo now opens the Missions tab, which Basic keeps, so the button is present and useful in both modes: - TimelineWindowUI calls RecordingsTableUI.ShowMissionForRecording, which force-opens the window, selects TabMissions, and delegates to the new MissionsWindowUI.RevealMissionForRecording. - Resolution is recording -> Recording.TreeId -> MissionStore.FindOriginalMission (the deterministic, name-linked original, never a clone). - The scroll reuses the recordings tab's two-frame handshake: the target header is located on a Repaint pass, the scroll is applied on a later one before BeginScrollView. The offset is the DIFFERENCE between the target header's y and the first header's y, so it is a distance between two rects rather than a raw layout coordinate. - Only the global MissionStore.HideArchived filter is cleared, and only when it is what would hide the target; Mission.Archived is a player decision this navigation never overwrites. - Every failure warns and lands the player on the tab unscrolled; a pending target is never left armed to fire on an unrelated frame. - The gate is kept, re-keyed to UiSurface.TabMissions. It hides nothing today; it exists so that re-pointing GoTo at a hidden surface would hide the button again instead of stranding the player. Two other Basic-mode dangling cross-links found by the same audit: - ParsekFlight's proximity alert told the player to open Real Spawn Control, whose only launcher Basic hides. The pure SelectiveSpawnUI.FormatProximityNotification now keeps the observation and drops the call to action when the window is unreachable, asking ParsekUI.IsSpawnControlReachable so the flight path never names the mode vocabulary. Written up as design section 9.1. - MergeDialog's post-merge seal guidance pointed at the "Recordings window": wrong name (it is Parsek - Missions) and wrong destination (Seal lives on the Timeline and in the Missions tab's Re-Fly cell). Retargeted to the Timeline. Plain wording fix, no mode read. ParsekUI.SelectedRecordingId, write-only dead state with zero readers, is removed along with its last two writers. Tests: Source/Parsek.Tests/TimelineGoToMissionTests.cs (12 cells) plus three FormatProximityNotification cells. Full xUnit suite and both grep gates green.
…w, de-brittle the source gate - RevealMissionForRecording now calls the idempotent MissionStore.EnsureDefaultsForTrees before resolving. A tree committed mid-scene (post-revert merge, rapid-switch commit) carries no Mission until the Missions tab's own draw seeds one, and the freshest flight is exactly the row a player is most likely to click GoTo on, so the common case was landing on the warn path one frame before the tab would have created the mission anyway. The warn is now the genuine backstop for a recording naming an uncommitted tree. - The empty-list early return now clears pendingRevealScrollY. Capture and apply straddle a frame boundary, so a list emptied in between (Wipe All Recordings, a load-time sweep) could leave an armed offset that applied silently whenever the list refilled. DropPendingReveal deliberately does not clear it: that also runs at the end of the pass that just captured one. - The source-text gate cell now matches whitespace-insensitively, so a line-wrap reformat of the gate or either call site cannot red it. Docs: corrected the test census in both docs (three headless-reachable failure paths, not four; the armed-but-never-drawn path is draw-loop-only), annotated the design 4.1 ScrollToRecording bullet as superseded, and recorded ParsekFlight's new indirect mode consumption in the grep-gate allowlist rationale. Tests: 13 cells, including a new one for the mid-scene seeding. Full suite and both grep gates green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The Timeline window's per-row
GoTobuttons navigated to the raw Recordings tab. Basic UI mode hides that tab, so design 4.1a hid the buttons too, which left a Basic player with no way at all to get from a timeline row to the flight it belongs to. That is a core-loop action, not an advanced one.Change
GoTonow opens the Missions tab, which Basic keeps, so the button is present and useful in both modes.TimelineWindowUIcalls the newRecordingsTableUI.ShowMissionForRecording, which force-opens the window, selectsTabMissions, and delegates to the newMissionsWindowUI.RevealMissionForRecording.Recording.TreeId->MissionStore.FindOriginalMission(the deterministic, name-linked original, never a clone). The reveal seeds the tree's default mission first via the idempotentEnsureDefaultsForTrees, so a tree committed mid-scene resolves instead of hitting the backstop.BeginScrollView. The offset is the difference between the target header's y and the first header's y, so it is a distance between two rects rather than a raw layout coordinate, and stays correct under non-uniform row heights.MissionStore.HideArchivedfilter is cleared, and only when it is what would hide the target.Mission.Archivedis a player decision this navigation never overwrites.UiSurface.TabMissions. It hides nothing today; it exists so that re-pointingGoToat a hidden surface would hide the button again instead of stranding the player.Other similar cases from the same audit
ParsekFlight's proximity alert told the player to open Real Spawn Control, whose only launcher Basic hides. The pureSelectiveSpawnUI.FormatProximityNotificationnow keeps the observation and drops the call to action when the window is unreachable, askingParsekUI.IsSpawnControlReachableso the flight path never names the mode vocabulary. Written up as new design section 9.1; the grep gate and the section 5 invariant are intact.MergeDialog's post-merge seal guidance pointed at the "Recordings window": wrong name (it isParsek - Missions) and wrong destination, since Seal lives on the Timeline's separation rows and in the Missions tab's Re-Fly cell. Retargeted to the Timeline. Plain wording fix, no mode read.ParsekUI.SelectedRecordingId, write-only dead state with zero readers, is removed along with its last two writers.Left open, deliberately
Recording.Hiddensuppresses timeline rows but every writer of that flag lives in the Recordings tab, so a recording hidden in Advanced vanishes from a Basic player's Timeline with no Basic control to bring it back. Fixing it means either having the Timeline ignoreHiddenin Basic or giving Basic an un-hide affordance; both change whatHiddenmeans to a Basic player, so it is recorded in the todo doc rather than decided here.ScrollToRecordingis retained but now has no production caller; whether to delete it and its scroll machinery is likewise left as a recorded follow-up.Validation
Full xUnit suite green (19506 passed, 1 pre-existing skip). Harness lib suite green (964). Both grep gates green. New
TimelineGoToMissionTests(13 cells) covers the happy path, the tab move off Recordings, mid-scene mission seeding, the original-not-clone pick, the three Archive-filter rules, the three headless-reachable failure paths, the stale-target clear, the gate key in both modes, and a whitespace-insensitive source gate over both button sites; three new cells cover the proximity-message wording. The IMGUI scroll itself is draw-loop-only and remains for operator playtest.One review pass (Fable) was run before the PR; its five findings are addressed in the second commit.
Docs updated in the same commits:
design-ui-basic-advanced.md(4.1a revised, new 9.1, v0.3),CHANGELOG.md,docs/dev/todo-and-known-bugs.md.