<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/110426/">http://git.reviewboard.kde.org/r/110426/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 26th, 2013, 7:07 p.m. UTC, <b>Vedant Agarwala</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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)</pre>
<br />
<p>- Stefan</p>
<br />
<p>On August 26th, 2013, 7:03 p.m. UTC, Vedant Agarwala wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok.</div>
<div>By Vedant Agarwala.</div>
<p style="color: grey;"><i>Updated Aug. 26, 2013, 7:03 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The KWalletHelper.cpp complies but fails to link to ManatuneConfig.cpp. Output of "make" command: http://paste.kde.org/743792/</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/CMakeLists.txt <span style="color: grey">(d7d11f9)</span></li>
<li>src/services/KWalletHelper.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/services/KWalletHelper.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/services/gpodder/GpodderServiceConfig.h <span style="color: grey">(90447fd)</span></li>
<li>src/services/gpodder/GpodderServiceConfig.cpp <span style="color: grey">(9098d59)</span></li>
<li>src/services/gpodder/GpodderServiceSettings.cpp <span style="color: grey">(34280c7)</span></li>
<li>src/services/lastfm/CMakeLists.txt <span style="color: grey">(a895bba)</span></li>
<li>src/services/lastfm/LastFmServiceConfig.h <span style="color: grey">(4b1552e)</span></li>
<li>src/services/lastfm/LastFmServiceConfig.cpp <span style="color: grey">(9912b22)</span></li>
<li>src/services/magnatune/CMakeLists.txt <span style="color: grey">(91f24c0)</span></li>
<li>src/services/magnatune/MagnatuneConfig.h <span style="color: grey">(552bcf8)</span></li>
<li>src/services/magnatune/MagnatuneConfig.cpp <span style="color: grey">(5842c63)</span></li>
<li>src/services/magnatune/MagnatuneDownloadHandler.h <span style="color: grey">(b257440)</span></li>
<li>src/services/magnatune/MagnatuneDownloadHandler.cpp <span style="color: grey">(3bce597)</span></li>
<li>src/services/magnatune/MagnatuneInfoParser.h <span style="color: grey">(7904b67)</span></li>
<li>src/services/magnatune/MagnatuneInfoParser.cpp <span style="color: grey">(f10ad13)</span></li>
<li>src/services/magnatune/MagnatuneNeedUpdateWidget.cpp <span style="color: grey">(044cf4b)</span></li>
<li>src/services/magnatune/MagnatuneRedownloadHandler.cpp <span style="color: grey">(99c1a54)</span></li>
<li>src/services/magnatune/MagnatuneSettingsModule.h <span style="color: grey">(4728a34)</span></li>
<li>src/services/magnatune/MagnatuneSettingsModule.cpp <span style="color: grey">(d45938f)</span></li>
<li>src/services/magnatune/MagnatuneStore.h <span style="color: grey">(c143d59)</span></li>
<li>src/services/magnatune/MagnatuneStore.cpp <span style="color: grey">(2863c5b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110426/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>