<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/122382/">https://git.reviewboard.kde.org/r/122382/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 3rd, 2015, 8:36 a.m. CET, <b>Martin Gräßlin</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;">David E. just pointed out that this could become quite heavy for the system as the history size can be large (up to 2048 items).</p></pre>
</blockquote>
<p>On February 3rd, 2015, 8:41 a.m. CET, <b>Martin Gräßlin</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;">Unfortunately I couldn't find out why we support up to 2048 items. Commit message is just:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">commit da8394ce42a24726392265436c3808f1ac9389aa
Author: Esben Mose Hansen <a href="mailto:kde@mosehansen.dk" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">kde@mosehansen.dk</a>
Date: Fri Nov 19 22:28:55 2004 +0000</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%">Introduced support <span style="color: #008000; font-weight: bold">for</span> large clipboard histories up to <span style="color: #666666">2048</span> items.
svn path<span style="color: #666666">=/</span>trunk<span style="color: #666666">/</span>kdebase<span style="color: #666666">/</span>klipper<span style="color: #666666">/</span>; revision<span style="color: #666666">=364353</span>
</pre></div>
</p></pre>
</blockquote>
<p>On February 3rd, 2015, 10:55 a.m. CET, <b>Martin Klapetek</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 think 2048 is insane. Can we make it like 32 by default and have it configurable with big fat warning when you choose more than say 100? </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Btw. does klipper store things encrypted or something? There's also a security concern, especially if your klipper contains passwords, that saving those to disk unecrypted after each copy is insecure (all you need is a watcher on the history file).</p></pre>
</blockquote>
<p>On February 3rd, 2015, 11:13 a.m. CET, <b>Martin Gräßlin</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think 2048 is insane. Can we make it like 32 by default and have it configurable with big fat warning when you choose more than say 100? </p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The default is 7. Adding a warning is certainly possible.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Btw. does klipper store things encrypted or something? There's also a security concern, especially if your klipper contains passwords, that saving those to disk unecrypted after each copy is insecure (all you need is a watcher on the history file).</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you are able to watch the file you are also able to connect to the X11 Display and just do a passive keyboard grab. So caring about that probably doesn't matter (on Wayland this might get more important - maybe we can skip passwords). But setting the file to 600 is certainly a good idea.</p></pre>
</blockquote>
<p>On February 3rd, 2015, 11:22 a.m. CET, <b>Martin Gräßlin</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But setting the file to 600 is certainly a good idea.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">this seems already to be the case (though I don't find the code for it)</p></pre>
</blockquote>
<p>On February 9th, 2015, 5:03 p.m. CET, <b>Filip Wieladek</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;">FYI: I happened to see cross this. I only use Klipper at 2048. With such a size of the data, it means that I usually have my most frequently used items always available.</p></pre>
</blockquote>
<p>On February 16th, 2015, 8:55 a.m. CET, <b>Martin Gräßlin</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'm still unsure what to do about this one. If we go for syncing to disk we probably break workflows like Filip's. Maybe we need to add an additional option to automatically sync and show a warning if the history size gets larger than e.g. 20?</p></pre>
</blockquote>
<p>On February 16th, 2015, 9:02 a.m. CET, <b>Filip Wieladek</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 how much work it would be, but would it be possible to separate the klipper history engine into a seperate daemon process and communicate with it over the DBUS? In this case the daemon would be much less likely to crash (due to plasma) and would not have to rely on frequent disk access.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If that is not possible, or too much work, why dont we simply take a snapshot, convert the history into a QList (that should be fast) and queue up for a save? The save process could then be independent and save a bit later (it could also debounce the save's).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">However, there might be one more problem. If the plasmashell crashing is really an issue, what happens if plasmashell crashes during a save operation? Now it has the potential to corrupt the data losing all the history instead of the latest entries. That means, that his approach would need to use at least 2 files, to write in a rotation, so that there is at least one good file to read from.</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't know how much work it would be, but would it be possible to separate the klipper history engine into a seperate daemon process and communicate with it over the DBUS?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think it's too much work, especially to keep everything race free (changing clipboard could happen from multiple places in async way) and up to date.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If that is not possible, or too much work, why dont we simply take a snapshot, convert the history into a QList (that should be fast) and queue up for a save? The save process could then be independent and save a bit later (it could also debounce the save's).</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">A throttling could also be implemented directly without the need to add a separate process.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now it has the potential to corrupt the data losing all the history instead of the latest entries. That means, that his approach would need to use at least 2 files, to write in a rotation, so that there is at least one good file to read from.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That should not be an issue as QSaveFile is used.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On February 2nd, 2015, 4:12 p.m. CET, Martin Gräßlin 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 and Eike Hein.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Feb. 2, 2015, 4:12 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="https://bugs.kde.org/show_bug.cgi?id=343333">343333</a>
</div>
<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;">By invoking saveHistory after each change we ensure that the clipboard
doesn't lose data in case klipper (or in dataengine mode plasmashell)
crashes.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">To not cause stalls, the saving is performed in a thread using
QtConcurrentRun. As klipper itself is not thread save a Mutex is
used to lock changes in the HistoryModel.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">BUG: 343333
FIXED-IN: 5.3.0</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;">looked at ~/.local/share/klipper/history2.lst in Okteta, changed clipboard and pressed F5 in Okteta. Repeated these steps multiple times.</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>klipper/klipper.cpp <span style="color: grey">(d49c165759f8171931167687c3b36b3a9d7dee07)</span></li>
<li>klipper/CMakeLists.txt <span style="color: grey">(a08f062480b15f32f049e2d0d0e311dbe2964c02)</span></li>
<li>klipper/historymodel.h <span style="color: grey">(78f955f0ec4b8f27dbca0573b68691be6a30e3be)</span></li>
<li>klipper/historymodel.cpp <span style="color: grey">(51860f6c3aca1022a2b721c27c859fc721915353)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122382/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>