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

Migrate app update checker to AndroidX Work #7975

Merged
merged 5 commits into from
Mar 15, 2022

Conversation

TacoTheDank
Copy link
Member

@TacoTheDank TacoTheDank commented Mar 1, 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

IntentService is deprecated, so I migrated to what was supposed to be its replacement (JobIntentService, which is also now deprecated), and then I migrated to AndroidX Work (which is now the recommended replacement).

Unfortunately this means yet another library, but it is what it is. Overall, this change only adds around 101 KB to the release build.

  1. I moved the utility methods out of the service class to make it smaller because that stuff didn't really belong in there. Also I cleaned up some parameters.
  2. Convert the new utility class to Kotlin so I can merge it together with NewVersionManager.
  3. Migrate the IntentService to what was supposed to be its replacement (JobIntentService). This itself would be enough to fix CheckForNewAppVersion randomly fails (when app is in background) #7397. However, I wanted to go further. This is because JobIntentService is also terrible in general and has an infamous crashing bug on API 26. Also it's deprecated (lol). So...
  4. Convert this Service class into a Worker. Workers are what we are now supposed to use. Also I renamed the class to make more sense.
  5. Converted this new Worker class to Kotlin.

I tested these changes on API 29. I checked the manual update button in settings and it works.

Here's more info:

Here's another PR in which I made this change: AntennaPod/AntennaPod#5742

Fixes the following issue(s)

APK testing

app-release updateCheckerWorker.zip

This is a specially generated APK where the isReleaseApk() method always returns true (thus guaranteeing that the update notification will always be displayed). Also the applicationId is slightly changed to allow side-by-side installation. This is only for testing purposes.

Due diligence

@Stypox
Copy link
Member

Stypox commented Mar 1, 2022

and then I migrated to AndroidX Room

I assume this is a typo and you meant AndroidX Work

Unfortunately this means yet another library, but it is what it is.

I think that's not a big issue, since AndroidX Work was going to be added anyway by #2335

@TacoTheDank
Copy link
Member Author

I assume this is a typo and you meant AndroidX Work

Damn, and I checked it over like three times too LOL, fixed

I think that's not a big issue, since AndroidX Work was going to be added anyway by #2335

👍

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thank you! Note: this should be tested well, otherwise we might encounter the same problems that forced us to do 2 hotfix releases in a row a while ago ;-)

app/src/main/java/org/schabi/newpipe/NewVersionWorker.kt Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/NewVersionWorker.kt Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/util/ReleaseUtil.kt Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/util/ReleaseUtil.kt Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/util/ReleaseUtil.kt Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/util/ReleaseUtil.kt Outdated Show resolved Hide resolved
app/src/main/java/org/schabi/newpipe/util/ReleaseUtil.kt Outdated Show resolved Hide resolved
@TacoTheDank TacoTheDank force-pushed the updateCheckerRewrite branch 2 times, most recently from eb44a86 to 2638d14 Compare March 2, 2022 15:51
@TacoTheDank TacoTheDank force-pushed the updateCheckerRewrite branch from 2638d14 to b271473 Compare March 3, 2022 18:31
@TacoTheDank TacoTheDank force-pushed the updateCheckerRewrite branch from b271473 to b8b97fa Compare March 3, 2022 18:34
@TacoTheDank
Copy link
Member Author

Updated the attached testing APK.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 3, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@TacoTheDank TacoTheDank added bug Issue is related to a bug codequality Improvements to the codebase to improve the code quality labels Mar 6, 2022
@TacoTheDank
Copy link
Member Author

Any updates on this?

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

I tested on API 19 and API 31 three different situations (last update check expired -> notification, last update check not expired -> nothing, manual update check -> notification) and I can confirm it works. Thank you and sorry for the delay :-)

@Stypox Stypox merged commit b607a09 into TeamNewPipe:dev Mar 15, 2022
@TacoTheDank TacoTheDank deleted the updateCheckerRewrite branch March 16, 2022 15:05
@Stypox Stypox mentioned this pull request Apr 16, 2022
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug codequality Improvements to the codebase to improve the code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CheckForNewAppVersion randomly fails (when app is in background)
2 participants