Review Request: Implements the wishlist item 185397 - "Saved playlist default name could be smarter"

[Sergey Ivanov]

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


You got me wrong. I meant you should check albums and artists for existence, not for empty name. Because ArtistPtr/AlbumPtr can point to nowhere in case of NULL artist/album in DB.


src/playlistmanager/sql/SqlUserPlaylistProvider.cpp
<http://git.reviewboard.kde.org/r/100168/#comment298>

    It should be like:
    QString artist = tracks.first()->artist() ? tracks.first()->artist()->prettyName() : QString();
    here



src/playlistmanager/sql/SqlUserPlaylistProvider.cpp
<http://git.reviewboard.kde.org/r/100168/#comment299>

    And
    if( !track->artist() || artist != track->artist()->prettyName() )
    here.


It can be improved a bit by using album artist in case of singleAlbum.
And since all Artist/Album pointers are shared, you don't need to compare Its names, you can compare pointers. It will reduce overhead from strings comparisons and you won't need to check existence of artists/albums 'till you try to get Its names in 325-332 lines.

- Sergey


On 2010-11-25 01:01:23, Dennis Francis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100168/
> -----------------------------------------------------------
> 
> (Updated 2010-11-25 01:01:23)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> -------
> 
> The playlist name is generated as per the rules :
> 
>     if ( singleArtist && singleAlbum )
>         playlistName = artist + " - " + album + ' - ' + shortDate
> 
>     else if ( !singleArtist && singleAlbum )
>         playlistName = "Various" + " - " + album + ' - ' + shortDate
> 
>     else if ( singleArtist && !singleAlbum )
>         playlistName = artist + " - " + "Various" + shortDate
> 
>     else
>         playlistName = "Various" + " - " + shortDate
> 
> If the there are no tracks, the playlist name is set to "Empty Playlist - " + shortDate
> 
> 
> This addresses bug 185397.
>     https://bugs.kde.org/show_bug.cgi?id=185397
> 
> 
> Diffs
> -----
> 
>   src/playlistmanager/sql/SqlUserPlaylistProvider.h 3a5a62e 
>   src/playlistmanager/sql/SqlUserPlaylistProvider.cpp d2ae5b9 
> 
> Diff: http://git.reviewboard.kde.org/r/100168/diff
> 
> 
> Testing
> -------
> 
> Tested different possibilities. It is working fine for me.
> 
> Please suggest better alternatives ( if any ) to the shortDate addition. Without the shortDate, playlist database lookup would be necessary to 
> calculate a unique name.
> 
> 
> Thanks,
> 
> Dennis
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20101125/ea0a5e29/attachment.htm