Some simple refactors & beginning of kotlin conversions of the player classes #11965

Profpatsch · 2025-01-27T15:14:56Z

What is it?

Bugfix (user facing)
Feature (user facing)
Codebase improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

First few commits are from #11829 (merge first & rebase)
This partially conflicts with #9592

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.

Due diligence

I read the contribution guidelines.

And simplify the code a little

In Kotlin, dealing with nulls works better so we don’t need optional.

The new implementation would throw `ConcurrentModificationExceptions` when destroying the UIs. So let’s play it safe and put the list behind a mutex. Adds a helper class `GuardedByMutex` that can be wrapped around a property to force all use-sites to acquire the lock before doing anything with the data.

No adjustments done yet; only change is that I added an upper bound on `PlayerUi` to the `PlayerUiList` functions, so “everything” is now `null` in `destroyAll`.

This removes a bunch of unncessary checks. We forgo setting player to `null` in onDestroy, because shutting it down and when the service stops should be enough to free it for garbage collection.

Because the class is final, protected does not make sense (Android Studio auto-suggestions)

The `R.id` link from the comment cannot be resolved, so let’s not link it for now. We are using some exoplayer2 resources, let’s silence the warning.

It’s only used in this one place.

Isira-Seneviratne · 2025-02-01T05:18:15Z

Might be a good idea to look into migrating to Media3: https://developer.android.com/media/media3/exoplayer

Profpatsch · 2025-02-03T10:57:07Z

@Isira-Seneviratne that’s exactly what newPlayer does. This is the initial work to integrate NewPlayer into NewPipe

Profpatsch · 2025-02-03T10:58:11Z

i.e. adjust all the interfaces in a way that we can slot in NewPlayer easily into the existing player logic

Stypox

Generally looks good to me, thanks! You can probably ignore my comments in PlayerService since it will be quite different after the Android Auto PR anyway.

Stypox · 2025-02-27T20:10:12Z

app/src/main/java/org/schabi/newpipe/player/Player.java

+     * Saves the stream progress, sets recovery.
+     * Then destroys the player in all UIs and destroys the UIs as well.
+     */
+    public void saveAndShutdown() {
        if (DEBUG) {
            Log.d(TAG, "destroy() called");


Suggested change

Log.d(TAG, "destroy() called");

Log.d(TAG, "saveAndShutdown() called");

Stypox · 2025-02-27T20:12:11Z

app/src/main/java/org/schabi/newpipe/player/Player.java

@@ -1970,6 +1976,9 @@ public void setFragmentListener(final PlayerServiceEventListener listener) {
        triggerProgressUpdate();
    }

+    /** Remove the listener, if it was set.
+     * @param listener listener to remove
+     * */


Why do you write Javadocs this way? In my Android Studio when I type /** I already get the correctly formatted Javadoc. It's not a big issue, but it's strange you are deviating from the style used throughout the project, especially if it's also the standard style in Android Studio

Stypox · 2025-02-27T20:14:47Z

app/src/main/java/org/schabi/newpipe/player/PlayerService.kt

+            Log.d(
+                TAG,
+                (
+                    "onStartCommand() called with: intent = [" + intent +
+                        "], flags = [" + flags + "], startId = [" + startId + "]"
+                    )
+            )


Suggested change

Log.d(

TAG,

(

"onStartCommand() called with: intent = [" + intent +

"], flags = [" + flags + "], startId = [" + startId + "]"

)

)

Log.d(TAG, "onStartCommand() called with: intent = [$intent], flags = [$flags], startId = [$startId]")

Stypox · 2025-02-27T20:17:43Z

app/src/main/java/org/schabi/newpipe/player/PlayerService.kt

+        If the service is already started in foreground, requesting it to be started shouldn't
+        do anything
+         */
+        player.UIs().get<NotificationPlayerUi>(NotificationPlayerUi::class.java)


Suggested change

player.UIs().get<NotificationPlayerUi>(NotificationPlayerUi::class.java)

player.UIs().get(NotificationPlayerUi::class.java)

Stypox · 2025-02-27T20:17:50Z

app/src/main/java/org/schabi/newpipe/player/PlayerService.kt

+        }
+
+        player.handleIntent(intent)
+        player.UIs().get<MediaSessionPlayerUi>(MediaSessionPlayerUi::class.java)


Suggested change

player.UIs().get<MediaSessionPlayerUi>(MediaSessionPlayerUi::class.java)

player.UIs().get(MediaSessionPlayerUi::class.java)

Stypox · 2025-02-27T20:19:35Z

app/src/main/java/org/schabi/newpipe/player/ui/PlayerUiList.kt

+import java.util.Optional
+
+class PlayerUiList(vararg initialPlayerUis: PlayerUi) {
+    var playerUis = GuardedByMutex(mutableListOf<PlayerUi>())


Suggested change

var playerUis = GuardedByMutex(mutableListOf<PlayerUi>())

private val playerUis = GuardedByMutex(mutableListOf<PlayerUi>())

Stypox · 2025-02-27T20:21:48Z

app/src/main/java/org/schabi/newpipe/player/ui/VideoPlayerUi.java

@@ -761,7 +762,7 @@ public boolean isFullscreen() {
    }

    /**
-     * Update the play/pause button ({@link R.id.playPauseButton}) to reflect the action
+     * Update the play/pause button (`R.id.playPauseButton`) to reflect the action


Suggested change

* Update the play/pause button (`R.id.playPauseButton`) to reflect the action

* Update the play/pause button ({@code R.id.playPauseButton}) to reflect the action

snaik20

Just a few minor comments to look into

snaik20 · 2025-03-06T15:52:23Z

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java

@@ -244,7 +244,7 @@ public void onServiceConnected(final PlayerService connectedPlayerService,
        // It will do nothing if the player is not in fullscreen mode
        hideSystemUiIfNeeded();

-        final Optional<MainPlayerUi> playerUi = player.UIs().get(MainPlayerUi.class);
+        final Optional<MainPlayerUi> playerUi = player.UIs().getOpt(MainPlayerUi.class);


I would suggest the following rather than creating a deprecated getOpt function

Suggested change

final Optional<MainPlayerUi> playerUi = player.UIs().getOpt(MainPlayerUi.class);

@Nullable final MainPlayerUi playerUi = player.UIs().get(MainPlayerUi.class);

snaik20 · 2025-03-07T02:31:18Z

app/src/main/java/org/schabi/newpipe/player/ui/PlayerUiList.kt

+     * destroyed and removed
+     * @param T the class type parameter </T>
+     * */
+    fun <T : PlayerUi> destroyAllOfType(playerUiType: Class<T?>? = null) {


Should this type be Class<T>? instead of Class<T?>?

snaik20 · 2025-03-07T03:31:06Z

app/src/main/java/org/schabi/newpipe/player/PlayerService.kt

+import android.os.Binder
+import android.os.IBinder
+import android.util.Log
+import org.schabi.newpipe.player.PlayerService.LocalBinder


No need to import.

snaik20 · 2025-03-07T03:35:26Z

app/src/main/java/org/schabi/newpipe/player/PlayerService.kt

+     * Allows us this [PlayerService] over the Service boundary
+     * back to our [org.schabi.newpipe.player.helper.PlayerHolder].
+     */
+    class LocalBinder internal constructor(playerService: PlayerService?) : Binder() {


Suggested change

class LocalBinder internal constructor(playerService: PlayerService?) : Binder() {

class LocalBinder internal constructor(playerService: PlayerService) : Binder() {

snaik20 · 2025-03-07T04:09:34Z

app/src/main/java/org/schabi/newpipe/player/ui/VideoPlayerUi.java

@@ -947,6 +948,7 @@ public void onShuffleClicked() {
        player.toggleShuffleModeEnabled();
    }

+    @SuppressLint("PrivateResource")


We shold probably stop relying on the exoplayer's private resources.

Can you add a TODO here to replace the exo_controls_repeat_all, exo_controls_repeat_one and exo_controls_repeat_off drawables.

Profpatsch added 12 commits January 27, 2025 14:52

PlayerUIList: transform to kotlin

749c710

And simplify the code a little

PlayerUIList: rename get to getOpt and make get nullable

23aa1f8

In Kotlin, dealing with nulls works better so we don’t need optional.

PlayerService: Convert to kotlin (mechanical)

487f08c

No adjustments done yet; only change is that I added an upper bound on `PlayerUi` to the `PlayerUiList` functions, so “everything” is now `null` in `destroyAll`.

PlayerService: use lateinit for the player

7c1d33d

This removes a bunch of unncessary checks. We forgo setting player to `null` in onDestroy, because shutting it down and when the service stops should be enough to free it for garbage collection.

PlayerService: simplify nullable calls

b9eeee8

VideoDetailFragment: apply visibility suggestions

969f59a

Because the class is final, protected does not make sense (Android Studio auto-suggestions)

VideoDetailFragment: apply more IDE suggestions

44cb912

VideoDetailFragment: remove duplicate code in startLoading

b82d048

VideoPlayerUi: suppress warnings

e56307a

The `R.id` link from the comment cannot be resolved, so let’s not link it for now. We are using some exoplayer2 resources, let’s silence the warning.

PlayerHolder: inline setListener

c9d273b

It’s only used in this one place.

PlayerHolder: improve interface docstrings

5a2047f

github-actions bot added the size/giant PRs with more than 750 changed lines label Jan 27, 2025

ShareASmile added codequality Improvements to the codebase to improve the code quality rewrite Issues and PRs related to rewrite player Issues related to any player (main, popup and background) labels Jan 27, 2025

Stypox requested changes Feb 27, 2025

View reviewed changes

snaik20 reviewed Mar 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some simple refactors & beginning of kotlin conversions of the player classes #11965

Some simple refactors & beginning of kotlin conversions of the player classes #11965

Profpatsch commented Jan 27, 2025 •

edited by Stypox

Loading

Isira-Seneviratne commented Feb 1, 2025

Profpatsch commented Feb 3, 2025

Profpatsch commented Feb 3, 2025

Stypox left a comment

Stypox Feb 27, 2025

Stypox Feb 27, 2025

Stypox Feb 27, 2025

Stypox Feb 27, 2025

Stypox Feb 27, 2025

Stypox Feb 27, 2025

Stypox Feb 27, 2025

snaik20 left a comment

snaik20 Mar 6, 2025

snaik20 Mar 7, 2025

snaik20 Mar 7, 2025

snaik20 Mar 7, 2025

snaik20 Mar 7, 2025

	Log.d(TAG, "destroy() called");
	Log.d(TAG, "saveAndShutdown() called");

	player.UIs().get<NotificationPlayerUi>(NotificationPlayerUi::class.java)
	player.UIs().get(NotificationPlayerUi::class.java)

	player.UIs().get<MediaSessionPlayerUi>(MediaSessionPlayerUi::class.java)
	player.UIs().get(MediaSessionPlayerUi::class.java)

	var playerUis = GuardedByMutex(mutableListOf<PlayerUi>())
	private val playerUis = GuardedByMutex(mutableListOf<PlayerUi>())

	* Update the play/pause button (`R.id.playPauseButton`) to reflect the action
	* Update the play/pause button ({@code R.id.playPauseButton}) to reflect the action

	final Optional<MainPlayerUi> playerUi = player.UIs().getOpt(MainPlayerUi.class);
	@Nullable final MainPlayerUi playerUi = player.UIs().get(MainPlayerUi.class);

	class LocalBinder internal constructor(playerService: PlayerService?) : Binder() {
	class LocalBinder internal constructor(playerService: PlayerService) : Binder() {

Some simple refactors & beginning of kotlin conversions of the player classes #11965

Are you sure you want to change the base?

Some simple refactors & beginning of kotlin conversions of the player classes #11965

Conversation

Profpatsch commented Jan 27, 2025 • edited by Stypox Loading

What is it?

Description of the changes in your PR

APK testing

Due diligence

Isira-Seneviratne commented Feb 1, 2025

Profpatsch commented Feb 3, 2025

Profpatsch commented Feb 3, 2025

Stypox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snaik20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Profpatsch commented Jan 27, 2025 •

edited by Stypox

Loading