[KDE Usability] Review Request 123806: [klipper] Ignore empty / blank entries
Thomas Pfeiffer
colomar at autistici.org
Mon May 18 01:30:50 BST 2015
> On May 16, 2015, 9:37 p.m., 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. :/
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.
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123806/#review80490
-----------------------------------------------------------
On May 16, 2015, 9:31 p.m., Patrick Eigensatz wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123806/
> -----------------------------------------------------------
>
> (Updated May 16, 2015, 9:31 p.m.)
>
>
> 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/7fc52925/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