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.

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.

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

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

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

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

HatakeKakashri

Just a few minor comments to look into

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

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

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

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

Profpatsch · 2025-05-13T14:05:13Z

Rebased on top of #11829

I removed the “lateinit” commit for PlayerService, because there is no reasonable way to make the player non-nullable now that the Android Auto change has landed.

Stypox

After you fix the artifacts from the conversion to Kotlin feel free to merge this (or ask me for approval). Thanks!

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

Stypox · 2025-05-13T21:59:51Z

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

I think the Kotlin code style in this class can be improved, e.g. the Consumers can be replaced with just lambdas I believe, and a lot of !! can be removed or replaced with ? or reworked in general

Yupp, Can convert all Consumer and BiConsumer to lambdas

Stypox · 2025-05-13T22:01:04Z

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

-        if (Intent.ACTION_MEDIA_BUTTON.equals(intent.getAction())
-                && (player == null || player.getPlayQueue() == null)) {
+        if (Intent.ACTION_MEDIA_BUTTON == intent.action &&
+            (player == null || player!!.playQueue == null)


Suggested change

(player == null || player!!.playQueue == null)

(player?.playQueue == null)

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

Stypox · 2025-05-13T22:05:44Z

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

            if (onPlayerStartedOrStopped != null) {
                // notify that the player is being destroyed


Suggested change

if (onPlayerStartedOrStopped != null) {

// notify that the player is being destroyed

// notify that the player is being destroyed

onPlayerStartedOrStopped?.accept(null)

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

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

HatakeKakashri

Only few comments in Kotlin conversion. You can fix those and go ahead with merge.

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

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 a bit unwieldy in places, but should improve the safety of the code in the face of possible race conditions.

We directly call the `getService` function after receiving the argument, so resolving the WeakPointer should never return `null` in our case. Of course there could be a race condition in theory, but I feel like if that happens we have bigger problems?

Profpatsch · 2025-06-05T11:53:08Z

@snaik20 btw if you have some nitpicks/trivial improvements, it’s fine if you add a commit and push that directly on the branch; I’m guessing you have the editor open anyway and it prevents a bunch of back-and-forth

sonarqubecloud · 2025-06-05T12:11:58Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

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

HatakeKakashri reviewed Mar 7, 2025

View reviewed changes

Profpatsch force-pushed the player-classes-kotlin-conversion branch from 5a2047f to 31ade7c Compare May 13, 2025 14:04

Profpatsch requested a review from Stypox May 13, 2025 14:04

Stypox force-pushed the player-classes-kotlin-conversion branch from 31ade7c to b537be8 Compare May 13, 2025 21:45

github-actions bot added size/large PRs with less than 750 changed lines and removed size/giant PRs with more than 750 changed lines labels May 13, 2025

github-project-automation bot added this to Rewrite May 13, 2025

github-project-automation bot moved this to In Progress in Rewrite May 13, 2025

Stypox requested changes May 13, 2025

View reviewed changes

Profpatsch force-pushed the player-classes-kotlin-conversion branch from b537be8 to 8d0249b Compare May 14, 2025 16:44

github-actions bot added size/giant PRs with more than 750 changed lines and removed size/large PRs with less than 750 changed lines labels May 14, 2025

HatakeKakashri approved these changes May 31, 2025

View reviewed changes

ShareASmile added the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label Jun 1, 2025

Profpatsch added 7 commits June 5, 2025 13:52

PlayerUIList: restrict superclasses a little

545c4f0

PlayerService: Convert to kotlin (mechanical)

945fbd8

PlayerService: simplify nullable calls, getters

b5dd49e

VideoDetailFragment: apply visibility suggestions

4d6e1a4

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

VideoDetailFragment: apply more IDE suggestions

c37db85

VideoDetailFragment: remove duplicate code in startLoading

0b32738

VideoPlayerUi: suppress warnings

26050d8

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.

Profpatsch added 6 commits June 5, 2025 13:52

PlayerHolder: improve interface docstrings

06cf511

Player: destroy -> saveAndShutdown

f5a4af2

PlayerUIList: make UI list private

be373dc

PlayerService: remove !! where possible

36115c3

It’s a bit unwieldy in places, but should improve the safety of the code in the face of possible race conditions.

PlayerService/PlayerUIList: some small improvements

73fef26

github-actions bot removed the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label Jun 5, 2025

Profpatsch force-pushed the player-classes-kotlin-conversion branch from 31ca1c5 to 73fef26 Compare June 5, 2025 11:56

Profpatsch merged commit 006b4c9 into TeamNewPipe:refactor Jun 5, 2025
7 checks passed

github-project-automation bot moved this from In Progress to Done in Rewrite Jun 5, 2025

	(player == null \|\| player!!.playQueue == null)
	(player?.playQueue == null)

		if (onPlayerStartedOrStopped != null) {
		// notify that the player is being destroyed

Uh oh!

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

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

Conversation

Profpatsch commented Jan 27, 2025 • edited by Stypox Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is it?

Description of the changes in your PR

APK testing

Due diligence

Uh oh!

Isira-Seneviratne commented Feb 1, 2025

Uh oh!

Profpatsch commented Feb 3, 2025

Uh oh!

Profpatsch commented Feb 3, 2025

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HatakeKakashri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Profpatsch commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Stypox left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Stypox May 13, 2025

Choose a reason for hiding this comment

Uh oh!

HatakeKakashri May 31, 2025

Choose a reason for hiding this comment

Uh oh!

Stypox May 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Stypox May 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HatakeKakashri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Profpatsch commented Jun 5, 2025

Uh oh!

sonarqubecloud bot commented Jun 5, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Profpatsch commented Jan 27, 2025 •

edited by Stypox

Loading

Profpatsch commented May 13, 2025 •

edited

Loading

Stypox left a comment •

edited

Loading