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

[Matěj Laitl]

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



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

    Won't bee needed in next version of the patch.



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

    Oh, just please make the Reason a QString. (sorry for the wasted effort, I may have been too vague)



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

    Please change the documentation and implementation to:
    
    /**
     * Returns whether playableUrl() will return a playable Url.
     * In principle this means that the url is valid.
     *
     * Default implementation returns true if notPlayableReason() is
     * empty, false otherwise.
     */



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

    Please make notPlayableReason() a QString and ditch prettyNotPlayableReason(). I imagine following docstring (yes, please provide default implementation):
    
    /**
     * Return user-readable localized reason why isPlayeble() is false. Guaranteed to be empty if isPlayable() is true.
     *
     * Default implementation just returns an empty string.
     */
    
    Also please don't implement methods in header files in normal cases.



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

    Thinking about it more, the logic should be following:
    
    if( s_showToolTip )
        toolTipForTrack() // contains logic for showing not played notice if track isn't playable
    else if( !track->notPlayebleReason().isEmpty() )
        return i18n( "<b>Note:</b> the track is not playable.\n%1", notPlayableReason );  // i.e. just show the notice if s_showToolTip is false


- Matěj Laitl


On April 9, 2013, 1:40 p.m., Anmol Ahuja wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109817/
> -----------------------------------------------------------
> 
> (Updated April 9, 2013, 1:40 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/audiocd/AudioCdMeta.h fc26fcc 
>   src/core-impl/collections/audiocd/AudioCdMeta.cpp 218ce2a 
>   src/core-impl/collections/daap/DaapMeta.h 9a9c257 
>   src/core-impl/collections/daap/DaapMeta.cpp e66afb7 
>   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-impl/meta/timecode/TimecodeMeta.h 3aed4f9 
>   src/core-impl/meta/timecode/TimecodeMeta.cpp e3490e4 
>   src/core/meta/Meta.h f86cf39 
>   src/core/podcasts/PodcastMeta.h f3b21de 
>   src/playlist/PlaylistModel.cpp 246b9a1 
>   src/services/ServiceMetaBase.h f25159d 
>   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 
> 
> 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/20130412/966c62e2/attachment-0001.html>