<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/124856/">https://git.reviewboard.kde.org/r/124856/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Agosto 21st, 2015, 8:08 a.m. UTC, <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;">I thought the idea was "there's no race, because as long as the recipient doesn't go to the event loop, it won't be processing any incoming messages anyway"?</p></pre>
</blockquote>
<p>On Agosto 21st, 2015, 4:04 p.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;">But with Thiago's patches dbus runs in a thread so that's not true anymore.</p></pre>
</blockquote>
<p>On Agosto 21st, 2015, 8:19 p.m. UTC, <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;">Why don't Thiago's patches preserve behavior compatibility by starting/enabling the dbus thread only when reaching the event loop?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Otherwise it sounds like any case of "talking to an application that is starting" will create races (am I making the call before or after the object has been published?). Not just KUniqueApplication, but any other dbus call to a starting app, including the dbus activation case.</p></pre>
</blockquote>
<p>On Agosto 21st, 2015, 8:47 p.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;">I don't know, he knows better, hope he has time to answer. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And anyhow even if you disagree with his patches to Qt dbus, there's no reason to discard this patch, saves us a dbus call on the startup case, makes the app start marginally faster i guess :D</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;">Right, the patch itself looks fine and I'd say "ship it".</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As for the behaviour-compatibility... it's because I had not thought of it until now. And if it weren't for the QDaemonThread, it wouldn't be doing that. I guess we can discuss what to do -- back to the ML for this.</p></pre>
<br />
<p>- Thiago</p>
<br />
<p>On Agosto 21st, 2015, 4:06 p.m. UTC, Albert Astals Cid 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 Thiago Macieira.</div>
<div>By Albert Astals Cid.</div>
<p style="color: grey;"><i>Updated Ago. 21, 2015, 4:06 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs4support
</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;">For the first invocation, call newInstance ourselves instead of relying on the parent process to do it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">With the new threading patches in qt dbus the current code races as the parent may end up calling newInstance before the child has set up the object that handles it.</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;">konsole starts fine with this + Thiago's Qt dbus threading patches</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/kdeui/kuniqueapplication.cpp <span style="color: grey">(713c6f4)</span></li>
<li>src/kdeui/kuniqueapplication_p.h <span style="color: grey">(de4b328)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124856/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>