<table><tr><td style="">sredman added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D16092">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D16092#340672" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D16092#340672</a>, <a href="https://phabricator.kde.org/p/apol/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@apol</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>Are you sure this makes it blocking? Where are we blocking?</p></div>
</blockquote>
<p>See the inline comments. Would you like these written in to the source code?</p>
<p>The idea is per <a href="https://wiki.qt.io/Threads_Events_QObjects#Forcing_event_dispatching" class="remarkup-link" target="_blank" rel="noreferrer">https://wiki.qt.io/Threads_Events_QObjects#Forcing_event_dispatching</a> . It works because the event queue handles signals serially, and the Device object (which receives the packet from the network), the SmsPlugin object (which handles the packet and the app's message request) and the ConversationDbusInterface (which handles the dbus request) are all resident on the same thread, so all events are handled by the same thread. Reentering the thread's event queue allows the data coming from Android to be handled. Exiting the event queue after the message is handled means the data we need is ready.</p>
<p>I tried to achieve the same thing using locks and multiple threads, but I could not make it work. The core issue is that I could not get the ConversationDbusInterface and the Device to be on separate threads AND have the dbus interface properly registered to the dbus. This means, regardless what happens, the dbus interface waiting would block the packet handling. QWaitCondition::wait() does not allow other events on the same thread to be handled, apparently 😢</p>
<p>For proof, put a breakpoint (or debug print) on smsplugin.ccp lines 56, 91, and 103, then launch the SMS app and open a conversation. You should see that the daemon hits line 91 when the request comes in, then 56 when the phone replies with the messages, then 103 as the daemon returns the messages to the phone (via the ConversationDbusInterface, of course)</p>
<p>For a less difficult test, open the messaging app before this patch and open a conversation. Notice that only one message shows *the first time* because that is all the conversation interface knows about. After the first time a conversation is opened, it will show 10 messages when a conversation is opened because they are in cache. With this patch, you will see 10 every time because the dbus interface waits for its cache to be populated before returning.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16092#inline-87447">View Inline</a><span style="color: #4b4d51; font-weight: bold;">smsplugin.cpp:55</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">return</span></span> <span class="n">handleBatchMessages</span><span class="p">(</span><span class="n">np</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">bool</span></span><span class="bright"> </span><span class="n"><span class="bright">success</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span> <span class="n">handleBatchMessages</span><span class="p">(</span><span class="n">np</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">Q_EMIT</span> <span style="color: #004012">messagesPacketHandled</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">return</span> <span class="n">success</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Unblock - The awaited data is ready (hopefully...)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16092#inline-87448">View Inline</a><span style="color: #4b4d51; font-weight: bold;">smsplugin.cpp:92</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QEventLoop</span> <span class="n">loop</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">connect</span><span class="p">(</span><span style="color: #aa4000">this</span><span class="p">,</span> <span style="color: #aa2211">&</span><span class="n">SmsPlugin</span><span style="color: #aa2211">::</span><span class="n">messagesPacketHandled</span><span class="p">,</span> <span style="color: #aa2211">&</span><span class="n">loop</span><span class="p">,</span> <span style="color: #aa2211">&</span><span class="n">QEventLoop</span><span style="color: #aa2211">::</span><span class="n">quit</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">loop</span><span class="p">.</span><span class="n">exec</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Connect the signal that we have processed some new data to the new event loop's quit signal</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D16092#inline-87449">View Inline</a><span style="color: #4b4d51; font-weight: bold;">smsplugin.cpp:93</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">connect</span><span class="p">(</span><span style="color: #aa4000">this</span><span class="p">,</span> <span style="color: #aa2211">&</span><span class="n">SmsPlugin</span><span style="color: #aa2211">::</span><span class="n">messagesPacketHandled</span><span class="p">,</span> <span style="color: #aa2211">&</span><span class="n">loop</span><span class="p">,</span> <span style="color: #aa2211">&</span><span class="n">QEventLoop</span><span style="color: #aa2211">::</span><span class="n">quit</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">loop</span><span class="p">.</span><span class="n">exec</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Reenter the thread's event loop. loop.exec() does not return until signaled to quit</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R224 KDE Connect</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D16092">https://phabricator.kde.org/D16092</a></div></div><br /><div><strong>To: </strong>sredman, KDE Connect<br /><strong>Cc: </strong>apol, kdeconnect, wistak, dvalencia, rmenezes, julioc, Leptopoda, timothyc, jdvr, yannux, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, tctara<br /></div>