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

Fix wrong thumbnail in notification on Android 13 #8899

Merged

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Aug 28, 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

This PR fixes yet another issue about the notification thumbnail not changing when going to the previous or next item in the player queue (see #8890 (comment)).

What I think the problem was (though this might need more investigation) is this:

  1. when tapping on "next", NewPipe's queue implementation would go to the next item
  2. then the ExoPlayer's internal media item queue gets synchronized, by telling the player to go to the next item
  3. the player internally switches to the next item, and it starts loading its stream details to be able to play it, also updating the item index in the media session (a component that the system can read to get info)
  4. the player switches state to onBuffering, which triggers a notification update for NewPipe
  5. notification updates are implemented not based on the selected queue item (currentItem) but rather on the StreamInfo (currentMetadata) and the currentThumbnail stored in the player. That StreamInfo, though, still belongs to the previous item in the queue, since stream details have not finished loading yet. This causes the wrong title, uploader and thumbnail to be temporarily set in the notification.
  6. the stream details finally load, they are set in the player and the notification is updated again, this time with the correct information
  7. title and uploader are correctly updated in the notification, but the thumbnail is not. This happens probably because the system associated the thumbnail of the previous item to the index of the next, and then tries to avoid reloading the thumbnail again to save cpu, assuming the first set thumbnail was the correct one. Triggering notification updates in other ways (e.g. play-pause) does not make a difference.

So in the PR that will merge NotificationUtil and NotificationPlayerUi, the notification, the player and the media session should be switched to using data not about the player StreamInfo, but rather the actual current play queue item (except for the thumbnail, for which we need the high-res one from the fully-loaded stream details). Though it might not be this simple.

This PR fixes the problem by detecting when the queue item changes and then setting currentThumbnail to null. This happens in-between points 1 and 2 above, i.e. before the exoPlayer internal queue is synced and before the notification update is triggered.

The commits strictly necessary to solve the issue are the first two, though the other two are also important in my opinion.

Fixes the following issue(s)

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

@Stypox
Copy link
Member Author

Stypox commented Aug 28, 2022

@karyogamy could you explain why the !isPlaying() is needed on this line? You initially introduced it in 1d136c6 (#810), then accidentally removed it in 4e459b3 and readded it in b81eb35 (#8020).

@Stypox Stypox force-pushed the fix-player-thumbnail-handling branch from 2572a8c to ed87465 Compare August 28, 2022 21:23
@sonarcloud
Copy link

sonarcloud bot commented Aug 28, 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 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Stypox Stypox mentioned this pull request Aug 28, 2022
9 tasks
@AudricV AudricV added bug Issue is related to a bug device/software specific Issues that only happen on some devices or with some specific hardware/software player notification Anything to do with the MediaStyle notification labels Sep 2, 2022
@opusforlife2
Copy link
Collaborator

Has the reddit reporter been asked to test this?

@AudricV
Copy link
Member

AudricV commented Sep 3, 2022

Yes, and it was claimed to be fixed by the user but their comments have been now deleted.

@Stypox Stypox merged commit ca29f6c into TeamNewPipe:release-0.24.0 Sep 19, 2022
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 device/software specific Issues that only happen on some devices or with some specific hardware/software player notification Anything to do with the MediaStyle notification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants