193459 2009-05-21 08:35:42 +0000 [Patch] Automatically scroll playlist broken in some cases 2009-12-09 11:34:25 +0000 1 1 2 Applications amarok Playlist 2.3-GIT Ubuntu Linux RESOLVED FIXED NOR normal --- 1 jesusvpct amarok-bugs-null alex anthony.vital jeanpaul145 nhn ruiz teo webmaster 20 oldest_to_newest 761289 0 jesusvpct 2009-05-21 08:35:42 +0000 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! 761995 1 ruiz 2009-05-22 14:36:40 +0000 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). 791882 2 myriam 2009-07-14 09:47:32 +0000 *** Bug 195399 has been marked as a duplicate of this bug. *** 792007 3 myriam 2009-07-14 13:52:56 +0000 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? 792997 4 myriam 2009-07-15 21:32:10 +0000 *** Bug 199889 has been marked as a duplicate of this bug. *** 816623 5 anthony.vital 2009-08-22 02:16:32 +0000 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. 816624 6 36345 anthony.vital 2009-08-22 02:17:51 +0000 Created attachment 36345 EngineObserver.h patch 816625 7 36346 anthony.vital 2009-08-22 02:18:16 +0000 Created attachment 36346 EngineObserver.cpp patch 817280 8 mikko.cal 2009-08-23 13:27:15 +0000 *** Bug 201649 has been marked as a duplicate of this bug. *** 828017 9 teo 2009-09-09 20:32:09 +0000 Oops Sorry for taking so long, there seems to have been some mixup with bug assignement :-/ We'll check your patch ASAP 828046 10 teo 2009-09-09 21:20:05 +0000 I can't reproduce this, neither 1) nor 2). Can somebody else try please? 828053 11 nhn 2009-09-09 21:37:00 +0000 I cannot reproduce these issues with latest git master either. Closing it until someone can reproduce with current git. 828110 12 anthony.vital 2009-09-09 23:24:48 +0000 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. 828130 13 anthony.vital 2009-09-10 00:33:44 +0000 better: screencast showing the bug http://dl.free.fr/rIC00XSil 828149 14 ruiz 2009-09-10 02:51:35 +0000 I'm frequently reproducing the issue, but cannot gather a reliable case on how to reproduce. 834631 15 anthony.vital 2009-09-23 13:45:10 +0000 Any chance to have this reviewed for the 2.2 final release? 834639 16 teo 2009-09-23 14:01:44 +0000 Sure, we'll take a look right now. Good thing you poked :) 834844 17 teo 2009-09-23 22:03:22 +0000 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? 841324 18 alex 2009-10-07 10:38:58 +0000 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. 841373 19 kretschmann 2009-10-07 12:33:34 +0000 What Anthony said makes sense. I will look at the problem. 841383 20 kretschmann 2009-10-07 12:54:49 +0000 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 842601 21 kretschmann 2009-10-09 19:54:37 +0000 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. 842615 22 kretschmann 2009-10-09 20:35:08 +0000 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 842713 23 anthony.vital 2009-10-10 01:50:57 +0000 This works fine here. (Note: It seems that the previous patch hasn't been reverted in git master...) 842752 24 kretschmann 2009-10-10 09:16:03 +0000 @Anthony: It has been reverted, see here: http://gitorious.org/amarok/amarok/blobs/master/src/EngineObserver.h PS: Thanks for testing :) 844094 25 anthony.vital 2009-10-12 18:44:39 +0000 (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. 844099 26 kretschmann 2009-10-12 18:51:03 +0000 Uhm. You're right. Something went wrong with the revert... I'll fix, thanks. 36345 2009-08-22 02:17:51 +0000 2009-08-22 02:17:51 +0000 EngineObserver.h patch patch_EngineObserver_h.diff text/plain 558 anthony.vital LS0tIEVuZ2luZU9ic2VydmVyLmh+CTIwMDktMDgtMjEgMTY6NDI6MzIuNTA1MTM5Mzk2ICswMjAw CisrKyBFbmdpbmVPYnNlcnZlci5oCTIwMDktMDgtMjEgMTY6NDI6MzIuNTYxMTQ1ODIwICswMjAw CkBAIC0yMyw3ICsyMyw2IEBACiAjaW5jbHVkZSA8UGhvbm9uL0dsb2JhbD4KICNpbmNsdWRlIDxR SGFzaD4KICNpbmNsdWRlIDxRUG9pbnRlcj4KLSNpbmNsdWRlIDxRU2V0PgogCiBjbGFzcyBFbmdp bmVTdWJqZWN0OwogY2xhc3MgUVN0cmluZzsKQEAgLTEyOSw3ICsxMjgsNyBAQCBwcml2YXRlOgog ICAgIGJvb2wgaXNNZXRhRGF0YVNwYW0oIFFIYXNoPHFpbnQ2NCwgUVN0cmluZz4gbmV3TWV0YURh dGEgKTsKIAogICAgIFFMaXN0PFFIYXNoPHFpbnQ2NCwgUVN0cmluZz4gPiBtX21ldGFEYXRhSGlz dG9yeTsKLSAgICBRU2V0PEVuZ2luZU9ic2VydmVyKj4gT2JzZXJ2ZXJzOworICAgIFFMaXN0PEVu Z2luZU9ic2VydmVyKj4gT2JzZXJ2ZXJzOwogICAgIFBob25vbjo6U3RhdGUgbV9yZWFsU3RhdGU7 IC8vIFRvIHdvcmsgYXJvdW5kIHRoZSBidWZmZXJpbmcgaXNzdWUKIH07CiAK 36346 2009-08-22 02:18:16 +0000 2009-08-22 02:18:16 +0000 EngineObserver.cpp patch patch_EngineObserver_cpp.diff text/plain 484 anthony.vital LS0tIEVuZ2luZU9ic2VydmVyLmNwcH4JMjAwOS0wOC0yMSAxNjo0Mjo1NS4wNjUxNDA4MDIgKzAy MDAKKysrIEVuZ2luZU9ic2VydmVyLmNwcAkyMDA5LTA4LTIxIDE2OjQyOjU1LjEyMTE0MzczNCAr MDIwMApAQCAtMjExLDEzICsyMTEsMTMgQEAgdm9pZCBFbmdpbmVTdWJqZWN0OjphdHRhY2goIEVu Z2luZU9ic2VydgogewogICAgIGlmKCAhb2JzZXJ2ZXIgKQogICAgICAgICByZXR1cm47Ci0gICAg T2JzZXJ2ZXJzLmluc2VydCggb2JzZXJ2ZXIgKTsKKyAgICBPYnNlcnZlcnMuYXBwZW5kKCBvYnNl cnZlciApOwogfQogCiAKIHZvaWQgRW5naW5lU3ViamVjdDo6ZGV0YWNoKCBFbmdpbmVPYnNlcnZl ciAqb2JzZXJ2ZXIgKQogewotICAgIE9ic2VydmVycy5yZW1vdmUoIG9ic2VydmVyICk7CisgICAg T2JzZXJ2ZXJzLnJlbW92ZUFsbCggb2JzZXJ2ZXIgKTsKIH0KIAogLyogVHJ5IHRvIGRldGVjdCBN ZXRhRGF0YSBzcGFtIGluIFN0cmVhbXMuICovCg==