Review Request 109283: Bug 140198 - JJ: feature request: option to selectively disable submitting certain tracks/albums from collection to last.fm

[Matěj Laitl]

> On March 5, 2013, 1:41 a.m., Dan Meltzer wrote:
> > Hi.  All sorts of things going on here.  My biggest question is that it seems as if you are designing this to only allow one label to be set as the "banned" label.  Why not a list of labels?  Also, keep your patch on topic.  Formatting changes aren't a bad thing, but they make it hard to see what you are actually changing.

> My biggest question is that it seems as if you are designing this to only allow one label to be set as the "banned" label.  Why not a list of labels?

I don't think that multiple labels are necessary (and that's how the suggestion how the implement it on the bug was formulated). Intended use case is that user creates a label extra for that (e.g. "noscrobble") and assigns it to all tracks. (mass-assigning labels works fine in Amarok, and you can filter by existing labels)

[the things below are just replies to comments above, will do a review too]


> On March 5, 2013, 1:41 a.m., Dan Meltzer wrote:
> > src/core/collections/QueryMaker.h, line 35
> > <http://git.reviewboard.kde.org/r/109283/diff/1/?file=116979#file116979line35>
> >
> >     No need to reformat this in this patch.  It just makes it more confusing what's going on.

Right, moreover this change goes against Amarok coding style (we don't alight values to be in the same column).


> On March 5, 2013, 1:41 a.m., Dan Meltzer wrote:
> > src/services/lastfm/ScrobblerAdapter.cpp, line 280
> > <http://git.reviewboard.kde.org/r/109283/diff/1/?file=116985#file116985line280>
> >
> >     Is it your intention to only allow one label to be skipped for scrobbling?  If so, why?  Shouldn't it be a list of banned labels that we are comparing against?
> 
> Vedant Agarwala wrote:
>     I used the QComboBox as per Matej's comment (#21) in https://bugs.kde.org/show_bug.cgi?id=140198
>     If needed I can easily change it to a QListWidget, that will resolve this issue.

Yes, this was the intention.


- Matěj


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


On March 4, 2013, 7:31 p.m., Vedant Agarwala wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109283/
> -----------------------------------------------------------
> 
> (Updated March 4, 2013, 7:31 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Added a check-box to disable scobbling for tracks with a particular label. The label can be selected from a drop-down list or entered manually. Any track that contains this label is skipped from being submitted to last.fm for being scrobbled.
> 
> 
> Diffs
> -----
> 
>   src/core/collections/QueryMaker.h a30b94f 
>   src/services/lastfm/LastFmConfigWidget.ui 5c5e51b 
>   src/services/lastfm/LastFmServiceConfig.h 3f33b72 
>   src/services/lastfm/LastFmServiceSettings.h 41b2ead 
>   src/services/lastfm/LastFmServiceSettings.cpp f6e1564 
>   src/services/lastfm/ScrobblerAdapter.h ea74196 
>   src/services/lastfm/ScrobblerAdapter.cpp b1a09f8 
>   src/statsyncing/Process.cpp c42fdc4 
>   src/statsyncing/ScrobblingService.h 971edd7 
> 
> Diff: http://git.reviewboard.kde.org/r/109283/diff/
> 
> 
> Testing
> -------
> 
> Builds and runs successfully.
> 
> 
> Thanks,
> 
> Vedant Agarwala
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130305/bbe14bf3/attachment.html>