-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Clean up proguard file #8468
Clean up proguard file #8468
Conversation
@@ -39,12 +35,11 @@ | |||
} | |||
-keepnames class * { @icepick.State *;} | |||
|
|||
# Rules for OkHttp. Copy paste from https://github.com/square/okhttp | |||
## Rules for OkHttp. Copy paste from https://github.com/square/okhttp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the idea behind the double hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I just wanted to section it off from these next lines so it's more clear that they're not related to OkHttp.
-keepclassmembers class * implements java.io.Serializable {
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better if you just leave the #
and a a line above
-keepclassmembers class * implements java.io.Serializable
that says something like # keep required serialized stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't intending to add any note to that, I just wanted to section off the OKHttp stuff from the rest of the file. I guess I could add a note for that though, if you want.
@TacoTheDank in order to test these changes, the only thing I have to do is checking that the app builds fine? Or should I also test things at runtime? |
333548c
to
24cf197
Compare
Kudos, SonarCloud Quality Gate passed! |
@Stypox You can do both, like what I did with AntennaPod/AntennaPod#5903, which includes decompiling and diffing the release APKs. I tested it myself on my devices. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there does seem to be a difference, 57576 vs 57561 methods
Before | After |
---|---|
![]() |
![]() |
But as it can be seen in the images, it's just a difference in "prettytime", and as you said our rule was outdated anyway. This saves ~1KB :-D
Anyway, then this looks good, since everything builds normally and the app runs normally (prettytime works).
What is it?
Description of the changes in your PR
I went down the git blame hole and found that PrettyTime was first added using 4.0.1. Meanwhile, consumer proguard rules were added to PrettyTime in 4.0.3. Therefore we no longer need this rule (and also ours is now outdated anyway).
-keep class org.ocpsoft.prettytime.i18n.** { *; }
These are from before AndroidX. I don't think I need to explain these :)
The OkHttp rules are embedded into the library as of 3.11.0. OkHttp was first added using 3.9.1. Therefore, these specific ones are now obsolete and safe to remove.
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