----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109817/#review31274 -----------------------------------------------------------
This looks very well, thanks. In the mean time I got more ideas how to improve the patch - sorry for not mentioning them earlier, I just didn't know them yet. :-) src/core-impl/collections/ipodcollection/IpodMeta.cpp <http://git.reviewboard.kde.org/r/109817/#comment23284> Hmm, I see that this code is also a candidate for deduplication. Perhaps you can introduce protected: QString localFileNotPlayableReason( const QString &path ) const; into Meta::Track? Meta::Track subclasses that handle local files could then do their own checks and then resort to localFileNotPlayableReason(), or call it directly if they have no specific checks. src/core-impl/collections/proxycollection/ProxyCollectionMeta.cpp <http://git.reviewboard.kde.org/r/109817/#comment23273> As a special case, you should check isPlayable() here and return empty string in that case, so that: isPlayable === empty reason still holds. src/core-impl/collections/proxycollection/ProxyCollectionMeta.cpp <http://git.reviewboard.kde.org/r/109817/#comment23272> 1. code style nitpick: spaces around "," 2. typographical rules says that comma should be followed by space - please join using ", " and not "," src/core-impl/collections/support/MemoryMeta.h <http://git.reviewboard.kde.org/r/109817/#comment23275> As a rather special case, MemoryMeta should relay *both* isPlayable() and notPlayableReason(). The reason is that you cannot guarantee than m_track will also implement isPlayable() by checking that notPlayableReason() is empty. src/core-impl/meta/multi/MultiTrack.h <http://git.reviewboard.kde.org/r/109817/#comment23277> As in MemoryMeta, you should still relay isPlayable() here. src/core-impl/meta/proxy/MetaProxy.h <http://git.reviewboard.kde.org/r/109817/#comment23279> As in MemoryMeta and MultiTrack, you should still relay isPlayable() src/core/meta/Meta.h <http://git.reviewboard.kde.org/r/109817/#comment23280> Please document this method, something like: /** * Helper method for subclasses to implement notPlayableReason(). Checks network status and returns a non-empty string if it isn't on-line. */ src/playlist/PlaylistModel.cpp <http://git.reviewboard.kde.org/r/109817/#comment23282> If this makes the tooltip too wide, perhaps the \n can be replaced by <br> to actually force the newline? (please test, this is just a guess) src/playlist/PlaylistModel.cpp <http://git.reviewboard.kde.org/r/109817/#comment23283> Perhaps the \n is ignored here because Qt detects it is a right text message, which needs to have <br> to denote newline? (please test, this is just a guess) - Matěj Laitl On April 18, 2013, 3:18 p.m., Anmol Ahuja wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109817/ > ----------------------------------------------------------- > > (Updated April 18, 2013, 3:18 p.m.) > > > Review request for Amarok. > > > Description > ------- > > 1. Added notPlayableReason() to class Track > 2. Display track tooltip with the prettyNotPlayableReason() if track not > playable > > > Diffs > ----- > > src/services/ServiceMetaBase.h f25159d > src/playlist/PlaylistModel.cpp 246b9a1 > src/core/podcasts/PodcastMeta.h f3b21de > src/core/meta/Meta.cpp cd1b38a > src/core/meta/Meta.h f86cf39 > src/core-impl/meta/timecode/TimecodeMeta.cpp e3490e4 > src/core-impl/meta/timecode/TimecodeMeta.h 3aed4f9 > src/core-impl/meta/stream/Stream.cpp 6e35960 > src/core-impl/meta/stream/Stream.h 3e19b45 > src/core-impl/meta/proxy/MetaProxy.cpp 6a35bc5 > src/core-impl/meta/proxy/MetaProxy.h 15967df > src/core-impl/meta/multi/MultiTrack.h be55170 > src/core-impl/meta/file/File.h 7d3359d > src/core-impl/meta/file/File.cpp 2cd0a61 > src/core-impl/collections/upnpcollection/UpnpMeta.cpp 00cc915 > src/core-impl/collections/upnpcollection/UpnpMeta.h 55bc131 > src/core-impl/collections/support/MemoryMeta.h 40061d2 > src/core-impl/collections/proxycollection/ProxyCollectionMeta.cpp e58a20a > src/core-impl/collections/proxycollection/ProxyCollectionMeta.h ef342a4 > src/core-impl/collections/playdarcollection/PlaydarMeta.cpp 5f4ec3c > src/core-impl/collections/playdarcollection/PlaydarMeta.h 7a39a4f > src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp 6d5bcf7 > src/core-impl/collections/nepomukcollection/meta/NepomukTrack.h 27ff06d > src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp 2b66574 > src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.h 5d1b9f9 > src/core-impl/collections/ipodcollection/IpodMeta.cpp 9ffcf7e > src/core-impl/collections/ipodcollection/IpodMeta.h 1d380b1 > src/core-impl/collections/db/sql/SqlMeta.cpp 19ec936 > src/core-impl/collections/db/sql/SqlMeta.h b7f0a71 > src/core-impl/collections/daap/DaapMeta.cpp e66afb7 > src/core-impl/collections/daap/DaapMeta.h 9a9c257 > src/core-impl/collections/audiocd/AudioCdMeta.cpp 218ce2a > src/core-impl/collections/audiocd/AudioCdMeta.h fc26fcc > src/services/ServiceMetaBase.cpp 7537649 > src/services/ampache/AmpacheMeta.h 934fe75 > src/services/ampache/AmpacheMeta.cpp b59715c > src/services/lastfm/meta/LastFmMeta.h 9c54a10 > src/services/lastfm/meta/LastFmMeta.cpp 424d136 > tests/CMakeLists.txt d6b23ec > tests/synchronization/CMakeLists.txt db73c25 > > Diff: http://git.reviewboard.kde.org/r/109817/diff/ > > > Testing > ------- > > Works as expected > > > Thanks, > > Anmol Ahuja > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
