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

[Matěj Laitl]

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

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