Review Request: KWallet for Magnatune credentials

[Ralf Engels]

-----------------------------------------------------------
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 <kde at dontletsstart.com>                                
>  * Copyright (c) 2009 Leo Franchi <lfranchi at 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
> 
>

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