----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111655/#review36412 -----------------------------------------------------------
Ship it! Looks nice to me. I unfortunately don't have time to actually test it. Perhaps markey can? Then please push. src/context/widgets/RecentlyPlayedListWidget.cpp <http://git.reviewboard.kde.org/r/111655/#comment26889> Hmm, const-ref -> by-value change doesn't seem really needed, but this is sooo unimportant, feel free to leave it as it it. src/context/widgets/RecentlyPlayedListWidget.cpp <http://git.reviewboard.kde.org/r/111655/#comment26888> Yeah, this is sometimes needed, neither I like it but it seems like a fair price to pay for the fwd-declaration goodies. src/playlist/PlaylistController.cpp <http://git.reviewboard.kde.org/r/111655/#comment26887> Nice catch! Please commit this separately right away. - Matěj Laitl On July 23, 2013, 9:21 p.m., Konrad Zemek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/111655/ > ----------------------------------------------------------- > > (Updated July 23, 2013, 9:21 p.m.) > > > Review request for Amarok. > > > Description > ------- > > Reimplement RecentlyPlayedListWidget > > Also fix an issue where certain overloads of > Playlist::Controller::insertOptioned couldn't automatically start > playback when given StartPlay option. > > Rewrote RecentlyPlayedListWidget from the basics. There was a major > inconsistency in this widget, where tracks were added to it if they were > recently played at all, but the time shown was the "Last Played" > statistics which is only updated after whole song is played. This > caused gravely incorrect data to be displayed (see bug 302485). > > There were two different methods of solving the inconsistency: focusing > on the "Last Played" time, and adding songs to the widget only if the > "Last Played" changed; and focusing on recent plays completely > disregarding "Last Played". I opted for the latter as I felt it fits the > widget's nature better. > > Because we can't rely on already available data, the widget needs to do > its own housekeeping. It saves its data on shutdown, to be restored > on next startup. One other major benefit of this approach is that > widget's data remains correct even if collections change, while > previously tracks from removed collections would disappear. > > Finally, I added the feature to add tracks to playlist directly from the > widget, provided that the track exists. Adding to playlist works > consistently with other parts of Amarok, with standard double-click and > middle-click behavior. > > BUG: 302485 > FEATURE: 296090 > FEATURE: 279263 > REVIEW: 111655 > > > Diffs > ----- > > src/context/widgets/RecentlyPlayedListWidget.h > caa78039b891511ca36ada8f61cd4f776d1f3c6d > src/context/widgets/RecentlyPlayedListWidget.cpp > 6ea501affcf6e61d237002e15f7ed4e26989b91b > src/playlist/PlaylistController.cpp > 60c99e5a10459b84b9839f07aad0d1e2bfa5d259 > > Diff: http://git.reviewboard.kde.org/r/111655/diff/ > > > Testing > ------- > > Manual. > > > Thanks, > > Konrad Zemek > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
