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

[Dan Meltzer]

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


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.


src/core/collections/QueryMaker.h
<http://git.reviewboard.kde.org/r/109283/#comment21340>

    No need to reformat this in this patch.  It just makes it more confusing what's going on.



src/core/collections/QueryMaker.h
<http://git.reviewboard.kde.org/r/109283/#comment21341>

    Same here. Don't change comment style just to change comment style.  This patch is for something different.



src/services/lastfm/LastFmServiceSettings.cpp
<http://git.reviewboard.kde.org/r/109283/#comment21342>

    Use label.isEmpty().  Also, why would it ever = 0?



src/services/lastfm/ScrobblerAdapter.cpp
<http://git.reviewboard.kde.org/r/109283/#comment21345>

    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? 



src/services/lastfm/ScrobblerAdapter.cpp
<http://git.reviewboard.kde.org/r/109283/#comment21343>

    Again.  m_config->skipLabel().isEmpty().  It's a QString, it should never = 0.



src/statsyncing/ScrobblingService.h
<http://git.reviewboard.kde.org/r/109283/#comment21344>

    Is this really a Scrobbling Error?  Seems like a hack to me


- Dan Meltzer


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/e9c8d5f0/attachment-0001.html>