<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/120202/">https://git.reviewboard.kde.org/r/120202/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On setembre 18th, 2014, 10:28 p.m. UTC, <b>Albert Astals Cid</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="https://git.reviewboard.kde.org/r/120202/diff/1/?file=312224#file312224line545" style="color: black; font-weight: bold; text-decoration: underline;">kdeui/util/kwallet.h</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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class KDEUI_EXPORT Wallet : public QObject</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">544</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="nf">slotIdleTimedOut</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is bad, slots in an ifdef are a bad idea.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is there any reason this slot has to be in KWallet and not some other object?</p></pre>
</blockquote>
<p>On setembre 21st, 2014, 10:35 a.m. UTC, <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;">May I ask why slots in an #ifdef are a bad idea, worse than any other kind of code? I can see why platform-specific class members are not very elegant, but not why that would be different for slots.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The slot has to have access to the Wallet instance, but it should be possible to move it into the KWalletPrivate class since I already added a pointer to the instance to that class. Would that be better?</p></pre>
</blockquote>
<p>On setembre 21st, 2014, 10:39 a.m. UTC, <b>Albert Astals Cid</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;">An ifdef in a public class is horrible.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As a user of the KWallet class when i should connect to it? Never? Then don't show me to me it exists.</p></pre>
</blockquote>
<p>On setembre 21st, 2014, 10:53 a.m. UTC, <b>Thomas Lübking</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;">moc does not handle preprocessor statements.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
You'll likely get some error if the condition does not match because moc adds the slot unconditionally, but the function isn't available.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That aside, #ifdef'ing functions in a public header (ie. exported API) is a bad idea in general, because it causes different ABI (not that much a problem of an architecure split) and usually confusion.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">-> preattyplease #ifdef the implementation instead.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #B00040">void</span> Foo<span style="color: #666666">::</span>bar()
{
<span style="color: #BC7A00">#if WANT_THIS</span>
fooBarFoo();
<span style="color: #BC7A00">#endif</span>
}
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Albert<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
tbf, there're quite some present slots tagged "internal" in that class and since they're all private slots, they're not available to user code anyway. The general approach is ok, because they don't affect the vtable.</p></pre>
</blockquote>
<p>On setembre 21st, 2014, 11:04 a.m. UTC, <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;">Understood (and agreed). Not why moc doesn't take ifdefs into account, but that may be a design choice, and isn't relevant here.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Thomas, i think last i checked you can still to connect to private slots, the only thing is that you can't call them directly, but the metaobject doesn't know about "private".</p></pre>
<br />
<p>- Albert</p>
<br />
<p>On setembre 14th, 2014, 3:36 p.m. UTC, René J.V. Bertin 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 Software on Mac OS X and kdelibs.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated set. 14, 2014, 3:36 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</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;">I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus.</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;">idle time closing of wallets. This feature was not supported in the commited version presented in https://git.reviewboard.kde.org/r/119838/ The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is >= the timeout, the wallet is closed. This applies only to "KDE keychains", so keychains used by OS X applications should not be affected.</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;">"close when last application exits". This requires maintaining a "user list" which keeps track of what application has what wallet open. I've implemented an "internal" version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it.</p>
</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kwallet.cpp</code> wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">qdbusviewer</code>. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose.</p></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;">OS X 10.6.8, kdelibs 4.14.1 git/master, KDE/MacPorts 4.12.5 .<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Once finalised, all changes should port easily to KF5's kwallet_mac.cpp .</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>kdeui/util/kwallet.h <span style="color: grey">(d7f703f)</span></li>
<li>kdeui/util/kwallet_mac.cpp <span style="color: grey">(8344ebb)</span></li>
<li>kdeui/util/qosxkeychain.h <span style="color: grey">(d0934e6)</span></li>
<li>kdeui/util/qosxkeychain.cpp <span style="color: grey">(7cb9a22)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120202/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>