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

Use DrawableCompat. #4592

Merged
merged 1 commit into from
Oct 25, 2020
Merged

Conversation

Isira-Seneviratne
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne commented Oct 22, 2020

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

  • Use DrawableCompat to tint the retrieved notification icon in NotificationSettingsFragment.

APK testing

debug.zip

Due diligence

@Stypox
Copy link
Member

Stypox commented Oct 25, 2020

I remember doing this differently in order to prevent doing more actions than needed on >=Lollipop (e.g. calling wrap() and mutate() on the drawable). But if you think that doesn't improve anything in terms of performance this can be merged

@Stypox
Copy link
Member

Stypox commented Oct 25, 2020

Oh, according to this SO just calling setTint was bad, so yeah, this is the best implementation. Thank you! Could you rebase? Then I'll merge

@Isira-Seneviratne
Copy link
Member Author

I remember doing this differently in order to prevent doing more actions than needed on >=Lollipop (e.g. calling wrap() and mutate() on the drawable). But if you think that doesn't improve anything in terms of performance this can be merged

There are issues with some types of drawables on Lollipop.

@Stypox
Copy link
Member

Stypox commented Oct 25, 2020

@TobiGr could you merge? I can't

@TobiGr TobiGr merged commit a86f8e9 into TeamNewPipe:dev Oct 25, 2020
@Isira-Seneviratne Isira-Seneviratne deleted the Use_DrawableCompat branch October 25, 2020 23:33
This was referenced Nov 10, 2020
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.

3 participants