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

[unified player] Swiping away the last stream in the queue does not close player #4129

Closed
Stypox opened this issue Aug 15, 2020 · 1 comment · Fixed by #4130
Closed

[unified player] Swiping away the last stream in the queue does not close player #4129

Stypox opened this issue Aug 15, 2020 · 1 comment · Fixed by #4130
Labels
bug Issue is related to a bug player Issues related to any player (main, popup and background)

Comments

@Stypox
Copy link
Member

Stypox commented Aug 15, 2020

Version

  • Apk in the regression kanban: d2e6700 (2020-08-04 13:18 UTC)

Steps to reproduce the bug

  1. Play a (single) stream in the background
  2. Open the background play queue through the notification
  3. Swipe away the stream

Expected behavior

The player should stop and close

Actual behaviour

The stream keeps playing as if nothing happened, controls are still working, but there is no stream in the play queue.

Analysis

When the only stream playing is removed, player.onPlaybackShutdown() is called. But that function does absolutely nothing, and there is even a comment asserting that it should do nothing:

    @Override
    public void onPlaybackShutdown() {
        if (DEBUG) {
            Log.d(TAG, "onPlaybackShutdown() called");
        }
        // Override it because we don't want playerImpl destroyed
    }

@avently why is this the case? Shouldn't that function be removed alltoghether if it does nothing? Anyway, onPlaybackShutdown() is used in the situation discussed above and when an unrecoverable error occours. Is it safe to have that function to what it ought to, i.e. shutting down everything?

@Stypox Stypox added bug Issue is related to a bug player Issues related to any player (main, popup and background) labels Aug 15, 2020
@avently
Copy link
Contributor

avently commented Aug 15, 2020

why is this the case?

I added this blank override after I noticed that when errors occurs player view get destroyed (i.e. it becomes null) so I just wanted to make the view alive until service gets destroyed without worrying about NullPointerExceptions.

Is it safe to have that function to what it ought to, i.e. shutting down everything?

I think, yes. If the service will be shutted down too then everything will be fine

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 a pull request may close this issue.

2 participants