Review Request 116763: Use KWallet to store scrobbling credentials.

[Michael Pyne]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116763/#review53007
-----------------------------------------------------------


Good first pass at an implementation, but still some work to do.


scrobbleconfigdlg.h
<https://git.reviewboard.kde.org/r/116763/#comment37311>

    We would need to add a destructor that deletes the m_wallet pointer.
    
    Alternately, you can change the type here to be of a wrapper class that does this for you, such as QScopedPointer.



scrobbleconfigdlg.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37298>

    This call can fail, so we should check for that.



scrobbleconfigdlg.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37299>

    Likewise this call can fail. For readability purposes it's probably possible to make this a multi-stage set of conditionals. Something like
    
    if (m_wallet) {
     // Setup folder, set folder, with all checks
     // or delete m_wallet and set it to 0.
    }
    if (m_wallet) {
     // Wallet must be open, do stuff with it
    }
    else {
     // No wallet or it doesn't work...
    }



scrobbleconfigdlg.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37301>

    This message should be clarified that Last.fm credentials will be stored without encryption, so that there's no confusion about what credentials are insecure, and in case the user doesn't know why "plain text" is bad.



scrobbleconfigdlg.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37304>

    writeMap can fail, so you'd at least want to output to a kError() or show a message box if that happens here so the user knows not to get rid of their written password information yet. ;)



scrobbler.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37305>

    Same issues here with error checking.
    
    You may want to consider splitting out this KWallet-setup code into a common function in scrobbler.h that both scrobbler.cpp and scrobblerconfigdlg.cpp can use to obtain a valid Wallet* already opened up to JuK's folder.



scrobbler.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37310>

    Should add a "delete m_wallet" call here.



scrobbler.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37309>

    You should wrap the check for a wallet around a call to KWallet::folderDoesNotExist.
    
    This can check if JuK even has credentials stored in KWallet at all, and without decrypting the wallet.
    
    This would be a big deal here because as it stands your code would bring up a password dialog (if the wallet had a password) just to start playing music, even if the user didn't have a last.fm account.



scrobbler.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37308>

    You forgot to delete the returned Wallet here, which is a memory leak.
    
    You should probably hold the Wallet* in a QScopedPointer<Wallet::KWallet>, as it will automatically delete the Wallet.



scrobbler.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37306>

    Likewise here, 3 times this common code has been used. "Don't Repeat Yourself" would seem to start to apply here.



scrobbler.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37312>

    This .append() function isn't available if QT_NO_CAST_TO_ASCII is defined (which is common, as it's a good error-catching tool).
    
    What you really want is something like sessionKey.toUtf8() or sessionKey.toLatin1() (which might be quicker) to convert from QString to QByteArray. That way you don't even need a temp QByteArray to store into either.



scrobbler.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37313>

    This has to be a QString anyways for the QMap<> below.



scrobbler.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37314>

    So you can use a temp QByteArray to read into here, and then do "sessionKey = QString::fromLatin1(tempByteArray);" or similar.



scrobbler.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37315>

    And then this would revert to config.readEntry(...)



scrobbler.cpp
<https://git.reviewboard.kde.org/r/116763/#comment37316>

    Same issue here, this needs to be QString, you'll have to fit your calls to the Wallet around that like above.


- Michael Pyne


On March 12, 2014, 1:37 p.m., Arnold Dumas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116763/
> -----------------------------------------------------------
> 
> (Updated March 12, 2014, 1:37 p.m.)
> 
> 
> Review request for KDE Multimedia, Michael Pyne and Martin Tobias Holmedahl Sandsmark.
> 
> 
> Repository: juk
> 
> 
> Description
> -------
> 
> Use KWallet to store scrobbling credentials. If the KWallet is not available, or not allowed by the user, the old KConfigGroup code will be used. In the meanwhile, a dialog saying that the credentials will be saved in plain text will be shown.
> 
> 
> Diffs
> -----
> 
>   scrobbleconfigdlg.h 6674488 
>   scrobbleconfigdlg.cpp 7bcf19d 
>   scrobbler.h 5027459 
>   scrobbler.cpp 85da2e9 
> 
> Diff: https://git.reviewboard.kde.org/r/116763/diff/
> 
> 
> Testing
> -------
> 
> Manual testing on my laptop that uses KWallet and on my desktop that doesn't have KWallet. It seems to work in both cases.
> 
> 
> Thanks,
> 
> Arnold Dumas
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-multimedia/attachments/20140315/21f2638a/attachment.htm>
-------------- next part --------------
_______________________________________________
kde-multimedia mailing list
kde-multimedia at kde.org
https://mail.kde.org/mailman/listinfo/kde-multimedia