<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/105566/">http://git.reviewboard.kde.org/r/105566/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 14th, 2012, 9:19 a.m., <b>George Kiagiadakis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/105566/diff/1/?file=72583#file72583line11" style="color: black; font-weight: bold; text-decoration: underline;">KTp/pending-wallet-operation.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">PendingWalletOperation::PendingWalletOperation(KWallet::Wallet *wallet)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">11</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">connect</span><span class="p">(</span><span class="n">wallet</span><span class="p">,</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">walletOpened</span><span class="p">(</span><span class="kt">bool</span><span class="p">)),</span> <span class="n">SLOT</span><span class="p">(</span><span class="n">setFinished</span><span class="p">()));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What if it fails...?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">If it fails, the developer should continue using the WalletInterface as though everything is fine. The attempt to open it succeeded. Code in all the internal methods here will check if it can load/save passwords and act appropriately.
If it fails it potentially means the user cancelled entering their wallet password and we should /not/ try again.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 14th, 2012, 9:19 a.m., <b>George Kiagiadakis</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/105566/diff/1/?file=72585#file72585line52" style="color: black; font-weight: bold; text-decoration: underline;">KTp/wallet-interface.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">Tp::PendingOperation *WalletInterface::openWallet()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">52</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">static</span> <span class="n">QMutex</span> <span class="n">mutex</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">53</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">mutex</span><span class="p">.</span><span class="n">lock</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Good thing that you thought about threads here, but the rest of the API is still not thread-safe.
Solution #1 would be to have one global mutex and lock it in every method.
Solution #2 would be to make the class non-singleton so that you can claim the api is re-entrant and be done with it.
I would personally prefer the second solution as the first solution would only make things slower and still unsafe. By unsafe I mean that if there are two different parts of an application trying to access the wallet and one part assumes that the other part has opened the wallet, but then something happens and this assertion is no longer true, then you have a bug that is quite difficult to trace.
The solution would be to have a non-singleton WalletInterface with a private constructor and the openWallet() method returning a KTp::PendingWallet, which has a wallet() method returning the pointer to the interface (just like other PendingOperations in tp-qt). This way, to get access to the wallet you must go through the opening process, which ensures you are not going to do any mistake.
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">We need the call to KWallet::Wallet::openWallet to be in a singleton somewhere, otherwise we go back to a bug we had in 0.3 days where the KWallet requests to be opened lots of times and that causes a mess on setups where people have the wallet to "prompt every time" or "close when app stops using it". I need to always use the same instance of the KWallet::Wallet.
I did try (the alternate review) a version something similar to what you said...but given the wallet usages throughout the auth handler and accounts KCM it became a much more annoying change to implement. I have to put in pendingOperations to open the wallet all over the place, even though it's probably already tried being opened.
</pre>
<br />
<p>- David</p>
<br />
<p>On July 14th, 2012, 12:50 a.m., David Edmundson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Telepathy.</div>
<div>By David Edmundson.</div>
<p style="color: grey;"><i>Updated July 14, 2012, 12:50 a.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;">Adds a method to asynchronously open the wallet. This must be called (and completed) by the application if they want to use any other methods.
API is a bit horrendous, but that's mostly because of KWallet.
Wallet opening appears as a Tp::PendingOperation so that it can be used in a composite job.</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>KTp/CMakeLists.txt <span style="color: grey">(61da66d)</span></li>
<li>KTp/pending-wallet-operation.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>KTp/pending-wallet-operation.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>KTp/wallet-interface.h <span style="color: grey">(c43fb38)</span></li>
<li>KTp/wallet-interface.cpp <span style="color: grey">(d2dc2cb)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105566/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>