Bug 193459

Summary: [Patch] Automatically scroll playlist broken in some cases
Product: [Applications] amarok Reporter: Jesús Vidal <jesusvpct>
Component: PlaylistAssignee: Amarok Developers <amarok-bugs-dist>
Status: RESOLVED FIXED    
Severity: normal CC: alexc223, anthony.vital, jeanpaul145, nhn, ruiz, teo, webmaster
Priority: NOR    
Version: 2.3-GIT   
Target Milestone: ---   
Platform: Ubuntu   
OS: Linux   
Latest Commit: Version Fixed In:
Attachments: EngineObserver.h patch
EngineObserver.cpp patch

Description Jesús Vidal 2009-05-21 08:35:42 UTC
Version:           2.0.90 (using KDE 4.2.3)
OS:                Linux
Installed from:    Ubuntu Packages

I have found two little bugs:

1º When you press the Next track button, the playlist scroll to next track and you see it, but if you use key shortcut (in my case Winkey + B), the list scroll to the previous track and not to the current.

2º If amarok is iconified, and you restore again, the playlist is on top

Thanks!
Comment 1 Seb Ruiz 2009-05-22 14:36:40 UTC
In the future please create one report per bug. I've left this open because the first bug is valid, but the second one is now (i don't understand it and it is most likely fixed).
Comment 2 Myriam Schweingruber 2009-07-14 09:47:32 UTC
*** Bug 195399 has been marked as a duplicate of this bug. ***
Comment 3 Myriam Schweingruber 2009-07-14 13:52:56 UTC
The second means if Amarok doesn't have focus and you come back, the playlist has scrolled back to the top. A better description is available in bug 199889, which is set as a duplicate to bug 193459.
There seems to be a general problem with the playlist scrolling though, maybe putting these bugs together?
Comment 4 Myriam Schweingruber 2009-07-15 21:32:10 UTC
*** Bug 199889 has been marked as a duplicate of this bug. ***
Comment 5 Anthony Vital 2009-08-22 02:16:32 UTC
I investigated the code a little bit, and I think I figured what's
wrong:
The EngineObservers are stored in a QSet, which is unsorted (and
unpredictable). So from time to time, when iterating through the
EngineObservers (when EngineObserver::engineNewTrackPlaying() is triggered for
instance), if Playlist::PrettyListView (where autoscroll is done) is coming
before Playlist::Actions, then the playlist is being centered before Amarok
moves on to the next track.
The problem is that there is no way to ensure a certain order in a QSet, so
maybe the solution would be to use something like QList instead?
I add a small patch replacing QSet by QList to illustrate.
Comment 6 Anthony Vital 2009-08-22 02:17:51 UTC
Created attachment 36345 [details]
EngineObserver.h patch
Comment 7 Anthony Vital 2009-08-22 02:18:16 UTC
Created attachment 36346 [details]
EngineObserver.cpp patch
Comment 8 Mikko C. 2009-08-23 13:27:15 UTC
*** Bug 201649 has been marked as a duplicate of this bug. ***
Comment 9 Teo Mrnjavac 2009-09-09 20:32:09 UTC
Oops
Sorry for taking so long, there seems to have been some mixup with bug assignement :-/
We'll check your patch ASAP
Comment 10 Teo Mrnjavac 2009-09-09 21:20:05 UTC
I can't reproduce this, neither 1) nor 2).
Can somebody else try please?
Comment 11 Nikolaj Hald Nielsen 2009-09-09 21:37:00 UTC
I cannot reproduce these issues with latest git master either. Closing it until someone can reproduce with current git.
Comment 12 Anthony Vital 2009-09-09 23:24:48 UTC
Well I can. To be clear, I'm talking about the first bug. If you look at the different duplicated reports, you can find more informations about it. 
And again, it doesn't happen all the time. If the bug happens, you can notice it on the first "next track" triggered in Amarok. If not, the bug won't bother you until you close Amarok. But chances are that the bug appears when you launch Amarok again.
Comment 13 Anthony Vital 2009-09-10 00:33:44 UTC
better: screencast showing the bug
http://dl.free.fr/rIC00XSil
Comment 14 Seb Ruiz 2009-09-10 02:51:35 UTC
I'm frequently reproducing the issue, but cannot gather a reliable case on how to reproduce.
Comment 15 Anthony Vital 2009-09-23 13:45:10 UTC
Any chance to have this reviewed for the 2.2 final release?
Comment 16 Teo Mrnjavac 2009-09-23 14:01:44 UTC
Sure, we'll take a look right now. Good thing you poked :)
Comment 17 Teo Mrnjavac 2009-09-23 22:03:22 UTC
I've tried reproducing this, but had no luck :-/
The patch seems sane, however I'd like someone who can reproduce test it before pushing.
Seb, since you managed to reproduce it, could you please take a look?
Comment 18 alexc223 2009-10-07 10:38:58 UTC
I've had this bug since the release of 2.1.0, it happens every time. For example, I've just started Amarok 2.2.0 up this morning, and it started to play a track (the last one that I played before I closed Amarok) however I wanted to skip it so I pressed the big 'Next' button.

This did change the track which is playing, however it did not scroll the playlist to the location of the new track. Now when ever I press next again, it does move but is obviously 'off by one' and shows the previous one.

I have all 1,321 songs in 1 playlist that it goes through, if that is of any relevance.
Comment 19 Mark Kretschmann 2009-10-07 12:33:34 UTC
What Anthony said makes sense. I will look at the problem.
Comment 20 Mark Kretschmann 2009-10-07 12:54:49 UTC
I've committed a fix to git master (will surface in 2.2.1). Thanks again Anthony!


commit a9a46b6ceab835c646b89d744bfcc5a7176ef5c1
Author: Mark Kretschmann <kretschmann@kde.org>
Date:   Wed Oct 7 12:50:25 2009 +0200

    Fix: Automatic playlist scrolling was broken in some cases.

    The actual bug was very very nasty. EngineObserver used QSet for storing
    observer pointers. This is what Qt docs say:
    "QSet is unordered, so an iterator's sequence cannot be assumed to be predictable."
    Now we're using QList, whose iteration order is predictable.

    Thanks to Anthony Vital for pointing this out.

    BUG: 193459
Comment 21 Mark Kretschmann 2009-10-09 19:54:37 UTC
The patch was unfortunately not correct. The real problem is in the playlist itself, it shouldn't be coupled to the ordering of Observers. Reverted.
Comment 22 Mark Kretschmann 2009-10-09 20:35:08 UTC
I've committed a better fix, please test:


commit c17fa232fe8c3bbf1fd1b80d74c573dd75d1b9a1
Author: Mark Kretschmann <kretschmann@kde.org>
Date:   Fri Oct 9 20:23:28 2009 +0200

    Proper fix for Automatic Scrolling in playlist. Please test.

    Moved the scrolling from Playlist::View to PlaylistActions.
    Thanks to Seb Ruiz for coming up with the idea.

    BUG: 193459
Comment 23 Anthony Vital 2009-10-10 01:50:57 UTC
This works fine here.
(Note: It seems that the previous patch hasn't been reverted in git master...)
Comment 24 Mark Kretschmann 2009-10-10 09:16:03 UTC
@Anthony: It has been reverted, see here:

http://gitorious.org/amarok/amarok/blobs/master/src/EngineObserver.h


PS: Thanks for testing :)
Comment 25 Anthony Vital 2009-10-12 18:44:39 UTC
(In reply to comment #24)
> @Anthony: It has been reverted, see here:
> 
> http://gitorious.org/amarok/amarok/blobs/master/src/EngineObserver.h
hmm... EngineObserver* are still stored in a QList here.
Comment 26 Mark Kretschmann 2009-10-12 18:51:03 UTC
Uhm. You're right. Something went wrong with the revert... I'll fix, thanks.