<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/119866/">https://git.reviewboard.kde.org/r/119866/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 22nd, 2014, 1:55 p.m. UTC, <b>David Edmundson</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/119866/diff/1/?file=306729#file306729line73" style="color: black; font-weight: bold; text-decoration: underline;">applets/clipboard/contents/ui/ClipboardItemDelegate.qml</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">73</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nx">Item</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 item only contains one sub-item which fills the parent.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">it can be deleted, and these properties moved into the ListView</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;">The extra item is actually used now, it contains the image, the background overlay for the text, and the text itself.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 22nd, 2014, 1:55 p.m. UTC, <b>David Edmundson</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/119866/diff/1/?file=306729#file306729line94" style="color: black; font-weight: bold; text-decoration: underline;">applets/clipboard/contents/ui/ClipboardItemDelegate.qml</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">94</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">y:</span> <span class="nb">Math</span><span class="p">.</span><span class="nx">round</span><span class="p">((</span><span class="nx">parent</span><span class="p">.</span><span class="nx">height</span> <span class="o">-</span> <span class="nx">previewList</span><span class="p">.</span><span class="nx">itemHeight</span><span class="p">)</span> <span class="o">/</span> <span class="mi">2</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;">adjusting the listview to be vertically aligned would be better than each delegate.</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;">The code to do that would be quite a bit more complicated, since we'd have to check which item produces the size (there's conditionals, it could be either the items in the list, or the buttons, or the text or the image data), adjusting the y position here produces both, the cleaner code and less codepaths to be tested.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 22nd, 2014, 1:55 p.m. UTC, <b>David Edmundson</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/119866/diff/1/?file=306729#file306729line102" style="color: black; font-weight: bold; text-decoration: underline;">applets/clipboard/contents/ui/ClipboardItemDelegate.qml</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">102</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kd">function</span> <span class="nx">result</span><span class="p">(</span><span class="nx">job</span><span class="p">)</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;">IMHO this is clearer as a function outside Component.onCompleted.</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;">We use this pattern pretty consistently (i.e. declaring the slot and the connect in the same code block). I like how it keeps pieces that belong together in one block without polluting the public namespace. I can move the whole preview-get-and-set dance into a separate function, if you prefer that?</p></pre>
<br />
<p>- Sebastian</p>
<br />
<p>On August 22nd, 2014, 2:08 p.m. UTC, Sebastian Kügler 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 Plasma.</div>
<div>By Sebastian Kügler.</div>
<p style="color: grey;"><i>Updated Aug. 22, 2014, 2:08 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-workspace
</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;">This patch paints previews of copied images, instead of displaying the urls in plain text.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The display is limited to 4 images right now. The painting of the delegates is in line with the recommendations from the HIG at https://techbase.kde.org/Projects/Usability/HIG/Layout/Image</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It has a few rough edges:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- not all images get thumbnails -- need to investigate why<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- vertical alignment throughout the list is quite bad, this is the case already, I will fix that separately<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- I want to add an indicator that it's more than 4 files (if applicable), working on that still<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- When no preview is loaded, it should show an icon, almost done, with some fixes</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'd like some feedback on the code, so that with the remaining issues fixed, it can get in.</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;">Copied images and files in Dolphin, see them showing up in Klipper. Made sure that the previews are only loaded when needed, so no additional memory taken when the systray / klipper popup is not open.</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>applets/clipboard/contents/ui/ClipboardItemDelegate.qml <span style="color: grey">(8eecb80)</span></li>
<li>applets/clipboard/contents/ui/Menu.qml <span style="color: grey">(e6821c3)</span></li>
<li>applets/clipboard/contents/ui/clipboard.qml <span style="color: grey">(ac784d2)</span></li>
<li>klipper/CMakeLists.txt <span style="color: grey">(660b0d1)</span></li>
<li>klipper/clipboardjob.h <span style="color: grey">(b4f5284)</span></li>
<li>klipper/clipboardjob.cpp <span style="color: grey">(d84d014)</span></li>
<li>klipper/historyurlitem.cpp <span style="color: grey">(fb2a766)</span></li>
<li>klipper/org.kde.plasma.clipboard.operations <span style="color: grey">(813aafa)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119866/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/08/20/ebdde483-136e-44da-8735-f98ba88485a9__klipper-before.png">Before</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/08/20/65749cc3-3fed-4c9b-9a28-b9791b8e93d8__klipper-thumbnails.png">Klipper with thumbnails</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/08/22/4fb72495-0781-4535-8de8-5266652b467a__klipper-after.png">Klipper with thumbnails 2nd version</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>