-
-
Notifications
You must be signed in to change notification settings - Fork 528
feat: Use online player for offline playback #7928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: Use online player for offline playback #7928
Conversation
|
Problems and Errors:
Also, if there is no internet, you may need to gray out some buttons to avoid errors. XRecorder_20251118_01.mp4Crash log:
XRecorder_20251118_10.mp4
XRecorder_20251118_04.mp4
XRecorder_20251118_06.mp4
XRecorder_20251118_11.mp4 |
|
I believe I have fixed those problems. It should prefer to use the metadata from the online The I also found that offline shuffle was crashing and fixed that too. |
XRecorder_20251118_13.mp4
XRecorder_20251118_16.mp4XRecorder_20251118_31.mp4
XRecorder_20251118_20.mp4
XRecorder_20251118_22.mp4
XRecorder_20251118_24.mp4
XRecorder_20251118_29.mp4
XRecorder_20251118_33.mp4
XRecorder_20251118_34.mp4
|
Bnyro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for the PR! I think this is a definitely a good idea user experience-wise.
The way I did this was by merging the functionality of the OfflinePlayerService into the OnlinePlayerService
I see why you did that, but that will make it very complicated to maintain that code in the future.
First, the StreamItem type is not the one perfect fit here, we should instead use the Streams type because it already contains that information so you don't have to edit StreamItem.kt. The information in StreamItem.kt is only meant for video objects from search results or similar places, which don't contain any information like license, metaInfo, ...
The better approach would be the following:
- Create some more abstract methods in
AbstractPlayerServicethat work with theStreamstype - Implement them in both the
OnlinePlayerServiceand theOfflinePlayerService - Make
PlayerFragment.ktwork with theAbstractPlayerServiceclass instead of theOnlinePlayerServiceclass
Let me know if you have some questions about how that would work, don't hesitate to ask :)
|
I appreciate the feedback; always good to know the right way to implement this to enable good maintainability. I went into this just trying to get it working so I could show off the vision and get insight on how to better implement this. I like the idea of implementing through the |
I have tested again and would you please take a look at the above? |
Yes, exactly. We could also create a different data class instead of using |
|
@xlash123 Is this PR still active? |
|
Yes, I just haven't had much free time recently to devote to this unfortunately. |
This patch allows for using the standard online player
PlayerFragmentfor both online and offline video playback. This will replace the need for theOfflinePlayerActivityand allow for a single method of video playback.The way I did this was by merging the functionality of the
OfflinePlayerServiceinto theOnlinePlayerService. I also used the metadata from theStreamItemin thePlayerFragmentto act as a the common interface for accessing media metadata, sinceDownloadalready supported a.toStreamItem()function.I've tested the following related parts to ensure working functionality:
PlayOfflineDialogstill works for selecting which version of the video to playPlayOfflineDialogThis is my first time really getting into Android development (and first time using Kotlin), so if you'd like me to make changes to clean this up, I am happy to do so and am open to suggestions on this. I hope you find this useful!