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

[Anmol Ahuja]

> On April 4, 2013, 4:07 a.m., Matěj Laitl wrote:
> > src/playlist/PlaylistModel.cpp, lines 361-365
> > <http://git.reviewboard.kde.org/r/109817/diff/1/?file=127441#file127441line361>
> >
> >     This is way too specific and doesn't fit to abstraction Meta::Track provides. Note that here is Track::isPlayable()
> >     
> >     You should also show the notice *along* the original tooltip, as for example tracks from Local Collection can show metadata even thought the file ceased to be readable. (so that the changes fit more into tooltipFor() method)
> >     
> >     Also note that the notice is track-specific, MetaFile::Track may tell "File doesn't exist", while MetaStream::Track may want to tell "Network connection not available".
> >     
> >     Maybe it has sense to intoduce virutal Meta::Track::notPlayableReason(), but that would require acknowledge from other developers.
> 
> Anmol Ahuja wrote:
>     But the problem is specific to only local files.
>     And I'm not checking Track::isPlayable(), just whether it is a local file, and if we have read permissions on it if it is a local file.
> 
> Matěj Laitl wrote:
>     > But the problem is specific to only local files.
>     
>     Developers are free and encouraged to think about and question often narrow user problem reports whether they aren't in fact much broader. Bug https://bugs.kde.org/show_bug.cgi?id=313649 is a very good example of it.
>     
>     In other words, the patch as-is qualifies as ad-hoc hack that simply isn't acceptable (not to mention that Track::playableUrl() may not be valid until ::prepareToPlay()). Please fix the broader problem I outlined above, respecting Amarok abstraction layers.
>     
>     > And I'm not checking Track::isPlayable(), just whether it is a local file, and if we have read permissions on it if it is a local file.
>     
>     I can read code, no need to repeat what it does.

Ohk. Sorry.


- Anmol


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


On April 2, 2013, 2:50 a.m., Anmol Ahuja wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109817/
> -----------------------------------------------------------
> 
> (Updated April 2, 2013, 2:50 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Added two indicators for no-read permission:
> 1. A tooltip "No read permissions" for tracks with no read permissions
> 2. Upon right-clicking such a track, displays a "No read permissions" disabled action.
> 
> 
> Diffs
> -----
> 
>   src/playlist/PlaylistModel.cpp 246b9a1 
>   src/playlist/view/PlaylistViewCommon.h 9582b1b 
>   src/playlist/view/PlaylistViewCommon.cpp e3cdd53 
> 
> 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/20130404/65badce9/attachment.html>