Conversation
Implements shuffle for series, collections, and libraries with queue management & visual display on iOS; Introduces an Options menu in ActionButtonHStack that contains shuffle functionality and is designed to accommodate future actions.
|
Very cool, your implementation looks much more complete than mine. The one small suggestion I have is to move the shuffle button on the item details page. I don't think it should be behind an options menu, since it is just one option. An alternate idea I have is putting the shuffle button to the right of the play button, either taking a half, a third, or a square of the horizontal space. My reasoning for this is that in my opinion it is more similar to the play action than the actions in the HStack. I do understand why the play button should remain the main focus though. That is why I would personally prefer a smaller button to the right of the play button with just the shuffle symbol and no text. I will create a few visual examples using your repository in a few hours to make it more clear. |
JPKribs
left a comment
JPKribs
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
I haven't had a chance to test this out but just 2 UI changes that stand out to me. Please let me know if you have any questions!
I also would ask you clean up some of the comments to mirror other Swiftfin classes. We've historically only left comments for items that require explanation or break standard practice. The code itself should be self explanatory (which is appears to currently be). Primarily, I just mean the comments over func.
Since this is a playback change, I'm going to lean on @LePips since he is a lot more familiar with this. He can help provide a more technical/in-depth review for your changes.
Swiftfin tvOS/Views/ItemView/Components/ActionButtonHStack/ActionButtonHStack.swift
Outdated
Show resolved
Hide resolved
|
|
||
| // MARK: - Options Menu | ||
|
|
||
| if viewModel.item.canShuffle { |
There was a problem hiding this comment.
Similar to tvOS. I'm not sure this belongs in the toolbar menu on iOS (the advanced settings counterpart vs tvOS) but I'm not sure if it goes here either. I almost wonder if this is a contextMenu on the play button instead like how resume/start from beginning works?
There was a problem hiding this comment.
I've moved the Shuffle option to a long press on the Play button:
| Play button | Play button (long press) |
|---|---|
 |
 |
Although I had some concerns when I started working on it - the context menu might not be obvious enough w/o any display that a long press has some extra options.
One more thing I noticed is that Collections (boxSet) don't display a Play button on iOS (by design in presentPlayButton property) - and even though it could be shuffled, there's no actual way of doing so. Unlike tvOS which shows the Advanced Options button and allows us to shuffle a boxSet:
| tvOS | iOS |
|---|---|
 |
 |
I'd consider either adding the Advanced Options to the iOS view or keeping the long press but bringing it to tvOS (which is in the progress afaik).
There was a problem hiding this comment.
The toolbar button is available in the Media tab, in case users wanted to shuffle Movies, for example.
It's also right next to "Random" which seems appropriate.
This is iOS only at the moment since tvOS doesn't have a toolbar.
There was a problem hiding this comment.
Hm... You are completely right. Part of where I am getting stuck is that the play button row is for playback, and the action button row are for actions that are not directly related to playback. Favoriting, marking has complete completed, trailers, etc.
If we keep that pattern, the shuffle definitely belongs on the play button row, but I agree the context menu isn't readily apparent. Pulling from other examples, the context menu seems to be used in other similar iOS-centric apps (example: Infuse)
Apple TV + doesn't even seem to have a shuffle so unfortunately, we don't have a direct Apple example.
I remember the original reason that we got rid of the play button on collections was because the steps that it took to go from the collection to an individual piece of content could often times be several jumps. For example, if I had a television show in a collection, I would need to go from collection to show to season to episode. This also meant that the logic for the play button was costly as well because we would want to find the next available piece of content that was unplayed potentially requiring us to dig into items.
Full transparency, I've been busy since the playback changes so I haven't had a great opportunity to look into what all is entailed for starting playback anymore. I know the information required is a lot different so maybe we need to look at revisiting the play button on collections? But, my guess is that we're still in the same spot for that. Which might also be a hangup for shuffling on collections as well? But, play button on collections would enable this in both locations and might be more consistent to do it this way on both iOS and tvOS so long as playback on collections is doable where we don't need a dozen server calls to calculate the correct playback item.
Sorry, I'm just thinking "out loud". I should hopefully have some time this weekend where I can look at this more in-depth and hopefully we can figure out a good place to put this! I'll reach out when I've had a chance to look at this more
There was a problem hiding this comment.
I actually like the long button press, and maybe it doesn't need to be obvious, or maybe it's obvious enough idk.
Shuffle on collections isn't top priority I think, we could leave that as a todo and whenever collection playback is improved shuffle would be pretty easy to enable.
No problem, I'm cool with either honestly
There was a problem hiding this comment.
I am so sorry for the wait on this! I am pulling this down and taking a look at everything now. There were a couple changes on main updating the SDK to 10.11 so you will need to rebase to make sure you get everything but I think the localizations are the only part that isn't an auto-merge.
I will let you know as I wrap up testing this but please feel free to reach out if you have any trouble merging back with Main!
There was a problem hiding this comment.
Some notes:
Works well for a show but I think we'd want to make sure we can limit this to specific groupings. Something similar to what I started (and never finished) here: #1647. Specifically, we should have an option in the SeasonItemView to either shuffle a full series or just that specific season. I'm not sure how that would work menu wise but I think the ViewModel we can just add the ParentID as a parameter for input.
Movie shuffling, I am having some issues but these are purely related to the SDK changes that are now in Main. These seem to be resolved by bumping this version and getting this on 0.6.0. Since 10.11 added some HDR variations, if one of those HDR shows up in the shuffle everything crashes. Main resolves this.
The larger issue is the performance to start shuffling can be poor for larger shows or libraries. I think we need to think about a way to paginate the shuffle so we grab ~10 items, then grab more items later on. I'm not quite sure what that would look like. The issue I am thinking about is there are some shows with ~1,000 episodes. So shuffling those would take a decent chunk on this version.
Last UI item, I am actually a fan of the context menu that makes sense to me. I have family that still uses Infuse so that could be part of it that mirrors something I am familiar with but I think it makes sense. Only change I think would be valuable is add a loading spinner or something when the shuffle is selected. Since, now, we can have a few hundred items queue for shuffling, I found I could select shuffle then move like 5 views away before the player popped up. Adding a spinner will keep the user in place and also provide some feedback that the shuffle worked especially on slower networks where that call might take a second.
I think we want to extend the BaseItemDto and add a canShuffle function that we can use to toggle shuffle on and off. Additionally, we can put the shuffle logic as an override on the ItemView as a whole and cascade that down since we will want that for Music eventually as well.
There was a problem hiding this comment.
Hi, I am a bit confused why we use the local .shuffled() function, when there is a random itemSortBy option using in the api. Sorry to repeat myself, but can you take a look at my last comment on this pr. It's very possible I am overlooking something, but I made a pr on the fork just in case. Thanks.
There was a problem hiding this comment.
I am a bit confused why we use the local .shuffled() function
Sorry, yes I agree that we should use the API random sort as well. Using the API random plus pagination, we should be able to get the paginated results I was mentioning.
Apologies, I didn't mean to skip over your suggestion. I do agree that is the best way to go about this!
There was a problem hiding this comment.
All good. I feel like the Episodes button behavior is confusing as well. When I play an episode normally, the button shows the other episodes in the season. When I shuffle a show, the button shows the entire queue of episodes. Is the behavior of this button intended to be a queue?
If it is intended to be a queue then I think it should probably normally go beyond the season of the current episode. For example, if you play the last episode of a season, maybe the next season should show in this menu. This could work well with pagination, if it was just a queue of the next 10 episodes both ways.
|
I never realized there was a context menu on the play button. That may be a better idea than mine. |
|
One of these options is how I imagined the shuffle button on iOS:
|
Please make sure anything inline with the play button also accounts for the version selector. iOS it's in the action bar but tvOS it takes the same placement as your shuffle button. From an older PR, this is where that goes now: Simulator.Screen.Recording.-.Apple.TV.4K.3rd.generation.-.2025-03-20.at.12.49.44.mp4The context menu, to me, feels like our best option. Mirrors our resume/play from beginning while still being intuitive (I think). That being said, I'm not the most UI-minded guy in the world so there definitely could be a better, more intuitive solution. I'd prefer if we could keep it consistent between iOS and tvOS if possible. I'm a broken record but I'd be interested in @LePips's insight as he's a lot more involved in SwiftUI standard practice than I am. |
|
I'm sorry, I was not aware of the version selector's position on tvOS. Ironically that is inconsistent across platforms. I'll try my best to succinctly explain why I don't think the shuffle button should be in the context menu. Currently the play button has two actions as you mentioned, resume and play from the beginning, both of these actions pertain to a single episode. The shuffle action on the other hand is unrelated to the episode displayed on the play button as it shuffles episodes from the entire show. As a side note, I mentioned this before, but the context menu as a whole was not intuitive in my experience. |
Move Shuffle to Play Button Context Menu (iOS) & Advanced Options Menu (tvOS);
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
In my opinion, |
There was a problem hiding this comment.
Apologies, as I do change how libraries are made in #1752 - which afterward would make the implementation of this feature easier.
Based on the discussion above:
- This would be accessible from the play button via the context menu
- We can't retrieve all items from a parent at once, which is what the calls currently do, and only have a look-ahead of ~20 items. We should be retrieving the items from the server using the sort parameter rather than shuffling locally.
Based on general review:
- We should find a way to consolidate the views used between the episode queue supplement and this supplement, since they are exactly the same.
- Retrieving the child items from a parent doesn't need to be split between series/boxset. After #1752, we should be able to use an existing library and just pass in the filters. Additionally, we wouldn't specify the item types but use the
.videomedia type.
|
Hello there, sorry for the late reply, I'll be updating this comment along with commits with fixes. I'll address all of them in the coming days. Gonna keep this checklist here for now:
Since #1752 is pretty big but can make this implementation easier, I can either branch from
I'm not in a rush, if we can keep this PR here until #1752 is merged I'm cool with that, gonna work on something else in the meantime. |
|
Hello there, I've updated the branch following the latest comments. In short:
Shuffle flow
|
4 participants
Add Shuffle Playback Support
Summary
Implements shuffle playback for shows, collections, and library views. Users can shuffle from item detail pages, from library navigation menus, and view/navigate the shuffled queue on iOS.
Related Issues
Closes #673
New Files
ShuffleMediaPlayerQueue.swift- Queue management with iOS visual displayShuffleActionHelper.swift- Centralized shuffle logic for all item typesItemViewModel+Shuffle.swift- Unified shuffle playback method for ItemViewModelFeatures
Shuffle Entry Points
tvOS - Advanced Options Menu
Shuffle is accessible through the existing Advanced Options menu:
iOS - Play Button Context Menu
Shuffle is accessible via long-press on the Play button, alongside "Play from Beginning":
Library View Menu
Users can shuffle directly from library views (e.g., TV Shows, Collections):
Visual Queue Display (iOS/iPadOS Only)
On iOS and iPadOS, users can view and navigate the shuffled queue with a visual episode list:
iPadOS shuffle queue with episode list for quick navigation
Player Integration
Notes
EpisodeMediaPlayerQueuecomponents for consistencyPagingLibraryViewModel.swift,PagingLibraryView.swift(iOS), andPagingLibraryView.swift(tvOS) for implementing user-visible error messages. Currently, shuffle errors fail silently. A centralized error display mechanism is needed for background task errors across both platforms.