refactor: break up and rename play queue components for clearer separation #676
Conversation
huynhsontung
changed the title
huynhsontung
changed the title
Replaced `IMediaParsingService` with `IPlaylistFactory` and `PlaylistFactory` to centralize playlist creation and parsing logic. Refactored the `Playlist` class to support cloning properties from a reference playlist and added constructors for enhanced flexibility. Introduced `PlaylistCreateResult` to encapsulate playlist creation results. Updated `PlaybackControlService`, `MediaListViewModel`, and `PlaylistService` to use `IPlaylistFactory` for playlist operations. Removed `IMediaParsingService` and its implementation. Adjusted dependency injection to register the new factory. Enabled nullable reference types in several files and performed general code cleanup. Updated `Screenbox.Core.csproj` to reflect file additions and removals.
Renamed IPlaylistFactory to IMediaListFactory and updated its methods to return NextMediaList instead of Playlist. Replaced PlaylistFactory with MediaListFactory to align with the new interface.
dosubot
bot
added
the
size:XXL
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the media playlist components to improve code organization and maintainability by separating concerns and applying proper MVVM architecture patterns.
- Renames components from "Playlist" to "PlayQueue" for clearer UI terminology
- Extracts business logic from
MediaListViewModelinto dedicated stateless services (IPlaylistService,IPlaybackControlService,IMediaListFactory) - Introduces domain models (
Playlist,PlaybackNavigationResult,NextMediaList) to separate data from presentation logic
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| Screenbox/Screenbox.csproj | Updates project references from PlaylistView to PlayQueueControl |
| Screenbox/Pages/PlayerPage.xaml.cs | Updates references to use renamed PlayQueue control |
| Screenbox/Pages/PlayerPage.xaml | Updates XAML to use PlayQueueControl instead of PlaylistView |
| Screenbox/Pages/PlayQueuePage.xaml.cs | Removes unused import and updates control references |
| Screenbox/Pages/PlayQueuePage.xaml | Updates binding references to use PlayQueue control |
| Screenbox/Controls/PlayQueueControl.xaml.cs | Renames class from PlaylistView to PlayQueueControl and updates ViewModel type |
| Screenbox/Controls/PlayQueueControl.xaml | Updates class reference and event handler names |
| Screenbox.Core/ViewModels/PlayQueueViewModel.cs | Renames class from PlaylistViewModel to PlayQueueViewModel |
| Screenbox.Core/ViewModels/MediaListViewModel.cs | Major refactor to use injected services and domain models instead of inline business logic |
| Screenbox.Core/Services/PlaylistService.cs | New stateless service for playlist operations like shuffling and neighboring file management |
| Screenbox.Core/Services/PlaybackControlService.cs | New service handling playback navigation logic |
| Screenbox.Core/Services/IPlaylistService.cs | Interface for playlist operations |
| Screenbox.Core/Services/IPlaybackControlService.cs | Interface for playback control operations |
| Screenbox.Core/Screenbox.Core.csproj | Adds new service and model files to project |
| Screenbox.Core/Models/ShuffleBackup.cs | Extracted model for shuffle backup data |
| Screenbox.Core/Models/Playlist.cs | New domain model representing playlist data |
| Screenbox.Core/Models/PlaybackNavigationResult.cs | Model for playback navigation results |
| Screenbox.Core/Models/NextMediaList.cs | Model for media list parsing results |
| Screenbox.Core/Helpers/CollectionExtensions.cs | Minor import reordering |
| Screenbox.Core/Factories/MediaListFactory.cs | Extracted factory for media list creation and parsing |
| Screenbox.Core/Factories/IMediaListFactory.cs | Interface for media list factory |
| Screenbox.Core/Common/ServiceHelpers.cs | Updates service registration for renamed and new services |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (_delayPlay is MediaViewModel media && Items.Contains(media)) | ||
| { | ||
| PlaySingle(media); | ||
| await TryEnqueueAndPlayPlaylistAsync(media); | ||
| } | ||
| else | ||
| { | ||
| ClearPlaylist(); | ||
| await EnqueueAndPlay(_delayPlay); | ||
| } | ||
| _delayPlay = null; |
There was a problem hiding this comment.
Async lambda in TryEnqueue can cause unhandled exceptions. The async operations inside the lambda should be wrapped in try-catch blocks or the lambda should be fire-and-forget with proper error handling.
| if (_delayPlay is MediaViewModel media && Items.Contains(media)) | |
| { | |
| PlaySingle(media); | |
| await TryEnqueueAndPlayPlaylistAsync(media); | |
| } | |
| else | |
| { | |
| ClearPlaylist(); | |
| await EnqueueAndPlay(_delayPlay); | |
| } | |
| _delayPlay = null; | |
| try | |
| { | |
| if (_delayPlay is MediaViewModel media && Items.Contains(media)) | |
| { | |
| PlaySingle(media); | |
| await TryEnqueueAndPlayPlaylistAsync(media); | |
| } | |
| else | |
| { | |
| ClearPlaylist(); | |
| await EnqueueAndPlay(_delayPlay); | |
| } | |
| _delayPlay = null; | |
| } | |
| catch (Exception ex) | |
| { | |
| SentrySdk.CaptureException(ex); | |
| } |
Copilot uses AI. Check for mistakes.
| switch (args.Button) | ||
| { | ||
| case SystemMediaTransportControlsButton.Next: | ||
| await NextAsync(); | ||
| break; | ||
| case SystemMediaTransportControlsButton.Previous: | ||
| await PreviousAsync(); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Async lambda in TryEnqueue lacks error handling. Exceptions from NextAsync() or PreviousAsync() could become unhandled exceptions.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| _mediaPlayer.PlaybackItem = vm.Item.Value; | ||
| _mediaPlayer.Play(); |
There was a problem hiding this comment.
Potential null reference exception if _mediaPlayer is null. The null check at line 308 guards against this, but the assignment should be inside that check or use null-conditional operator.
| _mediaPlayer.PlaybackItem = vm.Item.Value; | |
| _mediaPlayer.Play(); | |
| _mediaPlayer!.PlaybackItem = vm.Item.Value; | |
| _mediaPlayer?.Play(); |
Copilot uses AI. Check for mistakes.
|
|
||
| public Playlist RestoreFromShuffle(Playlist playlist) | ||
| { | ||
| Guard.IsNotNull(playlist.ShuffleBackup, nameof(playlist.ShuffleBackup)); |
There was a problem hiding this comment.
Using Guard.IsNotNull with nameof(playlist.ShuffleBackup) will throw an exception with the property name rather than the parameter name. Consider using a more descriptive message or checking the condition explicitly and throwing a more meaningful exception.
| Guard.IsNotNull(playlist.ShuffleBackup, nameof(playlist.ShuffleBackup)); | |
| if (playlist.ShuffleBackup is null) | |
| throw new InvalidOperationException("Cannot restore from shuffle: ShuffleBackup is null. Ensure the playlist was previously shuffled."); |
Copilot uses AI. Check for mistakes.
| /// <summary> | ||
| /// Backup data for shuffle functionality | ||
| /// </summary> | ||
| internal sealed class ShuffleBackup |
There was a problem hiding this comment.
The ShuffleBackup class is marked as internal but is used in public properties of the Playlist class. This creates an inconsistent API surface where public members expose internal types. Consider making ShuffleBackup public or restructuring the API.
| internal sealed class ShuffleBackup | |
| public sealed class ShuffleBackup |
Copilot uses AI. Check for mistakes.
Updated `PlaybackNavigationResult` to enforce non-nullable `NextItem`, simplifying its structure and removing unused properties and constructors. Updated `IPlaybackControlService` and `PlaybackControlService` methods to return nullable `PlaybackNavigationResult?` instead of creating instances with `null` values. Simplified logic in `MediaListViewModel` to handle nullable results consistently.
|
@United600 Can you help me test this PR? It's a huge refactor that is needed for the persistent playlist feature in #514. I have done some testing on my end, but I just want to make sure there is no regression. |
Of course, I'd be happy to. Are there any specific actions or features I should pay special attention to? |
|
It seems that opening files directly from the explorer doesn't work. Gravacao.2025-09-29.192435.mp4Reordering the play queue also leads to unexpected behavior, and possible locks (and crashes) when clicking the Previous button. Gravacao.2025-09-29.193300.mp4 |
Screenbox.Core/Screenbox.Core.csproj
Outdated
| <None Include="REFACTORING_SUMMARY.md" /> | ||
| <None Include="TESTING_GUIDE.md" /> |
There was a problem hiding this comment.
Good catch! Definitely leftovers from vibe coding
Introduced `MediaViewModelFactory` in `MediaListViewModel` to short circut media list parsing whenever possible. Replaced `UpdateItemsFromPlaylist` with `LoadFromPlaylist` and `SaveToPlaylist` methods, separating UI syncing and playlist state management. Consolidated file handling logic into a new `ParseAndPlayAsync` method, unifying media type handling. Added `CreatePlaylistAndPlay` to reduce duplication. Enhanced shuffle handling with robust backup updates in `OnCollectionChanged`. Updated navigation methods (`NextAsync`, `PreviousAsync`, `OnEndReached`) to save playlist state before actions. Removed redundant methods (`HandleSingleFileAsync`, `HandleMultipleFilesAsync`, `EnqueueAndPlay`).
MediaListViewModelfrom a monolithic class into a clean MVVM architecture, separating business logic, domain models, and presentation layer for maintainability and testability.IPlaylistService,IPlaybackControlService,IMediaListFactory) to encapsulate playlist operations, navigation logic, and media parsing, all injected via dependency injection.