[KDE Usability] Review Request 123806: [klipper] Ignore empty / blank entries
Patrick Eigensatz
patrick.eigensatz at gmail.com
Mon May 18 07:52:03 BST 2015
> On Mai 16, 2015, 9:37 nachm., Patrick Eigensatz wrote:
> > klipper/historyitem.cpp, line 91
> > <https://git.reviewboard.kde.org/r/123806/diff/5/?file=369605#file369605line91>
> >
> > 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 *m_bSomeSetting* vars inside the klipper class itself.
> >
> > I need some help on how I should access the setting. (Pass it? Check it in the klipper class?)
> >
> > Thank you :)
>
> Thomas Lübking wrote:
> ensure klippersettings.h is included (ie. add it, implicit inclusion is a timebomb) and use KlipperSettings::trimBlankEntries()
>
> "Klipper" is a class and you cannot access a class this way.
> If m_bTrimBlankEntries *was* (don't make it!) a static public member, you could access it via Klipper::m_bTrimBlankEntries (after including klipper.h here)
>
> You're btw. expected to compile the changes.
> Een reviews will easily oversee missing semicolons etc. - compilers do a far better job itr. ;-)
>
> Patrick Eigensatz wrote:
> Ah, I thougth we would load the settings from KlipperSettings:: into Klipper - an instance of the *klipper* 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!
>
> 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 ;)
>
> Patrick Eigensatz wrote:
> Just a small resumé:
> 1) Create an option "NAME" to simplify the strings using QString::simplify()
> 2) This option is enabled by default
>
>
> We only need to find a name for this option.
>
> Christoph Feck wrote:
> 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".
>
> Kai Uwe Broulik wrote:
> Btw I just pushed the highlight feature to the Klipper plasmoid and I don't see why that one should be configurable.
>
> Thomas Pfeiffer wrote:
> >I wouldn't enable this option by default. The default should be pasting what was copied.
>
> Please explain what ordinary users would need extra whitespace at the beginning or end of a selection for.
>
> > 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.
>
> 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.
>
> Christoph Feck wrote:
> > 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.
>
> Patrick Eigensatz wrote:
> I think it is okay to display whitespace characters in a better way like Kai Uwe's Patch does without adding a configuration option.
>
> 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.
>
> *The main purpose of this patch was to fix the attached bug, namely to prevent klipper from adding only whitespaces to the clipboard history.*
>
>
> 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. :/
>
> Thomas Pfeiffer wrote:
> 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.
>
> Patrick Eigensatz wrote:
> Okay.So am I supposed to add one option to simplfy the text or two options (simplify + allow blank entries)?
>
> Martin Gräßlin wrote:
> 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.
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.
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.
- Patrick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123806/#review80490
-----------------------------------------------------------
On Mai 16, 2015, 9:31 nachm., Patrick Eigensatz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123806/
> -----------------------------------------------------------
>
> (Updated Mai 16, 2015, 9:31 nachm.)
>
>
> Review request for kde-workspace, KDE Usability and Patrick Eigensatz.
>
>
> Bugs: 159267 and 192922
> https://bugs.kde.org/show_bug.cgi?id=159267
> https://bugs.kde.org/show_bug.cgi?id=192922
>
>
> Repository: plasma-workspace
>
>
> Description
> -------
>
> [PATCH] plasma-workspace: klipper: Fix #192922 Ignore blank entries
>
> QString::isEmpty() is used to check if the string only consists of whitespace characters. If it does, the creation of the HistoryStringItem fails.
>
>
> Diffs
> -----
>
> klipper/generalconfig.ui f513e9c
> klipper/historyitem.cpp 36cbe61
> klipper/klipper.h 6952b11
> klipper/klipper.cpp 798b49f
> klipper/klipper.kcfg a03dd16
>
> Diff: https://git.reviewboard.kde.org/r/123806/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Patrick Eigensatz
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20150518/3a1995f2/attachment.htm>
-------------- next part --------------
_______________________________________________
kde-usability mailing list
kde-usability at kde.org
https://mail.kde.org/mailman/listinfo/kde-usability
More information about the kde-core-devel
mailing list