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

Remove no longer needed compat methods #5581

Merged
merged 1 commit into from Dec 14, 2021

Conversation

TacoTheDank
Copy link
Contributor

@TacoTheDank TacoTheDank commented Dec 4, 2021

With the recent bump of the minimum supported OS version from 16 to 19 (which makes me pretty happy), there are quite a few compat methods we no longer need to use, as the internal checks they do are for versions below 19.

Some side notes:

  • AppCompatResources.getDrawable calls AppCompatDrawableManager.get().getDrawable internally, and is what's supposed to be used anyway.
  • CompareCompat.compareLong's code was (or the comments were) incorrect for some reason. The greater than and less than symbols were flipped and didn't match up with the method's comments, causing the actual behavior to be equal to Long.compare(long2, long1) instead of Long.compare(long1, long2) like the comments say. I can only assume that was a mistake by the original contributor. Should I also flip these, then, since that was the behavior of the removed method?

@ByteHamster
Copy link
Member

AppCompatResources.getDrawable calls AppCompatDrawableManager.get().getDrawable internally, and is what's supposed to be used anyway.

Do you have an official source for that? The function does not look like we are supposed to use it directly and this SO post says it's not part of the public API.
https://stackoverflow.com/a/40242172/4193263

@TacoTheDank
Copy link
Contributor Author

The post is saying that AppCompatDrawableManager.get().getDrawable(Context, int) isn't part of the public API, not AppCompatDrawable.getDrawable (which does not exist, which leads me to believe they meant AppCompatResources.getDrawable).

Anyway, AppCompatDrawableManager.get().getDrawable(Context, int) is not part of the public API.

Furthermore, AppCompatResources.getDrawable calls ContextCompat.getDrawable internally.
https://developer.android.com/reference/androidx/appcompat/content/res/AppCompatResources#getDrawable(android.content.Context,%20int)

@ByteHamster
Copy link
Member

causing the actual behavior to be equal to Long.compare(long2, long1) instead of Long.compare(long1, long2) like the comments say. I can only assume that was a mistake by the original contributor. Should I also flip these, then, since that was the behavior of the removed method?

Yeah, I think that would be good. The behavior should not be changed. The change currently breaks the statistics screens.

The post is saying that AppCompatDrawableManager.get().getDrawable(Context, int) isn't part of the public API, not AppCompatDrawable.getDrawable

Oh, right. Sorry. I somehow mixed up the additions vs deletions and thought you are doing the exact opposite. Your change is obviously what we want/should do 👍

@TacoTheDank
Copy link
Contributor Author

TacoTheDank commented Dec 11, 2021

No worries lol. I'll get to flipping the compares when I can :)

@TacoTheDank
Copy link
Contributor Author

@ByteHamster Done 👍

@ByteHamster ByteHamster merged commit 20e8b3e into AntennaPod:develop Dec 14, 2021
@ByteHamster
Copy link
Member

Thanks!

@TacoTheDank TacoTheDank deleted the remove-old-compat branch December 14, 2021 20:27
@antennapod-bot
Copy link

This pull request has been mentioned on AntennaPod Forum. There might be relevant details there:

https://forum.antennapod.org/t/antennapod-2-5-release-notes/1636/1

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.

None yet

3 participants