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]Stop playback when next from last item in current playlist #15582

Merged
merged 1 commit into from Feb 28, 2019

Conversation

Projects
None yet
4 participants
@DaveTBlake
Copy link
Member

commented Feb 22, 2019

When user does "next" during playback of the last item of the current playlist then playback should stop. This was not happening, play continues and yet the item is undefined so no metadata or art is displayed on GUI or returned to JSON requests about the currently playing item. Once in this state "prev" does not goto the previous item in the playlist, instead play of that item restarts.

@ronie thanks for reporting here https://forum.kodi.tv/showthread.php?tid=341168&pid=2825962#pid2825962

image

This is a regression introduced in the major app changes of March 2018. Previously when the app processed GUI_MSG_PLAYLISTPLAYER_STOPPED messages it told the player to stop too, but app control of player and message sequences have changed significantly.

In many situations the player has stopped before GUI_MSG_PLAYLISTPLAYER_STOPPED is sent, so the best fix IMO is to have the playlistplayer request the player stop in the two situations when it is the playlistplayer processing that want to abort playback. That is either

  • when user does next from last item in list,
    or
  • when in repeat one mode and single song is invalid.

This also avoids changes in CApplication message handling which is desirable since it is considered "fragile" by other team members.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2019

LINUX-RBPI build failure unrelated to this PR

@ronie can you test this or do you need me to kick off a test build?

@fritsch

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

As discussed on slack. This is wrong as it adds a circular dependency from PlayListPlayer to Application. It furthermore hardcodes behaviour in the player itself.

As you send the message GUI_MSG_PLAYLISTPLAYER_STOPPED to the user of your Player, let the user, here application, handle it.

diff --git a/xbmc/Application.cpp b/xbmc/Application.cpp
index a8ff2ceb10..3dfb729dd6 100644
--- a/xbmc/Application.cpp
+++ b/xbmc/Application.cpp
@@ -3896,6 +3896,8 @@ bool CApplication::OnMessage(CGUIMessage& message)
   case GUI_MSG_PLAYLISTPLAYER_STOPPED:
     m_itemCurrentFile->Reset();
     CServiceBroker::GetGUI()->GetInfoManager().ResetCurrentItem();
+    if (m_appPlayer.IsPlaying())
+      m_appPlayer.StopPlaying();
     PlaybackCleanup();
     return true;

Should be enough as the user (I repeat: Application in that case) can care about behaviour and change it, e.g. handle messages like it wants.

Stop playing current file when playlist player has stopped.
Playlistplayer sends a GUI_MSG_PLAYLISTPLAYER_STOPPED message when aborting playback when next from last item in list, or repeat one and song is invalid. This ensures that the player stops too.

@DaveTBlake DaveTBlake force-pushed the DaveTBlake:PlayStopNoNext branch from aaf78d8 to bb090d3 Feb 22, 2019

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2019

@fritsch, @a1rwulf if you prefer CApplication to change and stop playback rather than CPlaylistPlayer then fine. I asked and got a clear answer.

@DaveTBlake

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2019

Weird - jenkins red light despite all platforms being green.

Both @Rechi and @a1rwulf 👍 the design that @fritsch suggested and I have implemented, so good to merge?

@ronie

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

@ronie can you test this or do you need me to kick off a test build?

tested the current patch and can confirm it fixes the issue.
thx! :-)

@a1rwulf a1rwulf self-requested a review Feb 23, 2019

@a1rwulf

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

Can't find the approve button on the phone...
Yeah design wise it's good now.

@DaveTBlake DaveTBlake merged commit 9430144 into xbmc:master Feb 28, 2019

1 check passed

default You're awesome. Have a cookie
Details

@DaveTBlake DaveTBlake deleted the DaveTBlake:PlayStopNoNext branch Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.