Review Request 110082: fixes Bug 275821 - JJ: Proper tooltips for Saved Playlists; remove Playlist::description() method

[Matěj Laitl]

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


This looks pretty well, but please have a look at some potential problems related to database updating.


src/playlistmanager/sql/SqlPlaylist.cpp
<http://git.reviewboard.kde.org/r/110082/#comment24371>

    Please stip debugging lines used for development before submitting patches.



src/playlistmanager/sql/SqlUserPlaylistProvider.h
<http://git.reviewboard.kde.org/r/110082/#comment24372>

    Nitpick: add one more extra leading space on this 2 lines.



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

    1. I think you can drop support for the "unreleased versions" - this was in pre-Amarok 2.0 time.
    
    In the default:, you should issue a warning message for the user instead:
    "Version %1 of playlist database schema encountered, however this Amarok version only supports version %2 (and previous versions starting with 2). Playlists saved in Amarok Database won't probably work and any write operations with them may result in loosing them. Perhaps you've started an older version of Amarok with a database written by newer version?"
    
    This also means that the deleteTables() method is subject for removal.
    
    2. I don't think you update USERPLAYLIST_DB_VERSION in the database when updating from version 2 to 3, which is na error. The best place for this update would be just before the "case 3:" line, with a comment to keep it just before the current version case switch.
    
    3. Nitpick: we usually add a space between \\ and comment text.


- Matěj Laitl


On May 4, 2013, 4:18 p.m., Vedant Agarwala wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110082/
> -----------------------------------------------------------
> 
> (Updated May 4, 2013, 4:18 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> As agreed on the review for https://git.reviewboard.kde.org/r/104048/ , Qt::TooltipRole has been updated so that now the tooltip displays full name of the playlist. Occurrences of "description" have been removed (from the Playlist base class as well as the subclasses).
> 
> 
> This addresses bug 275821.
>     https://bugs.kde.org/show_bug.cgi?id=275821
> 
> 
> Diffs
> -----
> 
>   src/browsers/playlistbrowser/PlaylistBrowserModel.cpp 6ee3db3 
>   src/core-impl/collections/mediadevicecollection/playlist/MediaDevicePlaylist.h 9b94872 
>   src/core-impl/collections/mediadevicecollection/playlist/MediaDevicePlaylist.cpp 3510481 
>   src/core-impl/playlists/types/file/PlaylistFile.h de71057 
>   src/core-impl/playlists/types/file/PlaylistFile.cpp 073e140 
>   src/core/playlists/Playlist.h 9fa854f 
>   src/playlistmanager/SyncedPlaylist.h 214bb5c 
>   src/playlistmanager/SyncedPlaylist.cpp ae6f9ab 
>   src/playlistmanager/sql/SqlPlaylist.h d28d161 
>   src/playlistmanager/sql/SqlPlaylist.cpp 2d6ef61 
>   src/playlistmanager/sql/SqlPlaylistGroup.cpp 2862034 
>   src/playlistmanager/sql/SqlUserPlaylistProvider.h 2ff9fda 
>   src/playlistmanager/sql/SqlUserPlaylistProvider.cpp 0ffb3d6 
> 
> Diff: http://git.reviewboard.kde.org/r/110082/diff/
> 
> 
> Testing
> -------
> 
> Testing done. Works. Builds successfully and passes the tests.
> 
> 
> File Attachments
> ----------------
> 
> displays the new tooltip
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/04/19/amarok_screenshot1.png
> displays the new tooltip
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/04/19/amarok_screenshot.png
> 
> 
> Thanks,
> 
> Vedant Agarwala
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130520/dcbde871/attachment-0001.html>