----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110101/#review31662 -----------------------------------------------------------
Hi, thanks for the patch and sorry for delays reviewing. The general approach is good, however this seem to be so similar to last.fm code that it should be factored in a way for the code to be actually shared. Perhaps a common subclass or rather a helper class? I also have some smaller remarks. (I have not went through the whole patch because it will probably significantly change) src/services/magnatune/MagnatuneConfig.h <http://git.reviewboard.kde.org/r/110101/#comment23568> I'd prefer if you didn't change these lines, because: a) the changes are unrelated and would be better done in a separate patch (stupid reviewboard cannot handle multiple commits) b) we generally prefer to have the implementation in the .cpp file, even if it is trivial; so this change does actually against it. src/services/magnatune/MagnatuneConfig.h <http://git.reviewboard.kde.org/r/110101/#comment23569> please remove leading whitespace src/services/magnatune/MagnatuneConfig.h <http://git.reviewboard.kde.org/r/110101/#comment23570> Hmm, this seem to be a candidate for deduplication with LastFmServiceConfig - perhaps you could come up with a way so that these could share the common code? src/services/magnatune/MagnatuneConfig.h <http://git.reviewboard.kde.org/r/110101/#comment23571> I actually like the previous name more. src/services/magnatune/MagnatuneConfig.h <http://git.reviewboard.kde.org/r/110101/#comment23572> Ditto, the "Timestamp" actually provided more semantics. src/services/magnatune/MagnatuneConfig.h <http://git.reviewboard.kde.org/r/110101/#comment23573> please mind Amarok coding style. src/services/magnatune/MagnatuneConfig.cpp <http://git.reviewboard.kde.org/r/110101/#comment23574> such rather useless debugging without context is better removed. - Matěj Laitl On April 20, 2013, 2:14 p.m., Vedant Agarwala wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/110101/ > ----------------------------------------------------------- > > (Updated April 20, 2013, 2:14 p.m.) > > > Review request for Amarok. > > > Description > ------- > > Now Magnatue service uses KWallet for password storage rather than storing > passwords in plaintext in amarokrc. The patch is based on the LastFm service > settings (that uses KWallet) complete with a dialog to ask the user to store > password in plain text if KWallet does not exist/unavailable. > Contrary to before, now the save() method (of MegatuneConfig) runs > asynchronously so it may to be required to update other classes that call > methods of MegatuneConfig to connect to the updated() SIGNAL. > > > This addresses bug 242256. > https://bugs.kde.org/show_bug.cgi?id=242256 > > > Diffs > ----- > > src/services/magnatune/MagnatuneConfig.h 552bcf8 > src/services/magnatune/MagnatuneConfig.cpp 5842c63 > > Diff: http://git.reviewboard.kde.org/r/110101/diff/ > > > Testing > ------- > > Testing done, works. Builds, runs and passes the build tests. > > > File Attachments > ---------------- > > now megatune requests for KWallet > > http://git.reviewboard.kde.org/media/uploaded/files/2013/04/20/amarok_megatune_screenshot.png > > > Thanks, > > Vedant Agarwala > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
