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

Keep strong references to Picasso notification icon loading targets #8677

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Jul 22, 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

Before the Target would sometimes be garbage collected before being called with the loaded channel icon, since Picasso holds weak references to targets. This meant that sometimes a new streams notification would not be shown, because the lambda that should have shown it had already been garbage collected.

I don't know if there was a way to properly reproduce something wrong with the previous behavior, but Picasso clearly states in RequestCreator#into(Target):

This method keeps a weak reference to the Target instance and will be garbage collected if you do not keep a strong reference to it.

Anyway, I tested the new code by subscribing to the usual Roel Van De Paar and it works.

Note that this is also one of the causes of a missing thumbnail in the notification, that will be fixed with #8678.

Fixes the following issue(s)

None that I could find

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

@AudricV AudricV added the bug Issue is related to a bug label Jul 22, 2022
Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

Code LGTM

Is it somehow possible to test this?

@Stypox
Copy link
Member Author

Stypox commented Jul 26, 2022

Yes, just subscribe to Roel Van De Paar and enable its notifications, wait for a few minutes (until he uploads a new video, or find a channel which uploads more frequently), then manually trigger the streams notification update from debug settings.

@triallax triallax added the player notification Anything to do with the MediaStyle notification label Jul 26, 2022
Before the Target would sometimes be garbage collected before being called with the loaded channel icon, since Picasso holds weak references to targets. This meant that sometimes a new streams notification  would not be shown, because the lambda that should have shown it had already been garbage collected.
@sonarcloud
Copy link

sonarcloud bot commented Aug 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

LGTM, quick test also successful

@litetex litetex merged commit ce6f3ca into TeamNewPipe:dev Aug 25, 2022
@Stypox Stypox mentioned this pull request Aug 27, 2022
9 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 player notification Anything to do with the MediaStyle notification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants