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

[Matěj Laitl]

> On April 3, 2013, 10:37 p.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.

> 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.


- Matěj


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


On April 1, 2013, 9:20 p.m., Anmol Ahuja wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109817/
> -----------------------------------------------------------
> 
> (Updated April 1, 2013, 9:20 p.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/ade02749/attachment-0001.html>