Skip to content

Conversation

@xlash123
Copy link

This patch allows for using the standard online player PlayerFragment for both online and offline video playback. This will replace the need for the OfflinePlayerActivity and allow for a single method of video playback.

The way I did this was by merging the functionality of the OfflinePlayerService into the OnlinePlayerService. I also used the metadata from the StreamItem in the PlayerFragment to act as a the common interface for accessing media metadata, since Download already supported a .toStreamItem() function.

I've tested the following related parts to ensure working functionality:

  • PlayOfflineDialog still works for selecting which version of the video to play
  • Switching between audio and video still works as seamlessly as before
  • Selecting the video/audio from the downloads page will not prompt the PlayOfflineDialog

This 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!

@Figim
Copy link
Contributor

Figim commented Nov 17, 2025

Problems and Errors:

  1. You have turned off your internet connection. The app crashes when you play the downloaded video.

Also, if there is no internet, you may need to gray out some buttons to avoid errors.
Quality, Audio track, Captions, Download, Comments, Subscribe

XRecorder_20251118_01.mp4

Crash log:

Crashlog.docx

  1. If there is internet, YouTube-like description (currently not clickable) , subscription and view counts, and related videos should load properly. But it is not done.
XRecorder_20251118_10.mp4
  1. ** If the internet is open, play the downloaded video. Error when clicking the Caption button**
XRecorder_20251118_04.mp4

Crashlog2.docx

  1. The player does not work when switching from offline player to online player
XRecorder_20251118_06.mp4
  1. Cannot switch from notification to Offline audio player if there is no internet connection
XRecorder_20251118_11.mp4

@xlash123
Copy link
Author

I believe I have fixed those problems. It should prefer to use the metadata from the online Streams object, else fallback on the downloaded metadata.

The PlayerFragment (and likely other components) contain many references to the MainActivity, so instead of trying to make the NoInternetActivity work with this, I had the NoInternetFragment load into the nav if no internet was detected at launch.

I also found that offline shuffle was crashing and fixed that too.

@Figim
Copy link
Contributor

Figim commented Nov 18, 2025

  1. Video description is not clickable ( Not loading). This is problem for videos that are downloaded or not.
XRecorder_20251118_13.mp4
  1. If I play the currently downloaded audio ( (You are playing it now.) from the watch history or switch to the "switch to video player" button in the audio player, either the player is dark or the player is dark and related videos etc. content are not loading. (internet is open). If there is internet, even if this audio is the case, when switching to a video player, it should play the online video instead of the audio and load the content.
XRecorder_20251118_16.mp4
XRecorder_20251118_31.mp4
  1. When switching from offline video player to offline audio player, the audio player does not load if the internet is on or off.
XRecorder_20251118_20.mp4
  1. Error when clicking the Caption button in the offline video player while not connected to the internet
XRecorder_20251118_22.mp4
  1. If you not connected to the internet, you played a video that was not downloaded from your watch history. Click the "Background" button. Then, when you click the "Switch to online video player" button, an error occurred.
XRecorder_20251118_24.mp4
  1. Videos are not added to the queue when playing online videos from an offline player
XRecorder_20251118_29.mp4
  1. When the internet closed and the downloaded video is played as "play in the background" and then switched to the video player with the "video switch" button, the offline video does not play and the loading is endless.
XRecorder_20251118_33.mp4
  1. If the "Audio only mode" option is enabled, the audio player loads endlessly when playing a video downloaded without internet. No problem playing with the internet
XRecorder_20251118_34.mp4
  1. It's not your fault. But it would be great if issues Error when clicking on the "chapters" and "Switch to video" buttons that appear when the audio player is not fully loaded #7936 and If "Audio only mode" is on, the video played after the current audio will not open in the audio player. #7937 were also resolved with this PR?

Copy link
Member

@Bnyro Bnyro left a 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 AbstractPlayerService that work with the Streams type
  • Implement them in both the OnlinePlayerService and the OfflinePlayerService
  • Make PlayerFragment.kt work with the AbstractPlayerService class instead of the OnlinePlayerService class

Let me know if you have some questions about how that would work, don't hesitate to ask :)

@xlash123
Copy link
Author

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 AbstractPlayerService and can see how that would make things easier for maintaining. For conveying the metadata of these downloaded streams, is the idea to convert the data within DownloadWithItems into the Streams format, and then use that to implement any new Streams-based methods to be created in the AbstractPlayerService by the OfflinePlayerService?

@Figim
Copy link
Contributor

Figim commented Nov 18, 2025

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 AbstractPlayerService and can see how that would make things easier for maintaining. For conveying the metadata of these downloaded streams, is the idea to convert the data within DownloadWithItems into the Streams format, and then use that to implement any new Streams-based methods to be created in the AbstractPlayerService by the OfflinePlayerService?

I have tested again and would you please take a look at the above?

@Bnyro
Copy link
Member

Bnyro commented Nov 18, 2025

I like the idea of implementing through the AbstractPlayerService and can see how that would make things easier for maintaining. For conveying the metadata of these downloaded streams, is the idea to convert the data within DownloadWithItems into the Streams format, and then use that to implement any new Streams-based methods to be created in the AbstractPlayerService by the OfflinePlayerService?

Yes, exactly. We could also create a different data class instead of using Streams because Streams additionally contains audioStreams and videoStreams which we don't need here, but I think that would just overcomplicate things here.

@xlash123
Copy link
Author

@Bnyro understood, thanks for the guidance!

@Figim I see your tests; thanks for finding those! I'll tackle those after Bynro's refactoring suggestions.

@Figim
Copy link
Contributor

Figim commented Dec 2, 2025

@xlash123 Is this PR still active?

@xlash123
Copy link
Author

xlash123 commented Dec 2, 2025

Yes, I just haven't had much free time recently to devote to this unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants