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, 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)
> 
> Vedant Agarwala wrote:
>     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.

To quote the docs:
> duplicatesEnabled : bool
> This property holds whether the user can enter duplicate items into the combobox.
> 
> Note that it is always possible to programmatically insert duplicate items into the combobox.

And here you indeed insert them programatically.


> 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.
> 
> Vedant Agarwala wrote:
>     Right. But I think we have to check for both because someone might check the check box but leave the combo box empty.

Checking for skipLabel().isEmpty() won't hurt, but just note the code would work even without as you'd compare track labels against empty label which would be always false.


- Matěj


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