Review Request 110101: fixes Bug 242256 - JJ: Make Magnatue service use KWallet for password storage

[Vedant Agarwala]

> On April 27, 2013, 9:27 a.m., Matěj Laitl wrote:
> > 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)

I agree. I will upload another patch making changes to both last.fm and magnatune code. I was thinking of a common helper class complete with the "no KWallet found dialog". Tell me any other specifics that should be a part of this KWalletHelper class.

And I will fix the other issues.


> On April 27, 2013, 9:27 a.m., Matěj Laitl wrote:
> > src/services/magnatune/MagnatuneConfig.h, lines 58-89
> > <http://git.reviewboard.kde.org/r/110101/diff/2/?file=140104#file140104line58>
> >
> >     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.

I will be making another review request reverting this change. But shouldn't the last.fm code be changed in a similar way? Should there be another review request for such a change?


- Vedant


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


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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130427/3a9502d5/attachment-0001.html>