feat: Jellyfin SyncPlay support#735
Conversation
There was a problem hiding this comment.
Probably better to move these callbacks to the ExoPlayer and listen to state changes from the player itself.
There was a problem hiding this comment.
Same here I'm not even sure we need to use these callbacks.
The player already reports back it's state to flutter we could probably use that for reporting seek/pause/play states. That way we don't rely on any additional kotlin implementation.
| modifier: Modifier = Modifier | ||
| ) { | ||
| val syncPlayState by VideoPlayerObject.syncPlayCommandState.collectAsState() | ||
| val visible = syncPlayState.processing && syncPlayState.commandType != null |
There was a problem hiding this comment.
This will recalculate whenever the state changes. Probably fine for a small composable but lets change this to
| val visible = syncPlayState.processing && syncPlayState.commandType != null | |
| val visible by remember(syncPlayState) { | |
| derivedStateOf { | |
| syncPlayState.processing && syncPlayState.commandType != null | |
| } | |
| } |
| Translate( | ||
| callback = { cb -> | ||
| when (syncPlayState.commandType) { | ||
| "Pause" -> Localized.syncPlayCommandPausing(cb) |
There was a problem hiding this comment.
Create a enum for this. Pigeon supports enum's that way both flutter/kotlin are in sync and we don't rely on Strings.
| } | ||
|
|
||
| // SyncPlay command state for overlay | ||
| data class SyncPlayCommandState( |
There was a problem hiding this comment.
Let's put this in pigeon.
| key: const Key("Search"), | ||
| onPressed: () => context.router.navigate(LibrarySearchRoute()), | ||
| child: const Icon(IconsaxPlusLinear.search_normal_1), | ||
| ); |
There was a problem hiding this comment.
Not sure about the position of this widget. Let's remove it from the dashboard for now.
We should not be using multiple fabs together in a single navigation rail.
We'll have to find a better spot.
| final isProcessing = ref.watch(syncPlayProvider.select((s) => s.isProcessingCommand)); | ||
| final processingCommand = ref.watch(syncPlayProvider.select((s) => s.processingCommandType)); | ||
|
|
||
| final (icon, color) = switch (groupState) { |
|
|
||
| String _getProcessingText(BuildContext context, String? command) { | ||
| return switch (command) { | ||
| 'Pause' => context.localized.syncPlaySyncingPause, |
There was a problem hiding this comment.
This is re-used quite a lot just adding a reminder to replace this with the enum and extension method.
| _loadGroups(); | ||
| } | ||
|
|
||
| Future<void> _loadGroups() async { |
There was a problem hiding this comment.
Would be cleaner to lift this state out of the widget and put it in a provider.
| flutter_native_splash: ^2.4.7 | ||
| macos_window_utils: ^1.9.0 | ||
|
|
||
| web_socket_channel: ^3.0.3 |
There was a problem hiding this comment.
Let's move it to "# Network and HTTP" group.
There was a problem hiding this comment.
First of thanks for implementing this, pretty big PR. But something a lot of people where requesting 👍🏼.
Works pretty well for the most part, some notes/quirks though. These are some initial findings will have to go over it after some changes.
About the UI itself. I left some comments about UI choices. However I will probably go over it myself to make some changes to bring it more in line with Fladder as it currently is.
UX:
We should show a loading indicator when any of the users press a play button. Now it has to load for a bit before the playback starts because Fladder is still synchronising.
When a player “stops” playback should all other participants return to the previous screen as well?
Architecture:
Currently most of the calls inside of the.UI go to videoplayerprovider but it now either calls the original player.pause or a new syncprovider.pause.
Like mentioned in the comments it would be better to listen to the players state stream and adjust everything based on that.
Bugs:
Playback stops working when syncplay becomes out of sync. Leaving/creating a group does nothing to change this state.
Fladder starts playback and finishes loading the video but it remains in a “paused” state as if it’s awaiting the syncplay to synchronize.
Sometimes “play” commands seem to not propagate to other users
|
Also mentioned in some comments. But there is a lot of re-formatting making it difficult to review the changes. Please re-format all files using the .vscode/settings.json. The biggest issue being the 120 line length currently not being used in your formatter. |
|
Hi! What's the status of this PR? Are the fixes discussed above still needed, or has everything been fixed? I'm curious because I'm really looking forward to this feature. Thanks for such a great app! |
|
Hi ! |
|
I you need someone to test it out I can help. |
|
I've pushed the fixes and added sync correction (SpeedToSync and SkipToSync) that mirrors the behavior of the syncplay plugin on the official jellyfin-web. @PartyDonut I did not change the added UI for the syncplay. If you have an idea of how it should look I can implement it or we can merge this PR to another branch where you can make the UI changes |
|
Thanks for the improvements, had a quick test it does seem more stable then previously. I will have a closer look next week. When we are in a group and the users presses play nothing really happens, however in the background it is starting the playback. We should show a "loading" overlay/pop-up for all the people in that group so users don't start pressing play multiple times. Can we also have a "user has joined" pop-up. Currently the groups creator does not see a message. If any of the users stops playback the other users keep on viewing is this the intended behavior? If yes should a user be able to join back? About the UI, it's fine to leave it as is when you are done with your work on the PR I will just merge this and create a new PR for easier reviewing. |
|
Hi! I built a build based on this PR and noticed the following: The next episode switcher isn't working correctly. When I press the button to start an episode, it starts right at the end of the timeline. Sometimes, after pausing and unpausing, someone in the group gets kicked out and has to rejoin, but there's no notification about leaving the group. Do you need any examples from me in the form of screenshots, screen recordings, or logs? The server logs show nothing unusual, but the client logs only show errors, which are also blank. |
PartyDonut
left a comment
There was a problem hiding this comment.
Sorry for the late response, have been a bit busy. Also sorry about the merge conflicts quite a lot of re-work needed to get music working.
I took a quick glance through the files, at this point there is no way for me to properly review this PR.
There are a lot of files that are marked as changed but are not related to the PR at hand (might be because of the music re-work?), at this point the best path forward would be to re-base it on develop make sure all the changes are related to the sync-play functionality.
|
While testing this branch I hit a bug: starting playback from Fladder in a SyncPlay group didn't start it on the official clients — TV episodes never started (movies did), and "Continue Watching" restarted the group from 0:00. Verified against both the official Jellyfin web client and the LG webOS app. Root cause: Fix: I opened a PR against the |
Removes generated-file drift so the PR diff shows only SyncPlay-related changes (per maintainer review on DonutWare#735). Non-SyncPlay generated files (account/credentials/boxset/etc.) now match develop and drop out of the diff.
Revert files that syncplay only reformatted (import order, wrapping, blank lines) with no functional change, so the PR diff contains only SyncPlay-related changes (per maintainer review on DonutWare#735).
|
Thanks for the feedback. You were right that the diff was unreviewable. I've rebased onto the current
Happy to split further or adjust anything if it helps the review. |
Pull Request Description
Adds Jellyfin SyncPlay support so users can watch media together in sync across devices.
docs/syncplay-implementation.mddocuments the protocol and architecture.Issue Being Fixed
Feature request: SyncPlay support for watching together with other Jellyfin clients.
Screenshots / Recordings
fladder_syncplay_demo_beta_compressed.mp4
Checklist
(Added:
web_socket_channel^3.0.3 — used for SyncPlay WebSocket. pub.dev; cross-platform.)