<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/120920/">https://git.reviewboard.kde.org/r/120920/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On novembre 8th, 2014, 11:54 a.m. UTC, <b>Aaron J. Seigo</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;">So firstly: thanks for working on this; the slow menu population is indeed a wart on the application.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That said, I'd prefer to see it done with a QRunnable. There is less to get wrong than with a QThread, even though the code you have would remain nearly the same. (So little/no effort lost ...) The QRunnable would emit a signal with the populated list of actions which KSnapshot would then hang on to itself. It could even take a pointer to the parent QObject to parent the actions to (and move them to the same thread as that QObject) which would clean up a few more details.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">With that done, this could certainly go into master as well as the frameworks branch (by which point kipi should be shortly on the way to being ported)</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;">I find that QtConcurrent::run is usually easier than using a QRunnable, have a look at it too.</p></pre>
<br />
<p>- Albert</p>
<br />
<p>On novembre 1st, 2014, 1:46 p.m. UTC, Gregor Mi 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 Graphics, KSnapshot, Aaron J. Seigo, and Dominik Haumann.</div>
<div>By Gregor Mi.</div>
<p style="color: grey;"><i>Updated nov. 1, 2014, 1:46 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=312495">312495</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
ksnapshot
</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;">Hi, I also feel slightly impaired by Bug 312495 - Very slow "Send to" menu. So here is a fix.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It consists of the following commits:
1. move long-running retrieval of SendTo items to separate method
2. fillSendToActionsCache at startup but not yet async
3. move code related to SendTo actions to own class and file
(this includes the kipi code but KIPI_FOUND was not set in my dev env so thats not tested)
4. SendTo menu is built on a separate thread to speed up populating
5. delete actions in dtor</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The SendTo actions are now created in QThread which is started on startup of ksnapshot.
If the user opens the SendTo menu too soon there is a message (as single menu item) that says
that the menu is still being constructed and that he/she should try a moment later.</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;"><ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Start ksnapshot (notice that is as fast as before)</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Immediately open SendTo menu => there is a message that the menu is not ready yet.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Try again after 1 second: menu is populated.</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 ksnapshot.</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;">Start ksnapshot. Wait two seconds and then click SendTo menu
=> menu is populated right away</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 it.</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;">Start ksnapshot. Hit the SendTo menu with insanely high button click speed.
=> No crash observed.</p>
</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Could NOT test the moved kipi code because KIPI_FOUND is not set on my environment.</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>CMakeLists.txt <span style="color: grey">(7ec17867e599fb8ea06b709e22fb7437e0207e9b)</span></li>
<li>ksnapshot.h <span style="color: grey">(898e313cf963a132387e9e5ecf422f94a1437d0a)</span></li>
<li>ksnapshot.cpp <span style="color: grey">(81ecd392ec7fe4076d68d0ef52ca82b6315bf150)</span></li>
<li>ksnapshotsendtoactions.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>ksnapshotsendtoactions.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120920/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>