Skip to content
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

Set compileSdk and targetSdk to 33 (Android 13) #9306

Merged
merged 3 commits into from
Dec 31, 2022

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Nov 5, 2022

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

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)

Relies on the following changes

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

@tsiflimagas
Copy link
Contributor

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)


@ghost

This comment was marked as spam.

@TobiGr

This comment was marked as off-topic.

@ghost

This comment was marked as off-topic.

@opusforlife2

This comment was marked as off-topic.

@ghost

This comment was marked as off-topic.

@opusforlife2

This comment was marked as off-topic.

@ghost

This comment was marked as off-topic.

@AudricV
Copy link
Member

AudricV commented Nov 6, 2022

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.

@TacoTheDank
Copy link
Member

I second @AudricV's suggestion.

@Isira-Seneviratne
Copy link
Member

Isira-Seneviratne commented Nov 7, 2022

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.

Copy link

@acrodemocide acrodemocide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is crashing when attempting to view videos. There have already been several comments made in the conversations about it, so you're probably already aware. Once that is fixed, I'll take another look.

@antmonteiro
Copy link

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)

@Isira-Seneviratne
Copy link
Member

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

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.
This also required updating LeakCanary to the latest version as the older version being used was not using android:exported in AndroidManifest.xml.
@Stypox
Copy link
Member Author

Stypox commented Nov 28, 2022

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.

@sonarcloud
Copy link

sonarcloud bot commented Nov 28, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Stypox
Copy link
Member Author

Stypox commented Nov 28, 2022

The code smells are unrelated

@AudricV
Copy link
Member

AudricV commented Nov 28, 2022

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.

@Redirion Redirion mentioned this pull request Dec 16, 2022
5 tasks
@Stypox
Copy link
Member Author

Stypox commented Dec 28, 2022

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
Copy link
Member Author

Stypox commented Dec 31, 2022

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.

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.

9 participants