<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/130058/">https://git.reviewboard.kde.org/r/130058/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 1st, 2017, 9:59 p.m. CEST, <b>David Faure</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Makes sense to me, +1.</p></pre>
</blockquote>
<p>On April 8th, 2017, 2:39 p.m. CEST, <b>Andreas Sturmlechner</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks, anyone else who wants to +1?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've tried to test migration today but it didn't work. May as well have nothing to do with te patch and be caused by the permanently troubled migration agent though... KMail happily gets its IMAP password from kwallet5 though after manually export/import via XML files.</p></pre>
</blockquote>
<p>On April 9th, 2017, 9:20 p.m. CEST, <b>Andreas Sturmlechner</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As suspected, on my test system migration is broken regardless of with these patches or not.</p></pre>
</blockquote>
<p>On April 15th, 2017, 10:16 a.m. CEST, <b>David Faure</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Are you planning on looking into that? ;-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">These patches are related to migration, it feels a bit wrong to commit changes around migration and still leave it broken.</p></pre>
</blockquote>
<p>On April 16th, 2017, 7:54 p.m. CEST, <b>Andreas Sturmlechner</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't feel like I'm up to that task. Also, the reason for why it works for some, but not all the systems, has afaik never been established.</p></pre>
</blockquote>
<p>On May 27th, 2017, 1:58 p.m. CEST, <b>Andreas Sturmlechner</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Could we push it to give it some testing by others until the upcoming release?</p></pre>
</blockquote>
<p>On June 15th, 2017, 10:46 a.m. CEST, <b>René J.V. Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Was this tested, in the end?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I only became aware of the change after I installed 5.35.0 on my FrankenStux box with a Plasma4 desktop and KF5 installed into /opt/local, and I had to re-grant access to my wallets to all applications accessing them. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I cannot yet vouch that my wallets were migrated completely. I think they were, but how do I verify that now? Even the KDE4 kwalletmanager goes through kwalletd5. It too had to be granted access.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Confusingly, it still shows the old access list, which clearly it shouldn't if the information is stale.</p></pre>
</blockquote>
<p>On June 15th, 2017, 11:10 a.m. CEST, <b>Andreas Sturmlechner</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you see your old wallets now in kwalletd5, then migration has worked. I've never heard about issues with partial migration; if it does not work, it fails to migrate completely.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That's the impression I had, but I never had to check. Migration was done about once after every time I logged in after a reboot. I never had to check though, kwalletd5 only served KF5 applications of which very few ever need wallet access in my case.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">We'll see how this works out when I log in and /opt/local isn't available (which in theory is supposed to be possible).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">To be seen also how this affects KDE/Mac users who have KDELibs4 configured to use my Keychain "backend". There's no kwalletd4 there, no migration either, and first impressions suggest the 2 systems continue to function in perfect isolation.</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On May 27th, 2017, 4:32 p.m. CEST, Andreas Sturmlechner wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks and Stefan Brüns.</div>
<div>By Andreas Sturmlechner.</div>
<p style="color: grey;"><i>Updated May 27, 2017, 4:32 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kwallet
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">These are not my own patches, I'm creating this review request after having been made aware of <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">kwalletd4_dbus_compat</em> branch in kwallet.git, which I simply rebased on top of current master (author of course preserved) to be able to test it. I think it would be a great improvement over the current situation that is rather confusing to the users.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The changes are organised in 5 commits:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Check for unique applicaton instance as early as possible
Exit before KWalletD and the MigrationAgent has been initialized.
The return value is changed, but concurrent instatiation of kwalletd is
not a fault.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Only start timer for migration agent if necessary</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Whitespace fixup</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Signal completion of migration agent</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Replace kwalletd4 after migration has finished
kwalletd5 can service both org.kde.kwalletd5 and org.kde.kwalletd</li>
</ul></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Migration itself was not tested so far, but a legacy application like ksirk was able to create a new wallet just fine and can access it as well. I do not have kwalletd4 installed anymore.</p></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/runtime/kwalletd/kwalletd.h <span style="color: grey">(3571535cd8bd78415002795f9b61adf9f6cfb8c1)</span></li>
<li>src/runtime/kwalletd/kwalletd.cpp <span style="color: grey">(18ef9fa7e6ddaeba6e0b32deae3de1dae39df5bb)</span></li>
<li>src/runtime/kwalletd/main.cpp <span style="color: grey">(ff9620886fa1959e14b594be6bbb4644b637c000)</span></li>
<li>src/runtime/kwalletd/migrationagent.h <span style="color: grey">(0f6467c1753ef34b7f7f7e282503ec5607927db9)</span></li>
<li>src/runtime/kwalletd/migrationagent.cpp <span style="color: grey">(f3da94743ecd83fe406e058f560d4238caec1be8)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/130058/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>