-----------------------------------------------------------
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
> 
>

_______________________________________________
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel

Reply via email to