Repository navigation
Music Playback - #16
Conversation
…tion so the contentview can reference it. Was driving me crazy
… so one song can fit multiple contexts
…dioPlayer to be mutable. Initial playback & song info display works in my hacky tests.
…se. Clarified method descriptions.
…reference to ContentView
…ill handle music across views & for less clutter.
|
Progress bar works, going to leave the timestamp out for now until I figure out the UI redesign. |
There was a problem hiding this comment.
Pull request overview
Implements core in-app music playback with a shared audio manager, exposing play/pause, previous/next, shuffle, queue loading from the app bundle, and a basic UI for selecting songs.
Changes:
- Added
AudioPlayerManagersingleton to load songs from a bundledMusic/folder and manage playback/queueing/progress. - Updated
ContentViewwith playback controls, a toggleable song list, and queue reload hooks. - Expanded
Songmodel to supportsongName,artist, and multiplelocations; addedMusicfolder as a project resource.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| LittleRoute/Primary Views/ContentView.swift | Wires the new audio manager into the UI (song list + playback controls) and triggers initial queue setup. |
| LittleRoute/Data Handling/Song.swift | Updates the Song SwiftData model to include filename/artist and multiple locations. |
| LittleRoute/Data Handling/AudioPlayerManager.swift | Adds the playback/queue manager (AVAudioPlayer + bundle loading + progress timer). |
| LittleRoute.xcodeproj/project.pbxproj | Adds LittleRoute/Music folder to app resources so mp3s ship in the bundle. |
| .vscode/settings.json | Adds SweetPad workspace configuration for VS Code builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public func skip() { | ||
| guard !songQueue.isEmpty else { | ||
| print("No songs in queue") | ||
| return | ||
| } | ||
|
|
||
| // Set the current index to the next song in the queue | ||
| // if on the last song, loop back to the start of the queue | ||
| currentIndex = (currentIndex + 1) % songQueue.count | ||
| currentSong = songQueue[currentIndex] | ||
| let nextSong = songQueue[currentIndex].songName | ||
|
|
||
| loadAudio(fileName: nextSong) | ||
| audioPlayer?.play() | ||
| isPaused = false | ||
| startPlaybackTimer() | ||
| } |
There was a problem hiding this comment.
If loadAudio fails (file missing), skip()/previous() will still update currentSong/currentIndex and then audioPlayer?.play() can continue playing the previous audio (or leave the player in an inconsistent state). Make loadAudio return a success flag (or throw) and only update currentSong/start playback when loading succeeds; alternatively, set audioPlayer = nil/stop playback when the guard fails.
| // Prepare the audio player with the selected song | ||
| private func loadAudio(fileName: String) { | ||
|
|
||
| guard let path = Bundle.main.path(forResource: fileName, ofType: "mp3", inDirectory: "Music") else { | ||
| print("Could not find file: \(fileName).mp3") | ||
| return | ||
| } |
There was a problem hiding this comment.
If loadAudio fails (file missing), skip()/previous() will still update currentSong/currentIndex and then audioPlayer?.play() can continue playing the previous audio (or leave the player in an inconsistent state). Make loadAudio return a success flag (or throw) and only update currentSong/start playback when loading succeeds; alternatively, set audioPlayer = nil/stop playback when the guard fails.
| if(isShuffled) { | ||
| songQueue.shuffle() | ||
| } else { | ||
| reloadQueue(newContext: currentContext, shuffle: isShuffled, songs: songQueue) |
There was a problem hiding this comment.
Turning shuffle off does not restore the original (unshuffled) order: you call reloadQueue(..., songs: songQueue) using the already shuffled queue as the source. To make shuffle reversible, maintain an unshuffled source list (e.g., allSongsForContext) and build songQueue from that when isShuffled == false.
| if(isShuffled) { | |
| songQueue.shuffle() | |
| } else { | |
| reloadQueue(newContext: currentContext, shuffle: isShuffled, songs: songQueue) | |
| if isShuffled { | |
| // Shuffle the existing queue in place | |
| songQueue.shuffle() | |
| } else { | |
| // Rebuild the queue from the original source for the current context, | |
| // instead of using the already shuffled queue as the base ordering. | |
| reloadQueue(newContext: currentContext, shuffle: false, songs: []) |
| // shuffle if user has the option toggled | ||
| if isShuffled { |
There was a problem hiding this comment.
The shuffle parameter is ignored (reloadQueue uses isShuffled instead). This can produce unexpected behavior for callers that pass shuffle: expecting it to be honored. Either remove the parameter entirely (and rely on isShuffled), or use the parameter (if shuffle { ... }) consistently.
| // shuffle if user has the option toggled | |
| if isShuffled { | |
| // shuffle if requested by caller | |
| if shuffle { |
| func audioPlayerDidFinishPlaying(_ player: AVAudioPlayer, successfully flag: Bool) { | ||
| if flag { | ||
| print("Song finished, playing next song") | ||
| skip() | ||
| } | ||
| } |
There was a problem hiding this comment.
AVAudioPlayerDelegate callbacks are not guaranteed to occur on the main thread. skip() mutates multiple @Published properties and may create/prepare an AVAudioPlayer; doing this off-main can cause UI update warnings/crashes. Dispatch the state mutations to the main queue (or annotate the class/functions with @MainActor and ensure calls cross actors correctly).
| var songName: String /* { | ||
| // This is the actual file name of the song, without the extension | ||
| return title.replacingOccurrences(of: ".mp3", with: "") | ||
| } */ |
There was a problem hiding this comment.
Two issues here: (1) the commented-out computed property embedded in the songName declaration is confusing and reads like partial code; move this into a normal comment or implement a separate computed property if needed. (2) The deinit comment is inaccurate: deinit runs when the object is deallocated, not specifically/only when a user deletes a song, and SwiftData deletion does not guarantee immediate deallocation. Either remove deinit or update the commentary to reflect actual behavior.
| // remove class from memory | ||
| deinit { | ||
| // only run this when the user deletes a song | ||
| } |
There was a problem hiding this comment.
Two issues here: (1) the commented-out computed property embedded in the songName declaration is confusing and reads like partial code; move this into a normal comment or implement a separate computed property if needed. (2) The deinit comment is inaccurate: deinit runs when the object is deallocated, not specifically/only when a user deletes a song, and SwiftData deletion does not guarantee immediate deallocation. Either remove deinit or update the commentary to reflect actual behavior.
| if showSongList { | ||
| ScrollView { | ||
| LazyVStack(alignment: .leading, spacing: 8) { | ||
| ForEach(songs, id: \.songName) { song in |
There was a problem hiding this comment.
Using songName as the ForEach identifier will break UI diffing (and can crash) if there are duplicates (e.g., two records with the same filename base). Prefer a stable unique identifier from SwiftData (e.g., song.persistentModelID) rather than a human/file-derived string.
| ForEach(songs, id: \.songName) { song in | |
| ForEach(songs, id: \.persistentModelID) { song in |
| else if audioPlayer != nil && !audioPlayer!.isPlaying { | ||
| audioPlayer!.play() | ||
| paused = !paused | ||
| .onChange(of: songs) { oldValue, newValue in |
There was a problem hiding this comment.
Reloading the entire queue on every songs change (including the initial insert from loadSongsFromBundle) will reset currentIndex to 0 and call loadAudio, which can interrupt/replace an in-progress playback session. Consider gating this (e.g., only reload when the content meaningfully changes and the user is not playing, or preserve the currently playing song/index when rebuilding the queue).
| .onChange(of: songs) { oldValue, newValue in | |
| .onChange(of: songs) { oldValue, newValue in | |
| // Avoid interrupting an in-progress playback session by reloading the entire queue | |
| guard !audioManager.isPlaying else { | |
| return | |
| } |
| @Query private var songs: [Song] // Query all songs from the database | ||
| @State private var showSongList = false | ||
|
|
||
| let sampleSong: Song = Song(title: "Sample Song", songName: "RSEmart", artist: "Sample Artist", locations: ["all"], populationMin: 0, populationMax: 10000) |
There was a problem hiding this comment.
This sample Song is not used within ContentView (and locations: [\"all\"] also won’t match your context filter which uses Context.rawValue like \"All\"). Remove this constant if it’s leftover, or update it and wire it into previews/tests where it’s actually exercised.
| let sampleSong: Song = Song(title: "Sample Song", songName: "RSEmart", artist: "Sample Artist", locations: ["all"], populationMin: 0, populationMax: 10000) |
Music playback feature. Basic playback and queueing up next song for seamless playback when skipping. Added working play/pause, skip/rewind, and shuffle buttons. Progress bar now working.
Draft as of 3/11/2026
Requirements: