Bug 224003

Summary:	Amarok playlist should select next song before updating current
Product:	[Applications] amarok	Reporter:	BK <bk+bugzilla>
Component:	Playlist	Assignee:	Amarok Developers <amarok-bugs-dist>
Status:	RESOLVED NOT A BUG
Severity:	normal	CC:	langstr, nhn, ognyan_angelov, sven.burmeister, teo
Priority:	NOR
Version First Reported In:	2.3.2
Target Milestone:	2.4.0
Platform:	Fedora RPMs
OS:	Linux
Latest Commit:		Version Fixed In:
Sentry Crash Report:

Description BK 2010-01-24 07:59:57 UTC

Version:           2.2.2 (using KDE 4.3.4)
OS:                Linux
Installed from:    Fedora RPMs

When playing songs in a playlist, Amarok will, on finishing the current song, first update score/play count etc, then select the next song in the playlist. This is broken. Amarok should first pick the next song in the playlist, _then_ update the current song.

Consider the following playlist:

Name     Score
Song A      10
Song B       5
Song C      20

In this case, the playlist will work as expected, playing the songs in the order A, B, C.

Now lets sort the playlist on increasing score:

Name     Score
Song B       5
Song A      10
Song C      20

Now, Amarok will play song B, then update the score of song B, which moves it to the end of the playlist. Amarok then selects the next song in the playlist, but since song B now is at the end of the playlist, Amarok will stop.

For this specific case, the workaround is to enable Repeat playlist, but this does not generalize. Just add song D with a score of, say, 80:

Name     Score
Song B       5
Song A      10
Song C      20
Song D      80

After playing song B, it will move to the second to last position, then song D will play (defeating the intended play order). Assuming Repeat playlist is set, then song A will play, after which it will be moved to the second to last position, then song D will play again, then song C, then song D once more.

Comment 1 Mikko C. 2010-01-24 09:32:29 UTC

this does look like a bug, but I haven't tried reproducing it

Comment 2 BK 2010-03-17 05:11:57 UTC

Confirmed bug still present in version 2.3.

Still renders sorting of playlists on any automatically updated stat (e.g., score & last played) useless. Presumably does the same thing to manually updated stats, such as rating, title etc, but since that's the result of manual intervention, and not something that happens automatically, I figure that's OK.

Comment 3 Myriam Schweingruber 2010-03-17 09:02:58 UTC

Can somebody please try to reproduce this?

Comment 4 langstr 2010-03-21 01:01:11 UTC

I think BUG 229926 and BUG 230191 are duplicates of this bug.

Comment 5 langstr 2010-03-21 01:05:30 UTC

I suspect that most of this can be fixed as follows:

  - Move that one finishedPlaying() call out of PlaylistActions: handle it all in one place (EngineController). My unmerged pile already contains a patch for that.


  - EngineController and PlaylistActions have a somewhat rigid bi-directional coupling: Enginecontroller has hard-coded calls to 'The::playlistActions()->requestNextTrack()', which does a hard-coded call back into 'The::engineController()->play()'.

    I think 'The::playlistActions()->requestNextTrack()' should do the same thing as the track navigators function with that name: return a value, not do a 'play()' call.

    So the EngineController code would go from:
        if ( xyz )
            The::playlistActions()->requestNextTrack();

    to something like:
        void EngineController::setNextTrack( Meta::TrackPtr ... ) { ... }
        ...
        if ( xyz )
            setNextTrack( The::playlistActions()->requestNextTrack() );


  - And finally the specific fix for these bugs: in EngineController::slotAboutToFinish(), change the code from:
        m_currentTrack->finishedPlaying();
        The::playlistActions()->requestNextTrack();

to something like this:
        nextTrack = The::playlistActions()->requestNextTrack();
        m_currentTrack->finishedPlaying();
        setNextTrack( The::playlistActions()->requestNextTrack() );


--
However, EngineController really isn't code I know well. This is not something that I'm planning to tackle alone.

Comment 6 Sven Krohlas 2010-03-21 01:18:59 UTC

*** Bug 229926 has been marked as a duplicate of this bug. ***

Comment 7 Sven Krohlas 2010-03-21 01:19:20 UTC

*** Bug 230191 has been marked as a duplicate of this bug. ***

Comment 8 langstr 2010-03-21 01:49:22 UTC

Oops, copy-paste error, that last code should read:

        nextTrack = The::playlistActions()->requestNextTrack();
        m_currentTrack->finishedPlaying();
        setNextTrack( nextTrack );

(sorry if this is duplicated; bugs.kde.org is giving database timeout errors)

Comment 9 Myriam Schweingruber 2010-05-22 12:27:38 UTC

Changing target.

Comment 10 Ognyan Angelov 2010-12-06 21:56:40 UTC

Setup:
  Amarok 2.3.2
  KDE 4.4.5
Test Result:
  Same thing here. I have to refresh the playlist to show me the correct positions of the songs...

Comment 11 Myriam Schweingruber 2010-12-06 22:07:02 UTC

Bump version, confirmed by tester.

Comment 12 Myriam Schweingruber 2011-06-04 12:10:19 UTC

This is an automated message from the triager:

Amarok 2.4.1 has been released on May 8 already. Could you please upgrade and test if you can still reproduce this bug?

Without feedback within a month we will close this bug as resolved.

Thank you for your understanding.

Comment 13 Myriam Schweingruber 2011-07-15 09:04:04 UTC

Closing for lack of feedback. Feel free to reopen if you can still reproduce this with Amarok 2.4.2 beta 1 or later and provide the necessary feedback.