> 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
