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

Fixed a bug that caused the background player to stop working #9706

Merged

Conversation

Jared234
Copy link
Contributor

@Jared234 Jared234 commented Jan 20, 2023

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

The bug was caused by an incorrect comparison between the existing PlaylistQueue and the new PlaylistQueue. In the handleIntent method in the Player class, both are compared, and if they are considered equal, the existing PlaylistQueue remains unchanged (because why reinitialize the same queue?). However, the overridden equality check was incorrect, as it only checks if the streams are the same, without considering the queueIndex. By inserting this check, the bug is fixed and only if the queues are actually identical, the existing queue is not reinitialized anymore.
I also had to change a test that tests if the equality check disregards the index. However, IMO if you do that the "equal" check doesn't make sense, since these queues cannot be considered equal.

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

@TobiGr TobiGr added bug Issue is related to a bug player Issues related to any player (main, popup and background) labels Jan 20, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jan 21, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 1 Bug
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

@Jared234
Copy link
Contributor Author

The "SonarCloud Quality Gate" failed. However, the "bug" it found cannot occur. If you take a look at the equalStreams method, you can see that it checks if other is null. Therefore, the getIndex() method can only be called on other in the equalStreamsAndIndex method if it is not null.
Is there any way to ignore these kinds of issues? Or should I implement an additional check?

@Jared234 Jared234 requested a review from Stypox January 21, 2023 14:14
Stypox
Stypox previously approved these changes Jan 28, 2023
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@Stypox Stypox changed the base branch from dev to release-0.25.0 January 28, 2023 20:55
@Stypox Stypox dismissed their stale review January 28, 2023 20:55

The base branch was changed.

@Stypox Stypox merged commit 1e724eb into TeamNewPipe:release-0.25.0 Jan 28, 2023
@Stypox Stypox mentioned this pull request Jan 28, 2023
3 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 Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Start playing in background after clicking background header in playlists bug
3 participants