Review Request 109817: JJ 313649 - No warning if there are no permissions to read file

Sun Apr 14 12:30:32 UTC 2013

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

Thanks for the patch, it looks better now. I just realised that once a subclass implements notPlayableReason(), it doesn't have to implement isPlayable() anymore (except for 1 or 2 special proxy track subclasses). In future, I'd like to make isPlayable() a convenience method that just returns notPlayableReason().isEmpty(). Another observation is that the "network" notPlayableReason() is used at many places and is a candidate for deduplication. Please use implementation + my remarks from AmpacheTrack::notPlayableReason() to create the shared implementation. (see the last code comment)

src/core-impl/collections/daap/DaapMeta.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23037>

    Please remove this now redundant method - the base implementation + custom notPlayableReason() will suffice now.

src/core-impl/collections/db/sql/SqlMeta.h
<http://git.reviewboard.kde.org/r/109817/#comment23035>

    Unless subclass does something special, we don't usually document reimplemented methods. But I see you've just followed the pattern. Feel free to remove redundant comment above.

src/core-impl/collections/db/sql/SqlMeta.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23038>

    Please remove the now-redundant method.

src/core-impl/collections/ipodcollection/IpodMeta.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23039>

    Please remove redundant method.

src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23040>

    Ditto please remove redundant.

src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23041>

    KLocalizedString is a more appropriate include

src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23042>

    Ditto remove redundant

src/core-impl/collections/playdarcollection/PlaydarMeta.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23043>

    ditto :-)

src/core-impl/collections/proxycollection/ProxyCollectionMeta.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23045>

    This will crash if m_tracks is an empty list.

    Perhaps you can collect the reasons to a QStringList inside the foreach above and then QStringList::join( ", " ) them?

src/core-impl/collections/upnpcollection/UpnpMeta.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23046>

    ditto please remove redundant

src/core-impl/meta/file/File.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23047>

    ditto

src/core-impl/meta/stream/Stream.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23048>

    ditto please remove redundant

src/core/meta/Meta.h
<http://git.reviewboard.kde.org/r/109817/#comment23033>

    1. code-style nitpick: please align the docstrings like this:

    /**
     * Bla Bla bla
     */

    (i.e. one extra space starting with the second line)

    2. please break lines at 90 characters. 

    3. Add an empty line between 2 documented methods.

    4. Even though the implementation is trivial, please do it in the .cpp file - it looks better.

src/playlist/PlaylistModel.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23050>

    I think you should use a HTML snippet compatible with HTMLLine() (i.e. wrapped inside <tr><td colspan=2>Actual text</td></tr>)

src/playlist/PlaylistModel.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23049>

    You were right, !track->isPlayable() is better here

src/services/ampache/AmpacheMeta.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23051>

    Ditto please remove redundant method.

src/services/ampache/AmpacheMeta.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23052>

    Compiler will actually emit a "warning: unhandled value in switch" here. It is best to add all other known values (like above) and explicitly return an empty string for there, so we get notified by the compiler in case a new enum value was added.

src/services/lastfm/meta/LastFmMeta.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23054>

    ditto remove redundant method

src/services/lastfm/meta/LastFmMeta.cpp
<http://git.reviewboard.kde.org/r/109817/#comment23053>

    At this point I wonder whether code like this should be factored out to a common place to avoid code duplication. Yes, it should be.

    Possible places are:
    core/meta/support/MetaUtility.h,cpp: QString networkNotPlayableReason();
    or protected QString Meta::Track::networkNotPlayableReason() const;

- Matěj Laitl

On April 13, 2013, 1:38 p.m., Anmol Ahuja wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109817/
> -----------------------------------------------------------
> 
> (Updated April 13, 2013, 1:38 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> 1. Added notPlayableReason() and prettyNotPlayableReason() to class Track
> 2. Display track tooltip with the prettyNotPlayableReason() if track not playable
> 
> 
> Diffs
> -----
> 
>   src/core-impl/collections/daap/DaapMeta.cpp e66afb7 
>   src/core-impl/collections/daap/DaapMeta.h 9a9c257 
>   src/core-impl/collections/db/sql/SqlMeta.h b7f0a71 
>   src/core-impl/collections/db/sql/SqlMeta.cpp 19ec936 
>   src/core-impl/collections/ipodcollection/IpodMeta.h 1d380b1 
>   src/core-impl/collections/ipodcollection/IpodMeta.cpp 9ffcf7e 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.h 5d1b9f9 
>   src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp 2b66574 
>   src/core-impl/collections/nepomukcollection/meta/NepomukTrack.h 27ff06d 
>   src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp 6d5bcf7 
>   src/core-impl/collections/playdarcollection/PlaydarMeta.h 7a39a4f 
>   src/core-impl/collections/playdarcollection/PlaydarMeta.cpp 5f4ec3c 
>   src/core-impl/collections/proxycollection/ProxyCollectionMeta.h ef342a4 
>   src/core-impl/collections/proxycollection/ProxyCollectionMeta.cpp e58a20a 
>   src/core-impl/collections/support/MemoryMeta.h 40061d2 
>   src/core-impl/collections/upnpcollection/UpnpMeta.h 55bc131 
>   src/core-impl/collections/upnpcollection/UpnpMeta.cpp 00cc915 
>   src/core-impl/meta/file/File.h 7d3359d 
>   src/core-impl/meta/file/File.cpp 2cd0a61 
>   src/core-impl/meta/multi/MultiTrack.h be55170 
>   src/core-impl/meta/proxy/MetaProxy.h 15967df 
>   src/core-impl/meta/proxy/MetaProxy.cpp 6a35bc5 
>   src/core-impl/meta/stream/Stream.h 3e19b45 
>   src/core-impl/meta/stream/Stream.cpp 6e35960 
>   src/core/meta/Meta.h f86cf39 
>   src/playlist/PlaylistModel.cpp 246b9a1 
>   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 
> 
> Diff: http://git.reviewboard.kde.org/r/109817/diff/
> 
> 
> Testing
> -------
> 
> Works as expected
> 
> 
> Thanks,
> 
> Anmol Ahuja
> 
>

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