Review Request 111655: Reimplement RecentlyPlayedListWidget

[Konrad Zemek]

> On July 23, 2013, 7:40 p.m., Matěj Laitl wrote:
> > src/context/widgets/RecentlyPlayedListWidget.cpp, line 218
> > <http://git.reviewboard.kde.org/r/111655/diff/1-2/?file=173012#file173012line218>
> >
> >     Hmm, const-ref -> by-value change doesn't seem really needed, but this is sooo unimportant, feel free to leave it as it it.

You're very much right here. I misremembered something from GoingNative 2012 conference about danger of passing shared pointers by const reference. I'm going to change it back, as it also gets rid of the .moc file include.


- Konrad


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111655/#review36412
-----------------------------------------------------------


On July 23, 2013, 7: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, 7: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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130723/3dd1e083/attachment.html>