<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 May 16th, 2015, 11:37 p.m. CEST, <b>Patrick Eigensatz</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/5/?file=369605#file369605line91" style="color: black; font-weight: bold; text-decoration: underline;">klipper/historyitem.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">HistoryItemPtr HistoryItem::create( QDataStream& dataStream ) {</pre></td>
</tr>
</tbody>
<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">91</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span> <span class="n">Klipper</span><span class="p">.</span><span class="n">m_bTrimBlankEntries</span> <span class="o">&&</span> <span class="n">text</span><span class="p">.</span><span class="n">isEmpty</span><span class="p">()</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;">I'm not sure if I can access "Klipper" from here. If I have a look at how this is done at other options then I see that they only use the <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">m_bSomeSetting</em> vars inside the klipper class itself.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I need some help on how I should access the setting. (Pass it? Check it in the klipper class?)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thank you :)</p></pre>
</blockquote>
<p>On May 17th, 2015, 12:28 p.m. CEST, <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;">ensure klippersettings.h is included (ie. add it, implicit inclusion is a timebomb) and use KlipperSettings::trimBlankEntries()</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"Klipper" is a class and you cannot access a class this way.
If m_bTrimBlankEntries <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">was</em> (don't make it!) a static public member, you could access it via Klipper::m_bTrimBlankEntries (after including klipper.h here)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You're btw. expected to compile the changes.
Een reviews will easily oversee missing semicolons etc. - compilers do a far better job itr. ;-)</p></pre>
</blockquote>
<p>On May 17th, 2015, 12:52 p.m. CEST, <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;">Ah, I thougth we would load the settings from KlipperSettings:: into Klipper - an instance of the <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">klipper</em> class - and then never use it again. If I can simply include klippersettings.h and get the value
from there, that's fine. Thank you!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, I'm very sorry about this. I've spent hours and nights configuring kdesrc-build and I've tried to compile klipper in a Kubuntu-VM, followed the whole Plasma/Building guide - still didn't work.
Now trying with cmake ;)</p></pre>
</blockquote>
<p>On May 17th, 2015, 9:12 p.m. CEST, <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;">Just a small resumé:
1) Create an option "NAME" to simplify the strings using QString::simplify()
2) This option is enabled by default</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">We only need to find a name for this option.</p></pre>
</blockquote>
<p>On May 17th, 2015, 9:29 p.m. CEST, <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;">I wouldn't enable this option by default. The default should be pasting what was copied.
Also, please do not mix options that affect the actual clipboard contents and the representation in the menu. An option that says "Replace whitespace with symbols" is highly confusing next to an option that says "Trim whitespace".</pre>
</blockquote>
<p>On May 17th, 2015, 9:36 p.m. CEST, <b>Kai Uwe Broulik</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;">Btw I just pushed the highlight feature to the Klipper plasmoid and I don't see why that one should be configurable.</p></pre>
</blockquote>
<p>On May 18th, 2015, 12:44 a.m. CEST, <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;"><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 wouldn't enable this option by default. The default should be pasting what was copied.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Please explain what ordinary users would need extra whitespace at the beginning or end of a selection for.</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;">Also, please do not mix options that affect the actual clipboard contents and the representation in the menu. An option that says "Replace whitespace with symbols" is highly confusing next to an option that says "Trim whitespace".
Btw I just pushed the highlight feature to the Klipper plasmoid and I don't see why that one should be configurable.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Makes sense. Just one option for trimming, then. If it's not trimmed, it's replaced. I agree that there's no reason why one would not want the whitespace visualized in a better way.</p></pre>
</blockquote>
<p>On May 18th, 2015, 12:57 a.m. CEST, <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;">> Please explain what ordinary users would need extra whitespace at the beginning or end of a selection for.
When reordering words in a text-editor, I cut/paste them including a space, otherwise I end up with wrong (double or missing) spaces.</pre>
</blockquote>
<p>On May 18th, 2015, 1:11 a.m. CEST, <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 think it is okay to display whitespace characters in a better way like Kai Uwe's Patch does without adding a configuration option.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think however it should always be possible for a user to copy a text without klipper modifying it. If we would ignore this I think later, when distros are equipped with this patched version of klipper, users will file a bug because of this.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">The main purpose of this patch was to fix the attached bug, namely to prevent klipper from adding only whitespaces to the clipboard history.</em></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If the user copied the text (or code) with tabs and spaces I think
it is okay to display it using special chars, but not to "stumble" the text using the power of simplify. :/</p></pre>
</blockquote>
<p>On May 18th, 2015, 2:30 a.m. CEST, <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;">Okay then, no simplifying by default, but replacing with symobls (only in the visualization, of course, the actual content will not be affected), and with the option to simplify.</p></pre>
</blockquote>
<p>On May 18th, 2015, 7:22 a.m. CEST, <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 am I supposed to add one option to simplfy the text or two options (simplify + allow blank entries)?</p></pre>
</blockquote>
<p>On May 18th, 2015, 7:41 a.m. CEST, <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 for an option to "allow blank entries", but I'm strictly against a "simplify" option. I do not think that klipper should apply magic to copied strings. Given the way how I interact with the clipboard trying to simplify would turn klipper completely unuseable for me.</p></pre>
</blockquote>
<p>On May 18th, 2015, 8:52 a.m. CEST, <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;">The same holds true for me as well. If I'd copy code an every idention would be "simplified" to a single whitespace I'd remove or disable klipper.
So if this is okay I'll implement a "Allow history items to consist only of whitespaces"-option in the config dialog.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Too bad cmake does still not work. There are many (kde)forum entries and it seems to be a dependency problem. I installed every single dependency
mentioned at the building pages.</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;"><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;">Too bad cmake does still not work</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">best come on IRC #plasma and we can look into figuring out what's the problem.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On May 16th, 2015, 11:31 p.m. CEST, 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 May 16, 2015, 11:31 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=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>