Review Request 111655: Reimplement RecentlyPlayedListWidget

[Matěj Laitl]

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

Ship it!


Although I don't really know the old code and haven't tested this, I like this change! I propose merging it to 2.8 (unless a problem is found when testing this), there is a bug closed, the behaviour should be better than the old one and it doesn't break string freeze. Before merging please have a look at the minor issues below.


src/context/widgets/RecentlyPlayedListWidget.h
<http://git.reviewboard.kde.org/r/111655/#comment26858>

    I suggest adding explicit keyword as this could be called with one argument and misused as auto cast.



src/context/widgets/RecentlyPlayedListWidget.cpp
<http://git.reviewboard.kde.org/r/111655/#comment26868>

    Much better to use ::insertOptioned( const KUrl &url, ...) overload here because it calls CollectionManager::instance()->trackForUrl() asynchronously and handles other corner-cases.



src/context/widgets/RecentlyPlayedListWidget.cpp
<http://git.reviewboard.kde.org/r/111655/#comment26860>

    In an extremely unlikely case, this is not safe. track->artist() may become null between the first and second call (remember Meta::Track is thread-safe).
    
    We recommend to always use an extra temporary variable for all KSharedPtr-managed objects.



src/context/widgets/RecentlyPlayedListWidget.cpp
<http://git.reviewboard.kde.org/r/111655/#comment26861>

    This doesn't work for right-to-left languages and languages that don't use - as a separator etc, it needs to be made translatable. On the other hand, you don't want to introduce an extra i18n string if this should be merged into 2.8.
    
    The solution is to reuse (both context and text must be exactly the same)
    ScrobblerAdapter.cpp:267:    QString trackName = i18nc( "%1 is artist, %2 is title", "%1 - %2", ...



src/context/widgets/RecentlyPlayedListWidget.cpp
<http://git.reviewboard.kde.org/r/111655/#comment26866>

    It is much better to use the uidUrl() here. For example IpodCollection uses iPod serial name there, which is more "unique" than just file path. Additionally, playableUrl() might not be valid until prepareToPlay() is called and for example future MTP collection will return a temporary file name here.
    
    With uidUrl(), you have much better chance of Playlist::Controller::insertOptioned() will find the "original" track (from the right collection).


- Matěj Laitl


On July 23, 2013, 12:36 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, 12:36 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Reimplement RecentlyPlayedListWidget
> 
> 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 
> 
> 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/37b50512/attachment-0001.html>