NewPipe icon indicating copy to clipboard operation
NewPipe copied to clipboard

Set compileSdk and targetSdk to 33 (Android 13)

Open Stypox opened this issue 2 years ago • 12 comments

What is it?

  • [x] Bugfix (user facing)
  • [ ] Feature (user facing)
  • [x] Codebase improvement (dev facing)
  • [ ] Meta improvement to the project (dev facing)

Description of the changes in your PR

android:exported in now required in the manifest on all activities/services/receivers/providers. It was set to true for those that need to interact with outside apps or the OS, while others have exported=false. Changing the target sdk also required updating LeakCanary to the latest version as the older version being used was not using android:exported in AndroidManifest.xml.

I don't know whether problems could arise just because of this number increase, maybe @TacoTheDank can shed some light on it?

Fixes the following issue(s)

  • https://github.com/TeamNewPipe/NewPipe/issues/9292#issuecomment-1304571423

Relies on the following changes

  • #9333

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.

Due diligence

Stypox avatar Nov 05 '22 22:11 Stypox

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Nov 05 '22 23:11 sonarqubecloud[bot]

Opening any video crashes the app.

Exception

  • User Action: ui error
  • Request: ACRA report
  • Content Country: US
  • Content Language: en-US
  • App Language: en_US
  • Service: none
  • Version: 0.24.0
  • OS: Linux Android 13 - 33
Crash log

java.lang.IllegalArgumentException: org.schabi.newpipe.debug.targetapi33: Targeting S+ (version 31 and above) requires that one of FLAG_IMMUTABLE or FLAG_MUTABLE be specified when creating a PendingIntent.
Strongly consider using FLAG_IMMUTABLE, only use FLAG_MUTABLE if some functionality depends on the PendingIntent being mutable, e.g. if it needs to be used with inline replies or bubbles.
	at android.app.PendingIntent.checkFlags(PendingIntent.java:401)
	at android.app.PendingIntent.getBroadcastAsUser(PendingIntent.java:671)
	at android.app.PendingIntent.getBroadcast(PendingIntent.java:658)
	at org.schabi.newpipe.player.notification.NotificationUtil.createNotification(NotificationUtil.java:136)
	at org.schabi.newpipe.player.notification.NotificationUtil.createNotificationAndStartForeground(NotificationUtil.java:185)
	at org.schabi.newpipe.player.notification.NotificationPlayerUi.initPlayer(NotificationPlayerUi.java:32)
	at org.schabi.newpipe.player.Player$$ExternalSyntheticLambda0.accept(Unknown Source:2)
	at j$.util.Iterator$-CC.$default$forEachRemaining(Iterator.java:116)
	at j$.util.Iterator$-EL.forEachRemaining(Unknown Source:10)
	at j$.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
	at j$.util.stream.ReferencePipeline$Head.forEachOrdered(ReferencePipeline.java:590)
	at org.schabi.newpipe.player.ui.PlayerUiList.call(PlayerUiList.java:88)
	at org.schabi.newpipe.player.Player.initPlayer(Player.java:522)
	at org.schabi.newpipe.player.Player.initPlayback(Player.java:487)
	at org.schabi.newpipe.player.Player.lambda$handleIntent$2$org-schabi-newpipe-player-Player(Player.java:425)
	at org.schabi.newpipe.player.Player$$ExternalSyntheticLambda27.run(Unknown Source:16)
	at io.reactivex.rxjava3.internal.operators.maybe.MaybeCallbackObserver.onComplete(MaybeCallbackObserver.java:93)
	at io.reactivex.rxjava3.internal.operators.maybe.MaybeObserveOn$ObserveOnMaybeObserver.run(MaybeObserveOn.java:105)
	at io.reactivex.rxjava3.android.schedulers.HandlerScheduler$ScheduledRunnable.run(HandlerScheduler.java:123)
	at android.os.Handler.handleCallback(Handler.java:942)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loopOnce(Looper.java:201)
	at android.os.Looper.loop(Looper.java:288)
	at android.app.ActivityThread.main(ActivityThread.java:7898)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:936)


tsiflimagas avatar Nov 05 '22 23:11 tsiflimagas

If this pull request was merged, a new version would be released.

ghost avatar Nov 06 '22 13:11 ghost

@Nizami20052022 Please be aware that you are creating a lot of comments and new issues. Many of them do not add any value to the project or its discussions and are just consuming resources for reviewing and replying. Our resources are limited. Please make sure to stick to our contribution guidelines. If you fail to do so, we might need to restrict your ability to interact with our repositories to not bind any more resources.

TobiGr avatar Nov 06 '22 13:11 TobiGr

I haven't opened many issues.😪

I report problems.I will not open any more issues. Sorry!😪

ghost avatar Nov 06 '22 14:11 ghost

@Nizami20052022 It's more about the amount of effort it takes to deal with it. If a lot of your opened issues are being closed, and your comments being marked off-topic, that's an indication for you to read more and comment less. It will improve the quality of your interactions in FOSS repos in general, too.

opusforlife2 avatar Nov 06 '22 17:11 opusforlife2

If you allow me, I can create my own pull request.

ghost avatar Nov 06 '22 17:11 ghost

PRs are always welcome, as we've accepted yours previously. There's no permission needed. Just make sure the code is of good quality and follow our contribution guidelines.

opusforlife2 avatar Nov 06 '22 17:11 opusforlife2

I can contribute other than translation.I finished the translation.Don't worry about code quality.

ghost avatar Nov 06 '22 17:11 ghost

I don't know whether problems could arise just because of this number increase, maybe TacoTheDank can shed some light on it?

I recommend reading Android version differences (API 30 - changes when targeting this version, API 31 - changes when targeting this version, API 32 and API 33 - changes when targeting this version) and changelog of Jetpack libraries updates, to give us an idea about what changes are required.

AudricV avatar Nov 06 '22 17:11 AudricV

I second @AudricV's suggestion.

TacoTheDank avatar Nov 07 '22 04:11 TacoTheDank

Opening any video crashes the app.

The PendingIntent mutability flags are required when targeting Android 12 and higher, and most of the PendingIntents created in the app don't have the flags set.

Isira-Seneviratne avatar Nov 07 '22 11:11 Isira-Seneviratne

You may also want to update com.android.tools:desugar_jdk_libs to 1.1.8 to target API level 33. (https://github.com/google/desugar_jdk_libs/blob/master/CHANGELOG.md)

antmonteiro avatar Nov 18 '22 00:11 antmonteiro

You may also want to update com.android.tools:desugar_jdk_libs to 1.1.8 to target API level 33. (https://github.com/google/desugar_jdk_libs/blob/master/CHANGELOG.md)

That's done here: #9285

Isira-Seneviratne avatar Nov 18 '22 07:11 Isira-Seneviratne

I pushed two commits making sure that NewPipe now requests the notification permission, as explained here.

On Android 13+ we need to specify player notification actions with the PlaybackState class, see here. This can be done in a separate PR I think, since currently the custom actions are not shown on Android 13 regardless of whether the app targets 29 or 33.

Stypox avatar Nov 28 '22 17:11 Stypox

The code smells are unrelated

Stypox avatar Nov 28 '22 18:11 Stypox

The code smells are unrelated

No, they are: see https://developer.android.com/reference/android/app/Activity.html?hl=en#onBackPressed() and https://developer.android.com/reference/android/content/Intent?hl=en#getSerializableExtra(java.lang.String)

These methods are deprecated since API 33 and used in MainActivity.

AudricV avatar Nov 28 '22 19:11 AudricV

I don't think I am going to fix those deprecations here.

For example, getSerializableExtra(String) suggests getSerializableExtra(String, class) as a replacement, but the latter requires API 33 and I couldn't find a similar method in IntentCompat, so it's not solvable as of now. If you or @Isira-Seneviratne find a solution feel free to push commits to this PR and then merge it; otherwise please merge it as it is and we will fix deprecations once IntentCompat catches up.

image

As for the onBackPressed deprecations, I am not sure how to proceed. There seems to be too many changes to do in this PR. I think it's better to do them in another PR.

Stypox avatar Dec 28 '22 22:12 Stypox

I took a deeper look into the onBackPressed deprecation. When we just need to always consume the onBack it is simple to switch to just register a callback that is always called. But when you have a conditional onBack (e.g. close the drawer or the search bar if it is open, otherwise let super handle the onBack), this requires enabling or disabling the callback based on whether the drawer or the search bar are open. This means having to continuously sync the "openness" state of drawer/search with the "enabledness" state of the onBack callback. Not only does this require much more code than before, it also greatly enlarges the possibility for programming errors and non-synced state. Using enableable/disableable onBack callbacks in a state-based UI system like Jetpack Compose would work perfectly well, but for us it would be a nightmare. So I will merge this PR as it is.

Stypox avatar Dec 31 '22 13:12 Stypox