----------------------------------------------------------- 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
