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

Tue Mar 5 12:48:23 UTC 2013

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

Thanks for the patch, Vedant! Generally this looks good, there are just a few minor problems usual for newcomers, please resolve these before this can be merged.

It would be nice if you attached a screenshot of the new Last.fm config dialog.

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

    Right. Vedant, you're welcome to open another review request with just formatting cleanups - I like them! (git should make it easy to split the patch into 2 commits)

src/services/lastfm/LastFmConfigWidget.ui
<http://git.reviewboard.kde.org/r/109283/#comment21359>

    I don't think you should mess with the groupBox sizepolicy, it seems to behave well currently.

src/services/lastfm/LastFmConfigWidget.ui
<http://git.reviewboard.kde.org/r/109283/#comment21370>

    rename per comment in LastFmServiceConfig.h

src/services/lastfm/LastFmConfigWidget.ui
<http://git.reviewboard.kde.org/r/109283/#comment21360>

    No need to override default focuspolicy.

src/services/lastfm/LastFmConfigWidget.ui
<http://git.reviewboard.kde.org/r/109283/#comment21361>

    You obvisouly shouldn't just copy technical description right from the bug, instead invent something more user-friendly.

src/services/lastfm/LastFmConfigWidget.ui
<http://git.reviewboard.kde.org/r/109283/#comment21371>

    This is not a list, so I'd suggest kcfg_FilteredLabel name.

src/services/lastfm/LastFmConfigWidget.ui
<http://git.reviewboard.kde.org/r/109283/#comment21362>

    You shouldn't need to override default values, try pressing the small "revert to default" button in Qt Designer

src/services/lastfm/LastFmServiceConfig.h
<http://git.reviewboard.kde.org/r/109283/#comment21363>

    a) read intro_and_style.txt for correct include file order
    b) you don't use this include at all!

src/services/lastfm/LastFmServiceConfig.h
<http://git.reviewboard.kde.org/r/109283/#comment21364>

    Please rename (m_)selectiveScrobble to something more descriptive, perhaps (m_)filterByLabel

src/services/lastfm/LastFmServiceConfig.h
<http://git.reviewboard.kde.org/r/109283/#comment21365>

    ditto renaming, perhaps (m_)filteredLabel

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

    I propose renaming this to filteredLabelComboIndex() to me even clearer.

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

    Forgot to fill in docs? Also, the name looks strange, perhaps a typo? I suggert setupLabelsQueryMaker() alternatively you may consider factoring this short function directly into the constructor, it is not used anywhere else.

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

    Actually inside SIGNAL/SLOT macros we break Amarok coding style to use normalized signatures, see http://marcmutz.wordpress.com/effective-qt/prefer-to-use-normalised-signalslot-signatures/

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

    You need to call query->setAutoDelete( true ); to prevent memory leak here

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

    ditto SIGNAL/SLOT signature normalization.

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

    nitpick: code style: "& label" vs. " &label"

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

    Right. Vedant, note difference between QString and ordinary strings, I propose reading official QString documentation.

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

    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)

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

    nitpick: wil print '(...) label " labelname " which'

    Notice the spaces. Just get rid of the quotes and note that operator << adds a space on its own.

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

    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.

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

    "const Meta::LabelPtr &label" would be even better

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

    Adding new SkippedByUser enum value is the right way to implement this.

- Matěj Laitl

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