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

[Vedant Agarwala]

> On March 5, 2013, 12:48 p.m., Matěj Laitl wrote:
> > src/services/lastfm/LastFmServiceSettings.cpp, lines 255-256
> > <http://git.reviewboard.kde.org/r/109283/diff/1/?file=116983#file116983line255>
> >
> >     I think this will duplicate the label in case it is already in the combo box. (but do test it instead of fixing it blindly)

By default duplicates are disbaled in QComboBox. It needs to be enabled by setDuplicatesEnabled. That is why I didn't check if the label already exists in the QComboBox.


> On March 5, 2013, 12:48 p.m., Matěj Laitl wrote:
> > src/services/lastfm/ScrobblerAdapter.cpp, line 283
> > <http://git.reviewboard.kde.org/r/109283/diff/1/?file=116985#file116985line283>
> >
> >     In fact, there's no need to check whether skipLabel() is empty. Instead you MUST check whether selectiveScrobbling (raname pending) is true. Also document that this method does the check in its docstring.

Right. But I think we have to check for both because someone might check the check box but leave the combo box empty.


> On March 5, 2013, 12:48 p.m., Matěj Laitl wrote:
> > src/statsyncing/ScrobblingService.h, line 54
> > <http://git.reviewboard.kde.org/r/109283/diff/1/?file=116987#file116987line54>
> >
> >     Adding new SkippedByUser enum value is the right way to implement this.

Thank-you. This was my biggest concern about the patch. :)


- Vedant


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


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