diff --git a/graphcode/Sources/Features/App/AppFeature+Workspaces.swift b/graphcode/Sources/Features/App/AppFeature+Workspaces.swift index 27fa8231..80951327 100644 --- a/graphcode/Sources/Features/App/AppFeature+Workspaces.swift +++ b/graphcode/Sources/Features/App/AppFeature+Workspaces.swift @@ -136,8 +136,9 @@ extension AppFeature { case createConfirmed /// Raise (or launch) the instance that owns this workspace. case switchRequested(Workspace) - /// ⌘` and ⌘⇧` — the workspace `offset` places along from this one in creation order, - /// wrapping at both ends. + /// ⌘` and ⌘⇧` — the *running* workspace `offset` places along from this one in + /// creation order, wrapping at both ends. One with no instance is stepped over, never + /// launched. case cycleRequested(offset: Int) case switcherPresented(Bool) case manageRequested @@ -311,12 +312,20 @@ struct AppWorkspacesReducer: Reducer { // workspace another instance created since then is one ⌘` would skip over. let known = workspaces.list() state.workspaces.known = known - guard known.count > 1, - let index = known.firstIndex(where: { $0.id == state.workspaces.current.id }) + // Only the workspaces with a window. ⌘` is the system's cycle-*windows* key, and + // `switchRequested` launches whatever has no instance — so walking the whole + // list brought back the workspace someone had just quit (issue #330). The same + // question the update path asks before swapping the bundle, answered by the same + // client, filtered through `known` so creation order is kept. + let current = state.workspaces.current + let open = Set(workspaces.otherOpen().map(\.id)).union([current.id]) + let running = known.filter { open.contains($0.id) } + guard running.count > 1, + let index = running.firstIndex(where: { $0.id == current.id }) else { return .none } // `%` keeps a negative dividend negative in Swift, so ⌘⇧` at the head of the - // list would index -1 without the extra `+ known.count`. - let next = known[((index + offset) % known.count + known.count) % known.count] + // list would index -1 without the extra `+ running.count`. + let next = running[((index + offset) % running.count + running.count) % running.count] return .send(.workspaces(.switchRequested(next))) case .workspaces(.renameRequested(let workspace)): diff --git a/graphcode/Sources/Features/Workspaces/WorkspaceSwitcher.swift b/graphcode/Sources/Features/Workspaces/WorkspaceSwitcher.swift index 0d800bd7..a53569c9 100644 --- a/graphcode/Sources/Features/Workspaces/WorkspaceSwitcher.swift +++ b/graphcode/Sources/Features/Workspaces/WorkspaceSwitcher.swift @@ -35,9 +35,10 @@ struct WorkspaceMenuItems: View { .modifier(WorkspaceShortcut(index: showsShortcuts ? index : nil)) } Divider() - // ⌘` walks the same list the rows above are numbered in, which is the point of it: - // ⌥⌘ is for the workspace you can name, ⌘` for the next one along when you have - // more of them than you have fingers. + // ⌘` walks the list the rows above are numbered in, skipping any without a window: + // ⌥⌘ is for the workspace you can name, ⌘` for the next *open* one along when you + // have more of them than you have fingers. It never launches one — that is what the + // rows are for. Button("Next Workspace") { store.send(.workspaces(.cycleRequested(offset: 1))) } .modifier(CycleShortcut(isEnabled: showsShortcuts, isBackwards: false)) .disabled(store.workspaces.known.count < 2) diff --git a/graphcode/Tests/WorkspaceCycleTests.swift b/graphcode/Tests/WorkspaceCycleTests.swift index 0745b3e4..d9161531 100644 --- a/graphcode/Tests/WorkspaceCycleTests.swift +++ b/graphcode/Tests/WorkspaceCycleTests.swift @@ -7,10 +7,12 @@ import Testing /// ⌘` and ⌘⇧` — stepping to the next workspace rather than naming one with ⌥⌘. /// -/// Issue #175. The list these walk is `Workspace.all()`, which is in creation order; what -/// is pinned here is the walking itself — that it wraps at both ends, that it re-reads the -/// list rather than trusting whatever the last menu opening left in state, and that a -/// machine with one workspace gets nothing rather than a switch to itself. +/// Issue #175. The list these walk is `Workspace.all()`, which is in creation order, +/// narrowed to the workspaces that have a window (issue #330 — the ones the update path's +/// `otherOpen` reports, plus this one). What is pinned here is the walking itself — that +/// it wraps at both ends, that it re-reads the list rather than trusting whatever the last +/// menu opening left in state, that a machine with one workspace gets nothing rather than +/// a switch to itself, and that a workspace someone quit stays quit. @Suite struct WorkspaceCycleTests { private func workspace(_ slug: String) -> Workspace { @@ -23,14 +25,19 @@ struct WorkspaceCycleTests { return state } + /// `list` is what is on disk; `running` is which of the *others* have an instance — + /// `nil` means all of them, the pre-#330 world where the two were never told apart. @MainActor private func store( - current: Workspace, list: [Workspace], opened: LockIsolated<[Workspace]> + current: Workspace, list: [Workspace], running: [Workspace]? = nil, + opened: LockIsolated<[Workspace]> ) -> TestStoreOf { + let others = running ?? list.filter { $0.id != current.id } let store = TestStore(initialState: state(current: current)) { AppFeature() } withDependencies: { $0.workspaceClient.list = { list } + $0.workspaceClient.otherOpen = { others } $0.workspaceClient.open = { workspace in opened.withValue { $0.append(workspace) } } } store.exhaustivity = .off @@ -108,4 +115,62 @@ struct WorkspaceCycleTests { #expect(opened.value == [work]) #expect(store.state.workspaces.known == [.default, work]) } + + // MARK: - Issue #330: only running workspaces are cycled + + @Test + @MainActor + func aWorkspaceWithNoWindowIsSteppedOver() async { + // `work` is on disk and next in creation order, but nobody has it open. ⌘` must land + // on `oss` — going to `work` would mean launching it, which is the bug. + let work = workspace("work") + let oss = workspace("oss") + let opened = LockIsolated<[Workspace]>([]) + let store = store( + current: .default, list: [.default, work, oss], running: [oss], opened: opened) + + await store.send(.workspaces(.cycleRequested(offset: 1))) + await store.receive(\.workspaces.switchRequested) + + #expect(opened.value == [oss]) + } + + @Test + @MainActor + func nothingElseRunningMeansNowhereToGo() async { + // The other workspaces exist, and every one of them was quit. ⌘` does nothing rather + // than resurrecting the first of them. + let work = workspace("work") + let oss = workspace("oss") + let opened = LockIsolated<[Workspace]>([]) + let store = store( + current: .default, list: [.default, work, oss], running: [], opened: opened) + + await store.send(.workspaces(.cycleRequested(offset: 1))) + await store.send(.workspaces(.cycleRequested(offset: -1))) + + #expect(opened.value.isEmpty) + } + + @Test + @MainActor + func wrappingCountsTheRunningOnesNotTheOnesOnDisk() async { + // Four on disk, two of them dead: from the last running one, ⌘` wraps to the first + // running one, and ⌘⇧` from there comes straight back. An index taken from the disk + // list with a modulus of the running count — or the other way round — lands on a dead + // workspace or off the end. + let work = workspace("work") + let oss = workspace("oss") + let home = workspace("home") + let opened = LockIsolated<[Workspace]>([]) + let store = store( + current: home, list: [.default, work, oss, home], running: [work], opened: opened) + + await store.send(.workspaces(.cycleRequested(offset: 1))) + await store.receive(\.workspaces.switchRequested) + await store.send(.workspaces(.cycleRequested(offset: -1))) + await store.receive(\.workspaces.switchRequested) + + #expect(opened.value == [work, work]) + } }