Review Request 110426: KWalletHelper class for services using the KWallet

[Stefan Derkits]

> On Aug. 26, 2013, 7:07 p.m., Vedant Agarwala wrote:
> > Fixed the problems and incorporated Matej's patch.
> > 
> > Builds, passes tests and runs now. I didn't test it fully. I ran Amarok with this patch and at Amarok start-up I got the KWallet Dialog for Amarok ("Do you want to allow Amarok to access KWallet?"). I selected "Allow always" and Amarok is working fine. Some more testing with all the cases (missing KWallet, denying usage of KWallet, and working previously save config) might be a good idea.

Less code dupliation sounds good ... it would be a good idea to maybe change from using pure KWallet to using the QtKeychain (library for Mac OS Keychain, Gnome Keyring, KWallet and secure password Storage on Windows). While this is probably not the scope of this patch, now could be a good time to do this change (if the other devs agree). Also in the gpodder service you forgot in the constructor of GpodderServiceConfig m_enableProvider( false ) ... please add this again (explicitly initializing it in the constructor makes things more clear)


- Stefan


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


On Aug. 26, 2013, 7:03 p.m., Vedant Agarwala wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110426/
> -----------------------------------------------------------
> 
> (Updated Aug. 26, 2013, 7:03 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> I have created a KWalletHelper class so that services like Maganatune, Last.fm and GPodder can use this rather than duplicating code.
> Currently the patch applies only to Magnatune. The KWalletHelper class complies but it doesn't link properly to the MagnatuneConfig class.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt d7d11f9 
>   src/services/KWalletHelper.h PRE-CREATION 
>   src/services/KWalletHelper.cpp PRE-CREATION 
>   src/services/gpodder/GpodderServiceConfig.h 90447fd 
>   src/services/gpodder/GpodderServiceConfig.cpp 9098d59 
>   src/services/gpodder/GpodderServiceSettings.cpp 34280c7 
>   src/services/lastfm/CMakeLists.txt a895bba 
>   src/services/lastfm/LastFmServiceConfig.h 4b1552e 
>   src/services/lastfm/LastFmServiceConfig.cpp 9912b22 
>   src/services/magnatune/CMakeLists.txt 91f24c0 
>   src/services/magnatune/MagnatuneConfig.h 552bcf8 
>   src/services/magnatune/MagnatuneConfig.cpp 5842c63 
>   src/services/magnatune/MagnatuneDownloadHandler.h b257440 
>   src/services/magnatune/MagnatuneDownloadHandler.cpp 3bce597 
>   src/services/magnatune/MagnatuneInfoParser.h 7904b67 
>   src/services/magnatune/MagnatuneInfoParser.cpp f10ad13 
>   src/services/magnatune/MagnatuneNeedUpdateWidget.cpp 044cf4b 
>   src/services/magnatune/MagnatuneRedownloadHandler.cpp 99c1a54 
>   src/services/magnatune/MagnatuneSettingsModule.h 4728a34 
>   src/services/magnatune/MagnatuneSettingsModule.cpp d45938f 
>   src/services/magnatune/MagnatuneStore.h c143d59 
>   src/services/magnatune/MagnatuneStore.cpp 2863c5b 
> 
> Diff: http://git.reviewboard.kde.org/r/110426/diff/
> 
> 
> Testing
> -------
> 
> The KWalletHelper.cpp complies but fails to link to ManatuneConfig.cpp. Output of "make" command: http://paste.kde.org/743792/
> 
> 
> Thanks,
> 
> Vedant Agarwala
> 
>

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