Replace system playback menus with Tuner panels#69
Conversation
zachyzissou
force-pushed
the
codex/tuner-inline-action-panels
branch
from
0e8b35b to
f24c055
Compare
There was a problem hiding this comment.
Pull request overview
This PR replaces several system Menu-based playback controls with inline Tuner-styled panels/pickers, aligning playback-related UI with the app’s “instrument cluster” design language.
Changes:
- Replace Settings “Default playback rate” menu with an expandable inline rate picker.
- Replace Player speed and sleep timer menus with inline option grids/panels.
- Replace the episode download menu with a single primary action button based on download state.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| OffScript/SettingsView.swift | Adds an expandable inline default playback-rate picker UI. |
| OffScript/PlayerView.swift | Converts speed + sleep timer menus into inline Tuner pickers and adds expansion state. |
| OffScript/DownloadButton.swift | Replaces the download action menu with a state-driven single-tap primary action. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private func performPrimaryDownloadAction() { | ||
| switch episode.downloadState { | ||
| case .notDownloaded, .failed: | ||
| downloadService.startDownload(for: episode) | ||
| case .downloading, .queued: | ||
| downloadService.cancelDownload(for: episode) | ||
| case .downloaded: | ||
| downloadService.deleteDownload(for: episode) | ||
| } |
There was a problem hiding this comment.
When downloadState is .downloaded, a single tap now immediately deletes the local file (previously exposed as a destructive menu action). Consider adding a confirmation step (e.g. confirmationDialog) or requiring an explicit secondary action for delete to avoid accidental data removal.
| Button { | ||
| withAnimation(.easeInOut(duration: 0.16)) { | ||
| isDefaultRatePickerExpanded.toggle() | ||
| } | ||
| } label: { | ||
| HStack(spacing: 6) { | ||
| TunerLabel( | ||
| text: String(format: "%.2g×", currentDefaultRate), | ||
| color: .offscriptSignalYellow, | ||
| size: 11 | ||
| ) | ||
| Image(systemName: isDefaultRatePickerExpanded ? "chevron.up" : "chevron.down") | ||
| .font(.system(size: 10, weight: .bold)) | ||
| .foregroundStyle(Color.offscriptSignalYellow) |
There was a problem hiding this comment.
The playback-rate toggle button relies on its visual label (current rate + chevron) but has no explicit accessibility label/value/hint. For VoiceOver this will be read without context; consider adding an accessibilityLabel like “Default playback rate”, an accessibilityValue with the current rate, and a hint indicating it expands/collapses the picker.
| private func tunerRatePicker( | ||
| rates: [Float], | ||
| selectedRate: Float, | ||
| onSelect: @escaping (Float) -> Void | ||
| ) -> some View { |
There was a problem hiding this comment.
tunerRatePicker is now implemented separately in both SettingsView and PlayerView with very similar layout/styling. This duplication makes future tweaks (sizes, colors, accessibility) easy to drift; consider extracting a shared TunerRatePicker view/helper (e.g., in a UI components file) and reusing it in both places.
| .background(isSelected ? Color.offscriptSignalYellow : Color.clear) | ||
| .overlay(Rectangle().stroke(isSelected ? Color.offscriptSignalYellow : Color.offscriptHairline, lineWidth: 1)) | ||
| } | ||
| .buttonStyle(.plain) |
There was a problem hiding this comment.
The new inline speed buttons don’t provide per-option accessibility labels/selected traits. SettingsView’s rate picker adds an explicit accessibilityLabel per rate, but PlayerView’s picker does not; adding labels (e.g. “Set playback speed to …”) and .isSelected traits for the current rate would make this control much clearer to VoiceOver users.
| .buttonStyle(.plain) | |
| .buttonStyle(.plain) | |
| .accessibilityElement(children: .ignore) | |
| .accessibilityLabel("Set playback speed to \(String(format: "%.2g", rate))x") | |
| .accessibilityAddTraits(isSelected ? .isSelected : []) | |
| .accessibilityRemoveTraits(isSelected ? [] : .isSelected) |
| @@ -40,6 +27,17 @@ struct DownloadButton: View { | |||
| .accessibilityLabel(downloadAccessibilityLabel) | |||
| } | |||
There was a problem hiding this comment.
This button now performs different actions depending on download state (start/cancel/delete), but the accessibility label reads as a status in several cases (e.g. “is downloaded”, “is queued for download”). Add an accessibilityHint/value that describes what activating the control will do in each state (e.g. “Double-tap to remove download” when downloaded, “Double-tap to cancel” when downloading) so the action is not surprising for assistive-tech users.
|
Addressed Copilot review feedback in 10916dc:\n\n- Added shared TunerRatePicker and reused it from Player + Settings.\n- Added explicit accessibility labels/values/hints and selected traits for the inline rate options.\n- Changed downloaded episodes to require a second tap to remove the local file, using an armed REMOVE? state instead of restoring system confirmation chrome.\n\nVerification after the fixes:\n- git diff --check\n- xcodebuild build -project OffScript.xcodeproj -scheme OffScript -destination 'platform=iOS Simulator,OS=latest,name=iPhone 17 Pro'\n- xcodebuild test -project OffScript.xcodeproj -scheme OffScript -destination 'platform=iOS Simulator,OS=latest,name=iPhone 17 Pro' -only-testing:OffScriptTests |
Summary
Verification
Notes