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

[Thomas Lübking]

> On Mai 21, 2015, 5:02 nachm., Patrick Eigensatz wrote:
> > klipper/klipper.kcfg, line 24
> > <https://git.reviewboard.kde.org/r/123806/diff/7/?file=370396#file370396line24>
> >
> >     **Question:** Compiled plasma-workspace using *kdesrc-build --include-dependencies plasma-workspace*, applied patch and compiled again.
> >     -> Build broken because g++ could not
> >     locate *klippersettings.h* included in *historyitem.cpp*. Shouldn't this file be generated by *cmake*?
> >     If I generate *klippersettings{.h,.cpp}* using *kconfig_compiler klipper.kcfg klippersettings.kcfgc* and run *kdesrc* again,
> >     I get a build error:
> >     
> >         CMakeFiles/testHistory.dir/__/historyitem.cpp.o: In function `KlipperSettings::allowWhitespaceEntries()':
> >         /home/patrick/kdesrc/plasma-workspace/klipper/klippersettings.h:72: undefined reference to `KlipperSettings::self()'
> >     
> >     
> >     Note: The whole file is full of *return self()->member*, I don't know why it fails on line 72 if it worked
> >     the for last 20 members. (I didn't change anything, original kconfig_compiler output)
> 
> Thomas Lübking wrote:
>     > Shouldn't this file be generated by cmake?
>     
>     Yes, in the build dir.
>     
>     > I get a build error:
>     
>     That's a linker error. No idea about the actual reason (w/o seeing the source), but kconfig_compiler_kf5 should be used to generate that file.
>     I assume you don't have that tool and that's why the config build failed in the first place?
>     
>     Arch ships it with the kconfig package, your distro might have it in some kconfig-dev?
> 
> Patrick Eigensatz wrote:
>     Do you know why I manually have to generate the files?
>     
>     I'm on Arch and I've already installed the kconfig package, using kconfig_compiler_kf5 it doesn't compile neither.
>     
>         [ 38%] CMakeFiles/testHistory.dir/__/historyitem.cpp.o: In function `KlipperSettings::allowWhitespaceEntries()':
>         /home/patrick/kdesrc/plasma-workspace/klipper/klippersettings.h:73: undefined reference to `KlipperSettings::self()'
>         collect2: Fehler: ld gab 1 als Ende-Status zurück
>         
>     (I'm sorry for german error log)
>     
>     
>     Should I post the source code somewhere? (KDE Paste, Pastebin?)
>     
>     Thank you in advance!
> 
> Thomas Lübking wrote:
>     https://paste.kde.org/ but it's weird enough that you "need" to generate that file yourself - notably if this is only the case after your patch?
> 
> Patrick Eigensatz wrote:
>     Yes, building up-to-date repo works fine. (~ 10 minutes)
>     Applying patch `git am patch.patch`, building failes after 2 minutes.
>     
>     klipper/generalconfig.ui    https://paste.kde.org/pksckllah
>     klipper/historyitem.cpp     https://paste.kde.org/pfh1puwqu
>     klipper/klipper.cpp         https://paste.kde.org/pp8psqys1
>     klipper/klipper.h           https://paste.kde.org/pucpi5ri7
>     klipper/klipper.kcfg        https://paste.kde.org/paemgof0i
>     
>     
>     Generated files using kconfig_compiler_kf5:
>     klipper/klippersettings.h   https://paste.kde.org/pyzsmwvf4
>     klipper/klippersettings.cpp https://paste.kde.org/psldgyqbg
>     
>     
>     
>     Build Log:
>     
>     https://paste.kde.org/piqztg5eu
> 
> Thomas Lübking wrote:
>     > Linking CXX executable testHistory
>     > Linking CXX executable testHistoryModel
>     
>     It's not klipper, it's the unit tests.
>     check the autotest directory, you'll have to add
>     
>     kconfig_add_kcfg_files(testHistory_SRCS ../klippersettings.kcfgc)
>     
>     and
>     
>     kconfig_add_kcfg_files(testHistoryModel_SRCS ../klippersettings.kcfgc)
>     
>     there. This will build create the settings sources, build and link them.
> 
> Patrick Eigensatz wrote:
>     I've added this to the CMakeLists file. It seems as if it would create the config files now, but they seem to be invalid
>     
>         /home/patrick/src/kde/build/plasma-workspace/klipper/autotests/klippersettings.cpp: In statischer Elementfunktion »static KlipperSettings* KlipperSettings::self()«:
>         /home/patrick/src/kde/build/plasma-workspace/klipper/autotests/klippersettings.cpp:21:35: Fehler: »class KlipperSettings« hat kein Element namens »read«
>         s_globalKlipperSettings()->q->read();
>     
>     
>     Please have a look at the build file: https://paste.kde.org/pmptt6bdd

Try adding KF5::ConfigGui (compare parent dir cmakelists) - read() is a function of kconfigskeleton


- Thomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123806/#review80705
-----------------------------------------------------------


On Mai 21, 2015, 5 nachm., Patrick Eigensatz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123806/
> -----------------------------------------------------------
> 
> (Updated Mai 21, 2015, 5 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/20150522/ffe7a71f/attachment.htm>