----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104480/#review17537 -----------------------------------------------------------
Several small issues, but in principle I support this patch. Can you have a look at the things and maybe improve the patch. src/services/magnatune/MagnatuneConfig.h <http://git.reviewboard.kde.org/r/104480/#comment13737> Trailing space. You can see them in review board syntax highlighting. Nobody likes them :) src/services/magnatune/MagnatuneConfig.h <http://git.reviewboard.kde.org/r/104480/#comment13736> I am wondering: two more characters and m_askDiag would be a readable m_askDialog Really worth saving these two chars? src/services/magnatune/MagnatuneConfig.cpp <http://git.reviewboard.kde.org/r/104480/#comment13739> The dialogs are modale (shown with exec). You need deleteLater in cases where you might need the pointer later for something (e.g. because in a slot I am deleting myself but somebody else might be interested in the signal) So here "delete m_askDiag" would be sufficient. src/services/magnatune/MagnatuneConfig.cpp <http://git.reviewboard.kde.org/r/104480/#comment13740> Use amaroks debug() please. src/services/magnatune/MagnatuneConfig.cpp <http://git.reviewboard.kde.org/r/104480/#comment13741> Amarok coding convention. No space between if and ( src/services/magnatune/MagnatuneConfig.cpp <http://git.reviewboard.kde.org/r/104480/#comment13743> Do we really need to have a object variable to store the dialog? Modal dialogs are usually stored on the stack in Qt, especially when it's just asking a question. But you could also just do a "show" and allow the user to do other stuff before answering the question about the wallet. src/services/magnatune/MagnatuneConfig.cpp <http://git.reviewboard.kde.org/r/104480/#comment13742> And here you even added the space. - Ralf Engels On April 3, 2012, 8:04 p.m., Andrzej Hunt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104480/ > ----------------------------------------------------------- > > (Updated April 3, 2012, 8:04 p.m.) > > > Review request for Amarok. > > > Description > ------- > > Modifies the Magnatune service to use KWallet for storage (instead of > plaintext). > > The code has been copied from LastFmServiceConfig.cpp, with almost no > modification on my part. (KDevelop removed some whitespace and reformatted > some other bits of code meaning there are some formatting changes in the diff > -- I apologise if this is a problem for reviewing, and I can revert the > formatting changes if desired.) > > A question about copyright attribution: Would this be the correct thing to > add to MagnatuneConfig.cpp below the current copyright line? > > * Code copied from ../lastfm/LastFmServiceConfig.cpp: > * Copyright (c) 2007 Shane King <k...@dontletsstart.com> > > * Copyright (c) 2009 Leo Franchi <lfran...@kde.org> > > > This addresses bug 242256. > https://bugs.kde.org/show_bug.cgi?id=242256 > > > Diffs > ----- > > src/services/magnatune/MagnatuneConfig.cpp 18ee898 > src/services/magnatune/MagnatuneConfig.h f1d25eb > > Diff: http://git.reviewboard.kde.org/r/104480/diff/ > > > Testing > ------- > > > Thanks, > > Andrzej Hunt > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
