From 0edf280a3d1891f1cd902c4bbe2a7e792c9ae082 Mon Sep 17 00:00:00 2001 From: StuBehan Date: Fri, 18 Sep 2026 18:54:20 +0100 Subject: [PATCH] fix(panel): walk every control on the extensions browser --- .../ExtensionCatalogTests.swift | 77 ++++++-- .../FooterHintTests.swift | 76 ++++++++ panel/ExtensionsBrowser.swift | 177 ++++++++++++++---- panel/Panel.swift | 33 ++-- 4 files changed, 293 insertions(+), 70 deletions(-) diff --git a/Tests/StackNudgePanelCoreTests/ExtensionCatalogTests.swift b/Tests/StackNudgePanelCoreTests/ExtensionCatalogTests.swift index 1a11375..e24f610 100644 --- a/Tests/StackNudgePanelCoreTests/ExtensionCatalogTests.swift +++ b/Tests/StackNudgePanelCoreTests/ExtensionCatalogTests.swift @@ -159,18 +159,21 @@ final class ExtensionCatalogTests: XCTestCase { refused: [.init(id: "broken", reason: "nope")]) } - func testArrowsWalkTheRowsAndStopAtTheEnds() { + // The chevron is drawn above the list and is a target like anything else, so + // it is what ↓ reaches first from nothing and what ↑ stops against. + func testArrowsWalkEveryTargetAndStopAtTheEnds() { let c = catalog() let rows = threeRows XCTAssertEqual(rows.map(\.id), ["broken", "installed", "available"]) + XCTAssertEqual(c.targets(among: rows), + [.back, .row("broken"), .row("installed"), .row("available")]) c.moveSelection(among: rows, by: 1) - XCTAssertEqual(c.selectedID, "broken") - c.moveSelection(among: rows, by: 1) - c.moveSelection(among: rows, by: 1) - XCTAssertEqual(c.selectedID, "available") - c.moveSelection(among: rows, by: 1) - XCTAssertEqual(c.selectedID, "available", "stops rather than wrapping") + XCTAssertEqual(c.selection, .back) + c.moveSelection(among: rows, by: -1) + XCTAssertEqual(c.selection, .back, "stops rather than wrapping") + for _ in 0..<4 { c.moveSelection(among: rows, by: 1) } + XCTAssertEqual(c.selectedID, "available", "and stops at the far end too") } func testUpFromNoSelectionTakesTheLastRow() { @@ -179,6 +182,38 @@ final class ExtensionCatalogTests: XCTestCase { XCTAssertEqual(c.selectedID, "available") } + // Only while the fetch has failed, because that is the only time the button + // is drawn. Offering a ring to a button nobody can see is the same defect as + // drawing a button nothing can reach. + func testTryAgainIsATargetOnlyWhileTheFetchHasFailed() { + let loaded = catalog(fetch: { .success([]) }) + loaded.reload() + XCTAssertEqual(loaded.targets(among: threeRows).contains(.retry), false) + + let failed = catalog(fetch: { .failure(.downloadFailed("x")) }) + failed.reload() + XCTAssertEqual(failed.targets(among: threeRows)[1], .retry, + "drawn between the chevron and the rows, which is where it sits") + } + + // A failed fetch does not hide what is installed, so the page still has rows + // to seed onto; Try again is reached by walking up to it. + func testAFailedFetchStillSeedsOntoARow() { + let c = catalog(fetch: { .failure(.downloadFailed("x")) }) + c.reload() + c.reconcileSelection(among: threeRows) + XCTAssertEqual(c.selectedID, "broken") + } + + // With nothing installed and nothing published there is no row to land on, + // and Try again is the only thing on the page worth pressing. + func testAFailedFetchWithNoRowsSeedsOntoTryAgain() { + let c = catalog(fetch: { .failure(.downloadFailed("x")) }) + c.reload() + c.reconcileSelection(among: []) + XCTAssertEqual(c.selection, .retry) + } + // Enter does whatever the row's own button would: install an uninstalled // one, remove an installed one. func testActivatingAnUninstalledRowInstallsIt() { @@ -188,7 +223,7 @@ final class ExtensionCatalogTests: XCTestCase { let c = catalog(fetch: { .success([self.entry("available")]) }, install: { installed.append($0.id); return .success($0.id) }) c.reload() - c.selectedID = "available" + c.selection = .row("available") c.activateSelection(among: threeRows) XCTAssertEqual(installed, ["available"]) } @@ -201,7 +236,7 @@ final class ExtensionCatalogTests: XCTestCase { func testActivatingAnInstalledRowDoesNotRemoveIt() { var removed: [String] = [] let c = catalog(remove: { removed.append($0); return .success($0) }) - c.selectedID = "installed" + c.selection = .row("installed") c.activateSelection(among: threeRows) XCTAssertTrue(removed.isEmpty) XCTAssertNil(c.work["installed"]) @@ -232,7 +267,7 @@ final class ExtensionCatalogTests: XCTestCase { let c = catalog(fetch: { .success([self.entry("available")]) }, install: { _ in attempts += 1; return .failure(.installFailed("no")) }) c.reload() - c.selectedID = "available" + c.selection = .row("available") c.activateSelection(among: threeRows) XCTAssertEqual(attempts, 1) XCTAssertNotNil(c.failure(for: "available")) @@ -253,7 +288,7 @@ final class ExtensionCatalogTests: XCTestCase { // make Enter a no-op. func testTheSelectionMovesToTheFirstRowWhenItsOwnRowDisappears() { let c = catalog() - c.selectedID = "installed" + c.selection = .row("installed") c.reconcileSelection(among: threeRows) XCTAssertEqual(c.selectedID, "installed") @@ -261,10 +296,13 @@ final class ExtensionCatalogTests: XCTestCase { XCTAssertEqual(c.selectedID, "broken", "lands somewhere rather than nowhere") } - func testWithNoRowsAtAllThereIsNothingToSelect() { + // The row is gone, so the selection falls back to what the page still draws. + // Nothing is not an option: it leaves ⏎ advertised against no target. + func testWithNoRowsTheSelectionFallsBackToTheChevron() { let c = catalog() - c.selectedID = "installed" + c.selection = .row("installed") c.reconcileSelection(among: []) + XCTAssertEqual(c.selection, .back) XCTAssertNil(c.selectedID) } @@ -288,17 +326,20 @@ final class ExtensionCatalogTests: XCTestCase { XCTAssertEqual(seeded?.isInstalled, true) } - func testCommandArrowsJumpToTheFirstAndLastRow() { + func testCommandArrowsJumpToTheFirstAndLastTarget() { let c = catalog() c.selectEdge(among: threeRows, top: false) XCTAssertEqual(c.selectedID, "available") c.selectEdge(among: threeRows, top: true) - XCTAssertEqual(c.selectedID, "broken") + XCTAssertEqual(c.selection, .back) } - func testJumpingIsANoOpWithNoRows() { + // An empty catalogue still has its chevron, so ⌘↑↓ land somewhere rather + // than leaving the page with no ring at all. + func testJumpingWithNoRowsLandsOnTheChevron() { let c = catalog() c.selectEdge(among: [], top: true) + XCTAssertEqual(c.selection, .back) XCTAssertNil(c.selectedID) } @@ -569,14 +610,14 @@ final class ExtensionCatalogTests: XCTestCase { // written about, under a footer still advertising it. func testAQueryThatHidesTheSelectedRowMovesItToWhatIsLeft() { let c = catalog() - c.selectedID = "derby" + c.selection = .row("derby") c.reconcileSelection(among: ExtensionCatalog.matching(sample, query: "system")) XCTAssertEqual(c.selectedID, "system") } func testAQueryThatStillShowsTheSelectedRowKeepsIt() { let c = catalog() - c.selectedID = "derby" + c.selection = .row("derby") c.reconcileSelection(among: ExtensionCatalog.matching(sample, query: "derby")) XCTAssertEqual(c.selectedID, "derby") } diff --git a/Tests/StackNudgePanelCoreTests/FooterHintTests.swift b/Tests/StackNudgePanelCoreTests/FooterHintTests.swift index a9a0abc..db061cf 100644 --- a/Tests/StackNudgePanelCoreTests/FooterHintTests.swift +++ b/Tests/StackNudgePanelCoreTests/FooterHintTests.swift @@ -623,3 +623,79 @@ final class ExtensionConfigFooterTests: XCTestCase { } } } + +// The extensions browser's bar. Its chevron and its Try again are targets now, +// so ⏎ means different things depending on where the ring is, and the bar has to +// say which. One hint per action, with ⏎ added to the selected one. +final class ExtensionsBrowserFooterTests: XCTestCase { + + private func hints(selection: ExtensionCatalog.Target?, + activation: String = "Install", + updateSelected: Bool = false, + queryIsEmpty: Bool = true, + hasRows: Bool = true) -> [FooterHintSpec] { + ExtensionsView.footerHints(selection: selection, activation: activation, + updateSelected: updateSelected, + queryIsEmpty: queryIsEmpty, hasRows: hasRows) + } + + private func labels(_ specs: [FooterHintSpec]) -> [String] { specs.map(\.label) } + + private func keys(_ specs: [FooterHintSpec], _ label: String) -> [String]? { + specs.first { $0.label == label }?.keys + } + + func test_onARow_theRowVerbTakesReturn() { + let specs = hints(selection: .row("derby"), activation: "Settings") + XCTAssertEqual(labels(specs), ["Back", "Search", "Select", "Settings", "Reload"]) + XCTAssertEqual(keys(specs, "Settings"), ["⏎"]) + XCTAssertEqual(keys(specs, "Reload"), ["⌘R"]) + } + + // The row verb names something the chevron will not do, so it goes rather + // than sitting there wrong. + func test_onTheChevron_returnRidesOnBackAndTheRowVerbGoes() { + let specs = hints(selection: .back, activation: "Install") + XCTAssertEqual(labels(specs), ["Back", "Search", "Select", "Reload"]) + XCTAssertEqual(keys(specs, "Back"), ["⏎", "Esc"]) + } + + // Try again and Reload are one action, so the button being selected adds ⏎ + // to the hint that is already there rather than printing a second one. + func test_onTryAgain_returnRidesOnReload() { + let specs = hints(selection: .retry) + XCTAssertEqual(labels(specs), ["Back", "Search", "Select", "Reload"]) + XCTAssertEqual(keys(specs, "Reload"), ["⏎", "⌘R"]) + } + + // With a query typed, Esc clears it rather than leaving, so ⏎ on the chevron + // must not be advertised on a hint that now means something else. + func test_withAQueryTypedEscapeClearsAndKeepsReturnOffIt() { + let specs = hints(selection: .back, queryIsEmpty: false) + XCTAssertEqual(keys(specs, "Clear"), ["Esc"]) + XCTAssertFalse(labels(specs).contains("Back")) + } + + func test_noLabelIsAdvertisedTwice() { + for selection: ExtensionCatalog.Target? in [nil, .back, .retry, .row("derby")] { + let names = labels(hints(selection: selection)) + XCTAssertEqual(Set(names).count, names.count, "duplicate in \(names)") + } + } + + // An update pending takes ⏎ for the update, so the row's page needs ⌘⏎ or + // there is no keyboard route to it at all. + func test_anUpdatePendingAdvertisesTheRouteToTheRowsPage() { + let specs = hints(selection: .row("derby"), activation: "Update", updateSelected: true) + XCTAssertEqual(keys(specs, "Settings"), ["⌘⏎"]) + } + + // Nothing to walk, so the hint dims rather than disappearing: the bar must + // not reflow as a query filters the list down to nothing. + func test_selectDimsWhenThereIsNothingToWalk() { + XCTAssertEqual(hints(selection: .back, hasRows: false) + .first { $0.label == "Select" }?.dimmed, true) + XCTAssertEqual(hints(selection: .row("derby"), hasRows: true) + .first { $0.label == "Select" }?.dimmed, false) + } +} diff --git a/panel/ExtensionsBrowser.swift b/panel/ExtensionsBrowser.swift index c280f4b..df881b9 100644 --- a/panel/ExtensionsBrowser.swift +++ b/panel/ExtensionsBrowser.swift @@ -26,9 +26,33 @@ final class ExtensionCatalog: ObservableObject { @Published private(set) var load: Load = .idle @Published private(set) var entries: [ExtensionInstaller.IndexEntry] = [] @Published private(set) var work: [String: Work] = [:] + // Everything on the page the keyboard can land on, in the order it is drawn. + // Not just the rows: the page has a back chevron above them and, when the + // catalogue fetch failed, a Try again beside the reason. Both had a key + // already (Esc and ⌘R) and neither had a ring, so the page looked half + // wired next to an extension's own, which walks its buttons. + // + // The search field is deliberately not a target. Every other control here + // needs ⏎ to reach it; the field is reached by typing, which is what the + // page does with any printable key, and a ring you step onto to start + // typing would be a second way to do the thing that already needs none. + enum Target: Equatable { + case back + case retry + case row(String) + } + // Keyboard selection. The panel is keyboard-native and this page's own // footer advertises key hints, but every action on it was mouse-only. - @Published var selectedID: String? + @Published var selection: Target? + + // The row the selection is on, if it is on one. Most of the page cares only + // about this, and a card highlighting itself should not have to know the + // chevron exists. + var selectedID: String? { + if case .row(let id) = selection { return id } + return nil + } // What the search field holds. Filtering happens at render time rather than // over `entries`, so a query never hides an *installed* extension from the // merge that produces the rows — it only hides it from this list. @@ -150,14 +174,22 @@ final class ExtensionCatalog: ObservableObject { // MARK: - Keyboard + // Drawing order, which is also the order ↑↓ walk. Try again exists only + // while the fetch has failed, because that is the only time the button is + // drawn; back is always there, so the list is never empty even on a + // catalogue with nothing in it. + func targets(among rows: [ExtensionRow]) -> [Target] { + [.back] + (load.isFailure ? [.retry] : []) + rows.map { Target.row($0.id) } + } + func moveSelection(among rows: [ExtensionRow], by delta: Int) { - guard !rows.isEmpty else { return } - let current = rows.firstIndex { $0.id == selectedID } + let all = targets(among: rows) + let current = all.firstIndex { $0 == selection } // No selection yet: ↓ takes the first and ↑ the last, so either arrow // is a way in. Same rule as the extension tab's row list. - let next = current.map { min(max($0 + delta, 0), rows.count - 1) } - ?? (delta > 0 ? 0 : rows.count - 1) - selectedID = rows[next].id + let next = current.map { min(max($0 + delta, 0), all.count - 1) } + ?? (delta > 0 ? 0 : all.count - 1) + selection = all[next] } // What Enter does to the selected row: install it, or update it, or dismiss @@ -178,8 +210,8 @@ final class ExtensionCatalog: ObservableObject { // ⌘↑↓, which every other list page in the panel answers. func selectEdge(among rows: [ExtensionRow], top: Bool) { - guard !rows.isEmpty else { return } - selectedID = top ? rows.first?.id : rows.last?.id + let all = targets(among: rows) + selection = top ? all.first : all.last } // Keeps the selection on a row that still exists after a reload, a removal @@ -193,8 +225,16 @@ final class ExtensionCatalog: ObservableObject { // first, then what is installed, and only then what is merely published. On // any machine with an extension on it ⏎ lands on a page, not an install. func reconcileSelection(among rows: [ExtensionRow]) { - if let selectedID, rows.contains(where: { $0.id == selectedID }) { return } - selectedID = rows.first?.id + let all = targets(among: rows) + if let selection, all.contains(selection) { return } + // A row first where there is one, so ⏎ on arrival acts on the list + // rather than walking straight back out of the page. Then Try again, + // which is what somebody reading "couldn't download" wants ⏎ to do. + // The chevron last, for an empty catalogue that loaded fine: there is + // genuinely nothing else on the page. + selection = all.first(where: { if case .row = $0 { return true } else { return false } }) + ?? all.first(where: { $0 == .retry }) + ?? all.first } // Pure, so the matching rule is testable without a view. @@ -373,37 +413,22 @@ struct ExtensionsView: View { // so about two of them fit at the panel's 260pt minimum, and // without this ↑↓ moved a highlight straight off the bottom of // a catalogue of any size, which is the whole page. - .onChange(of: catalog.selectedID) { id in - guard let id else { return } + .onChange(of: catalog.selection) { target in + guard let anchor = Self.anchor(for: target) else { return } withAnimation(.easeOut(duration: 0.15)) { - proxy.scrollTo(id, anchor: nil) + proxy.scrollTo(anchor, anchor: nil) } } } PageFooter { - // Named for what Esc does from here, which depends on whether - // there is a query to clear first. - FooterHint(label: catalog.query.isEmpty ? "Back" : "Clear", keys: ["Esc"]) - FooterHint(label: "Search", keys: ["/"]) - // Dimmed rather than dropped when a query, or an empty - // catalogue, leaves nothing to walk: the bar must not reflow as - // the list filters, and an advertised key that does nothing is - // the thing this page kept doing. Same treatment the Settings - // footer gives its Cycle hint. - FooterHint(label: "Select", keys: ["↑↓", "⌘↑↓"]) - .opacity(rows.isEmpty ? 0.35 : 1) - FooterHint(label: activationLabel(in: rows), keys: ["⏎"]) - .opacity(rows.isEmpty ? 0.35 : 1) - // Only where Enter is busy doing something else. An installed - // row with an update pending takes Enter for the update, so - // without this there would be no keyboard route to its page. - if selectedRow(in: rows)?.updateAvailable == true { - FooterHint(label: "Settings", keys: ["⌘⏎"]) - } - // ⌘R rather than R: a plain letter seeds the search field, the - // same trade the history pane makes. - FooterHint(label: "Reload", keys: ["⌘R"]) + let hints = Self.footerHints( + selection: catalog.selection, + activation: activationLabel(in: rows), + updateSelected: selectedRow(in: rows)?.updateAvailable == true, + queryIsEmpty: catalog.query.isEmpty, + hasRows: !rows.isEmpty) + ForEach(hints.indices, id: \.self) { FooterHintRow(spec: hints[$0]) } } } .frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .topLeading) @@ -441,6 +466,68 @@ struct ExtensionsView: View { } } + // The bar as data, so it can be asserted rather than read. + // + // One hint per action, with ⏎ added to whichever the ring is on. A + // separate hint naming the selected target prints the bar's own labels + // twice the moment the selection reaches the chevron, which is what the + // extension config page ran into. + static func footerHints(selection: ExtensionCatalog.Target?, + activation: String, + updateSelected: Bool, + queryIsEmpty: Bool, + hasRows: Bool) -> [FooterHintSpec] { + var hints: [FooterHintSpec] = [] + // Named for what Esc does from here, which depends on whether there is a + // query to clear first. ⏎ joins it only when the chevron is selected + // *and* Esc would leave rather than clear, or the one key would be + // advertised for two different things. + let backSelected = selection == .back + let escapeLeaves = queryIsEmpty + hints.append(FooterHintSpec( + label: escapeLeaves ? "Back" : "Clear", + keys: backSelected && escapeLeaves ? ["⏎", "Esc"] : ["Esc"], + primary: backSelected && escapeLeaves)) + hints.append(FooterHintSpec(label: "Search", keys: ["/"])) + // Dimmed rather than dropped when a query, or an empty catalogue, leaves + // nothing to walk: the bar must not reflow as the list filters, and an + // advertised key that does nothing is the thing this page kept doing. + // Same treatment the Settings footer gives its Cycle hint. + hints.append(FooterHintSpec(label: "Select", keys: ["↑↓", "⌘↑↓"], + dimmed: !hasRows)) + // Only while the ring is on a row. On the chevron or Try again, ⏎ + // belongs to those, and the row verb would name something it will not do. + if case .row = selection { + hints.append(FooterHintSpec(label: activation, keys: ["⏎"], primary: true)) + // Only where Enter is busy doing something else. An installed row + // with an update pending takes Enter for the update, so without this + // there would be no keyboard route to its page. + if updateSelected { + hints.append(FooterHintSpec(label: "Settings", keys: ["⌘⏎"])) + } + } + // ⌘R rather than R: a plain letter seeds the search field, the same + // trade the history pane makes. Try again and Reload are one action, so + // selecting the button adds ⏎ here rather than printing a second hint. + let retrySelected = selection == .retry + hints.append(FooterHintSpec(label: "Reload", + keys: retrySelected ? ["⏎", "⌘R"] : ["⌘R"], + primary: retrySelected)) + return hints + } + + // The back chevron sits above the scroller and needs no anchor; scrolling to + // the top of the list is what brings it into view. + static let retryAnchor = "extensions-retry" + + static func anchor(for target: ExtensionCatalog.Target?) -> String? { + switch target { + case .row(let id): return id + case .retry: return retryAnchor + case .back, nil: return nil + } + } + private func selectedRow(in rows: [ExtensionRow]) -> ExtensionRow? { guard let id = catalog.selectedID else { return nil } return rows.first { $0.id == id } @@ -497,13 +584,20 @@ struct ExtensionsView: View { } private var header: some View { - HStack(spacing: 8) { + let selected = catalog.selection == .back + return HStack(spacing: 8) { Button(action: onBack) { HStack(spacing: 4) { Image(systemName: "chevron.left").font(.caption.weight(.semibold)) Text("Settings").font(.caption) } - .foregroundStyle(.secondary) + .foregroundStyle(selected ? Color.primary : .secondary) + .padding(.horizontal, 6) + .padding(.vertical, 3) + .background(RoundedRectangle(cornerRadius: 6, style: .continuous) + .fill(Color.accentColor.opacity(selected ? 0.18 : 0))) + .overlay(RoundedRectangle(cornerRadius: 6, style: .continuous) + .strokeBorder(Color.accentColor.opacity(selected ? 0.6 : 0), lineWidth: 1.5)) } .buttonStyle(.plain) @@ -529,7 +623,9 @@ struct ExtensionsView: View { Text(why).font(.caption).foregroundStyle(.orange) .fixedSize(horizontal: false, vertical: true) Spacer(minLength: 4) - cardButton("Try again", prominent: true) { catalog.reload() } + cardButton("Try again", prominent: true, + selected: catalog.selection == .retry) { catalog.reload() } + .id(Self.retryAnchor) } ForEach(rows) { row in extensionRow(row) } } @@ -624,7 +720,7 @@ struct ExtensionsView: View { .strokeBorder(Color.accentColor.opacity(catalog.selectedID == row.id ? 0.6 : 0), lineWidth: 1.5)) .contentShape(Rectangle()) - .onTapGesture { catalog.selectedID = row.id } + .onTapGesture { catalog.selection = .row(row.id) } // The scroll anchor, keyed by what the selection is keyed by. .id(row.id) .accessibilityElement(children: .combine) @@ -671,8 +767,9 @@ struct ExtensionsView: View { } private func cardButton(_ title: String, prominent: Bool = false, + selected: Bool = false, action: @escaping () -> Void) -> some View { - CardButton(title: title, prominent: prominent, action: action) + CardButton(title: title, prominent: prominent, selected: selected, action: action) } } diff --git a/panel/Panel.swift b/panel/Panel.swift index fbd73f3..127a479 100644 --- a/panel/Panel.swift +++ b/panel/Panel.swift @@ -4046,18 +4046,27 @@ final class PanelController: NSObject, NSApplicationDelegate, PanelKeyDelegate, case .moveSelection(let delta): extensionCatalog.moveSelection(among: visibleRows(), by: delta) case .activate: - // An installed row with nothing to update has no install action - // left, and Enter used to sit there doing nothing while the - // footer advertised it. Opening its page is what the card's own - // button does, so Enter now agrees with the button. - let rows = visibleRows() - if let id = extensionCatalog.selectedID, - let row = rows.first(where: { $0.id == id }), - row.isInstalled, !row.updateAvailable, - extensionCatalog.failure(for: id) == nil { - configureExtension(row, from: .extensions) - } else { - extensionCatalog.activateSelection(among: rows) + switch extensionCatalog.selection { + case .back: + closeExtensionsBrowser() + // The same action the ⌘R hint names; the button is the mouse's + // way to it and this is the keyboard's. + case .retry: + extensionCatalog.reload() + case .row, nil: + // An installed row with nothing to update has no install + // action left, and Enter used to sit there doing nothing + // while the footer advertised it. Opening its page is what + // the card's own button does, so Enter now agrees with it. + let rows = visibleRows() + if let id = extensionCatalog.selectedID, + let row = rows.first(where: { $0.id == id }), + row.isInstalled, !row.updateAvailable, + extensionCatalog.failure(for: id) == nil { + configureExtension(row, from: .extensions) + } else { + extensionCatalog.activateSelection(among: rows) + } } case .swallow: break }