AntennaPod icon indicating copy to clipboard operation
AntennaPod copied to clipboard

Update AndroidX libraries

Open TacoTheDank opened this issue 1 year ago • 9 comments

If you take the AntennaPod v3.2.0 APK, disassemble it, and look at the metadata, you'll find that most of these newer library versions were already being compiled, despite the lower versions in gradle. Basically it's #5474 again. There's no reason not to update these, since they were already being used anyway. Any possible bugs would have been evident and fixed already.

  • Annotation was not 1.2.0, but actually 1.4.0. ~~I updated further to 1.7.1.~~
  • AppCompat was not 1.3.1, but 1.5.1.
  • Core was not 1.5.0, but 1.8.0.
  • Fragment was not 1.3.6, but 1.5.5. ~~I updated further to 1.5.7.~~
  • Media was not 1.4.3, but 1.6.0.
  • Mediarouter was not 1.2.5, but 1.3.0.

Some other changes:

  • Fixed an ActivityResult deprecation. ~~- CoordinatorLayout 1.1.0 -> 1.2.0 (changelog)~~ ~~- Core Splashscreen 1.0.0 -> 1.0.1 (changelog)~~
  • Added POST_NOTIFICATIONS checks. Tell me if these should be done differently.

TacoTheDank avatar Feb 25 '24 07:02 TacoTheDank

If you take the AntennaPod v3.2.0 APK, disassemble it, and look at the metadata

Note that you can do this more easily using ./gradlew :app:dependencies :)

I updated further to [...]

So there we are back to the same question as in other dependency update PRs: Does upgrading fix anything that affects AntennaPod? If it doesn't fix a known bug, it probably just adds new bugs. Almost every time we update dependencies, something non-obvious breaks on some devices. Last time it was #6692, for example.

ByteHamster avatar Feb 26 '24 20:02 ByteHamster

Does upgrading fix anything that affects AntennaPod? If it doesn't fix a known bug, it probably just adds new bugs.

This might be a stupid question, but are those questions even relevant if grade just ignores our decency instructions and takes newer versions when it feels like? Isn't it then better to make such changes consciously and give special attention to it during beta roll-out (even though it doesn't cover all devices)?

keunes avatar Feb 26 '24 21:02 keunes

It just does so as transitive dependencies, not "when it feels like" :) So upgrading one dependency can cause many more to get upgraded, but it does not happen automatically.

ByteHamster avatar Feb 26 '24 21:02 ByteHamster

@ByteHamster

(I changed Mediarouter back to just 1.3.0 since 1.6.0 requires compiling on 34. 1.3.0 was already being used, so no changes were made there.)

Just be be sure we're on the same page: because of transitive dependencies, AntennaPod APKs were already compiling on AppCompat 1.5.1, Core 1.8.0, etc. despite the Gradles specifying AppCompat 1.3.1, Core 1.5.0, etc. So, if there were any new possible bugs as a result of these upgrades, they would already exist in the app. Thus, bumping them in Gradle would not introduce any bugs that weren't already present in the current release.

  • For Annotation, the only difference between 1.4.0 and 1.7.1 is improved Kotlin compatibility.
  • Even though I only needed to bump Fragment to 1.5.5, I updated to 1.5.7 because it fixes bugs that are present in 1.5.5 and 1.5.6.
  • CoordinatorLayout only fixes internal proguard rules. I see no reason not to.
  • Core-splashscreen is a simple bugfix for Wear devices to comply with Android 12 splashscreen changes. I see no reason not to.

TacoTheDank avatar Feb 27 '24 01:02 TacoTheDank

At work they deploy any tooling changes separately from any functional changes, in order to better monitor/identify potential issues (to be sure an issue not caused by other changes). Might this be an idea for AntennaPod as well?

keunes avatar Feb 27 '24 07:02 keunes

Even though I only needed to bump Fragment to 1.5.5, I updated to 1.5.7

Yet another library upgrade caused yet another non-obvious regression (see #6944). Please don't do library upgrades unless it fixes a bug that AntennaPod users are affected by.

ByteHamster avatar Feb 29 '24 21:02 ByteHamster

@ByteHamster Removed extra bumps. Should be good now.

TacoTheDank avatar Mar 02 '24 22:03 TacoTheDank

I think the notification updates would be better in their own PR. The documentation says one should use https://developer.android.com/reference/android/app/NotificationManager#areNotificationsEnabled(). Does https://developer.android.com/reference/androidx/core/app/NotificationManagerCompat#areNotificationsEnabled() have a similar behavior then?

ByteHamster avatar Mar 02 '24 22:03 ByteHamster

https://developer.android.com/reference/android/app/NotificationManager#areNotificationsEnabled() isn't available for devices under Nougat. https://developer.android.com/reference/androidx/core/app/NotificationManagerCompat#areNotificationsEnabled() is a compatibility wrapper.

I included it here because of ~~Spotbugs~~ lint, but I can include it in its own PR if you want.

TacoTheDank avatar Mar 03 '24 03:03 TacoTheDank

I just tried experimenting with predictive back gestures in AntennaPod. These require a bunch of dependency updates. Maybe now is indeed a good time to upgrade stuff and deal with the bugs that it introduces. I mean, we have just started with 3.3.0 beta releases, so it will take a while before the next release, which means we have a decent amount of time for testing. Would you be interested in working on the upgrades?

I would prefer to do the upgrades step by step. A good first step could be to upgrade the build tools (Android Gradle plugin).

ByteHamster avatar Mar 03 '24 11:03 ByteHamster

@ByteHamster

Maybe now is indeed a good time to upgrade stuff and deal with the bugs that it introduces.

Good idea, but perhaps that should be for another PR. I think this one is good to merge now.

Would you be interested in working on the upgrades?

Sure thing. ~~Should I worry about the SpotBugs plugin not working? #6941~~ (Edit: doesn't matter now.)

TacoTheDank avatar Mar 03 '24 19:03 TacoTheDank