Review Request 123806: [klipper] Ignore empty / blank entries

[Christoph Feck]

> 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.

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".


- Christoph


-----------------------------------------------------------
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/20150517/4356916d/attachment.htm>