<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/123806/">https://git.reviewboard.kde.org/r/123806/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Mai 16th, 2015, 4:37 nachm. UTC, <b>Christoph Feck</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/123806/diff/3/?file=369517#file369517line32" style="color: black; font-weight: bold; text-decoration: underline;">klipper/klipper.kcfg</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">32</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <whatsthis><qt>If this option is selected, klipper displays whitespaces in a clipboard history item which only consists of whitespace using symbolic placeholders.</qt></whatsthis></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;">It would be immensely useful, if Klipper also showed leading/trailing whitespace, i.e. for items that aren't completely whitespace.
Firefox loves to add leading whitespace when copying double-clicked text, but Klipper does not show this in the menu.</pre>
</blockquote>
<p>On Mai 16th, 2015, 4:44 nachm. UTC, <b>Christoph Feck</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;">See bug 159267.</pre>
</blockquote>
<p>On Mai 16th, 2015, 6:16 nachm. UTC, <b>Patrick Eigensatz</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;">Question if we should make an option to replace whitespaces generally. Like:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">[ ] Allow history items to consist only of whitespaces
[ ] Display whitespace characters with symbols</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The checkboxes would be independent, so it would be possible to see tabs and spaces <strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">AND</strong> the user would still be able select if he wants to see "blank" entries.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Seems like the best solution to me. :)</p></pre>
</blockquote>
<p>On Mai 16th, 2015, 6:50 nachm. UTC, <b>Heiko Tietze</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;">Did my pic before reading your last comment. Consider to use either a checkbox above the radio buttons, just instead of the title, or replace it all by a dropdown menu. Please don't use my bad English as a reference. The mockup illustrates only the alignment idea.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><img alt="Klipper text" src="http://oi59.tinypic.com/ac4o6c.jpg" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" /></p></pre>
</blockquote>
<p>On Mai 16th, 2015, 7:22 nachm. UTC, <b>Patrick Eigensatz</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;">Heiko, I see your concerns on grouping and aligning; I could change the design and create a seperate patch and a seperate view request.
Hmm, if we use radio buttons it won't be possible to have symbolic placeholders <strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">and</strong> ignore blank entries... How would you solve this?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thank you</p></pre>
</blockquote>
<p>On Mai 16th, 2015, 8:04 nachm. UTC, <b>Heiko Tietze</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;">If an option is not exclusive the checkbox comes into play. But I don't think you can have both features. And finally I agree that the solution in #123821 is good and a simple checkbox 'Trim whitespace characters' would be sufficient.
However, the grouping and the alignment has still room for improvements. To be honest the whole feature is not 'simple by default', which should be our first concern.</p></pre>
</blockquote>
<p>On Mai 16th, 2015, 8:24 nachm. UTC, <b>Patrick Eigensatz</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;">Okay, so solution #123821 for default...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm not sure if we should file a bug or if I just file another review request with a "design patch" to fulfill the HIG altough I'm new to QTDesigner...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I start now implementing a simple checkbox "Trim whitespaces" additionally to solution #123821.</p></pre>
</blockquote>
<p>On Mai 16th, 2015, 9 nachm. UTC, <b>Thomas Lübking</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;">@Heiko
"Ignore selection" (i assume the entire selection part) refers to the primary selection buffer, the thing where stuff is copied when you "select" it (with the mouse) and that's pasted w/ MMB clicks.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">http://en.wikipedia.org/wiki/X_Window_selection</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I assume the timeout and history size items don't belong into that box, which could then be a checkable groupbox (but some klipper dev should confirm - or we must lookup the code ;-)</p></pre>
</blockquote>
<p>On Mai 16th, 2015, 9:41 nachm. UTC, <b>Thomas Pfeiffer</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 do agree with Heiko that the whole dialog is in need of improvement. Let's please keep this thread to the discussion of the feature in question, however.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As for that: Having the radio button group makes sense to me. And, like Heiko, I do not think that replacing whitespace with placeholders but not allowing whitespace-only entries is a useful option (even though it is technically possible, of course). Both are useful mostly for people which often work with "code" of some kind, so there aren't likely to be many users who want one but not the other. And those few users that may want only one are not worth making the whole thing more complex for all users.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And this way we do not need any complicated "allow whitespace-only entries" label. Either whitespace is trimmed - which logically implies that entries consisting only of whitespace are eliminated altogether - or it isn't, which means that entries consisting only of whitespace are not eliminated.</p></pre>
</blockquote>
<p>On Mai 16th, 2015, 10:40 nachm. UTC, <b>Patrick Eigensatz</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 will open another review request so we can improve the dialog.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Did I understand correctly now?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Whitespace handling</strong>
( ) Trim whitespace characters <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">(default?)</em>
(o) Normal whitespace</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As you said, trimming would eliminate whitespace-only entries, too.</p></pre>
</blockquote>
<p>On Mai 16th, 2015, 10:59 nachm. UTC, <b>Thomas Pfeiffer</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;">Either that plus
( ) Replace whitespace with symbols</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Or just
[ ] Trim whitespace characters
And then always use placeholders if trim is unchecked.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm still not 100% sure if there might be people who want to keep their whitespace as is (which would be "Normal whitespace").</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ideally someone would show the solution from #123821 to a few people and ask them what that is and if they find it useful. And ask those who do not find it useful whether they'd prefer just trimming the whitespace</p></pre>
</blockquote>
<p>On Mai 16th, 2015, 11:19 nachm. UTC, <b>Patrick Eigensatz</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, we need to decide:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">a) The symbols from Solution #123821 always. Plus a trim checkbox in the config dialog.
<img alt="Trim checkbox" src="http://i62.tinypic.com/zr7sw.jpg" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" /></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">b) The 3 radiobuttons:
<img alt="Radiobutton group" src="http://i61.tinypic.com/2hqh69u.jpg" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" /></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Quote from Christoph Feck, Solution #123821:
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">"The coloring isn't annoying, so I see no reason why anyone would need to turn this off."</em></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If the people we ask think this is true, we would just make it default and renounce another checkbox.</p></pre>
</blockquote>
<p>On Mai 17th, 2015, 7:14 vorm. UTC, <b>Thomas Lübking</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;">"Trim blank whitespace entries"? ;-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Thomas P.</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;">And then always use placeholders if trim is unchecked.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Please notice itr. that "trim" is different from "simplify" - the former only removes trailing and leading whitespaces while the latter reduces every whitespace "area" to a single space, ie. trimmed text may still contain newlines (ie. pasting that into eg. konsole would cause a command execution)</p></pre>
</blockquote>
<p>On Mai 17th, 2015, 9:19 vorm. UTC, <b>Patrick Eigensatz</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;">@Thomas Lübking: Yes, this wording is somehow ironic... Should I use "Trim whitespaces"?</p></pre>
</blockquote>
<p>On Mai 17th, 2015, 9:42 vorm. UTC, <b>Thomas Lübking</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;">depends on what it ultimately will do.
Either "Trim whitespace" or "Allow/Ignore whitespace-only text" (Ignore if the negative wording isn't changed)</p></pre>
</blockquote>
<p>On Mai 17th, 2015, 11:51 vorm. UTC, <b>Thomas Pfeiffer</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;">Hm... I tend toward trimming all non-printable characters at the beginning and end of each selection, e.g. to avoid unintentional code execution while pasting into a console.
On the other hand, people may want to select and paste words with leading or trailing spaces into a text.
It may still make sense to only the choice between either trimming the whitespace or showing the placeholders. That way accidental code execution would be avoided.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It's also something we should test with a few users, though.</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;">if you paste QString(" \n \t sudo dd if=/dev/zero of=/dev/sda\necho BLAST\n \t \n ").trimmed() into konsole, you still have a problem (becomes "sudo dd if=/dev/zero of=/dev/sda\necho BLAST", "sudo dd if=/dev/zero of=/dev/sda" is executed)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you want to prevent accidental code execution, you'll have to go for QString(" \n \t sudo dd if=/dev/zero of=/dev/sda\necho BLAST\n \t \n ").simplified(), which becomes "sudo dd if=/dev/zero of=/dev/sda echo BLAST"</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So the toss is probably between verbatim whitespace (which should be indicated with placeholders) and simplified whitespace, where <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">all</em> whitespace is replaced by a single space (and trimmed from the ends)?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">PS: please don't try what that command does ;-)</p></pre>
<br />
<p>- Thomas</p>
<br />
<p>On Mai 16th, 2015, 9:31 nachm. UTC, Patrick Eigensatz 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-workspace, KDE Usability and Patrick Eigensatz.</div>
<div>By Patrick Eigensatz.</div>
<p style="color: grey;"><i>Updated Mai 16, 2015, 9:31 nachm.</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=159267">159267</a>,
<a href="https://bugs.kde.org/show_bug.cgi?id=192922">192922</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;">[PATCH] plasma-workspace: klipper: Fix #192922 Ignore blank entries</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QString::isEmpty() is used to check if the string only consists of whitespace characters. If it does, the creation of the HistoryStringItem fails.</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/generalconfig.ui <span style="color: grey">(f513e9c)</span></li>
<li>klipper/historyitem.cpp <span style="color: grey">(36cbe61)</span></li>
<li>klipper/klipper.h <span style="color: grey">(6952b11)</span></li>
<li>klipper/klipper.cpp <span style="color: grey">(798b49f)</span></li>
<li>klipper/klipper.kcfg <span style="color: grey">(a03dd16)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123806/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>