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!
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).
*** Bug 195399 has been marked as a duplicate of this bug. ***
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?
*** Bug 199889 has been marked as a duplicate of this bug. ***
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.
Created attachment 36345 [details] EngineObserver.h patch
Created attachment 36346 [details] EngineObserver.cpp patch
*** Bug 201649 has been marked as a duplicate of this bug. ***
Oops Sorry for taking so long, there seems to have been some mixup with bug assignement :-/ We'll check your patch ASAP
I can't reproduce this, neither 1) nor 2). Can somebody else try please?
I cannot reproduce these issues with latest git master either. Closing it until someone can reproduce with current git.
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.
better: screencast showing the bug http://dl.free.fr/rIC00XSil
I'm frequently reproducing the issue, but cannot gather a reliable case on how to reproduce.
Any chance to have this reviewed for the 2.2 final release?
Sure, we'll take a look right now. Good thing you poked :)
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?
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.
What Anthony said makes sense. I will look at the problem.
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
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.
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
This works fine here. (Note: It seems that the previous patch hasn't been reverted in git master...)
@Anthony: It has been reverted, see here: http://gitorious.org/amarok/amarok/blobs/master/src/EngineObserver.h PS: Thanks for testing :)
(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.
Uhm. You're right. Something went wrong with the revert... I'll fix, thanks.