build a minimal kproperty on mer/sfos
ClosedPublic

Authored by piggz on Nov 24 2015, 10:18 PM.

Diff Detail

Repository
R13 KProperty
Branch
kproperty-mobile
Lint
No Linters Available
Unit
No Unit Test Coverage
piggz updated this revision to Diff 1385.Nov 24 2015, 10:18 PM
piggz retitled this revision from to build a minimal kproperty on mer/sfos.
piggz updated this object.
piggz edited the test plan for this revision. (Show Details)
piggz added a reviewer: staniek.
staniek requested changes to this revision.Nov 24 2015, 11:09 PM
staniek edited edge metadata.

Adam, thanks. Quite a few comments, quite simple.
Do you plan to work on separating source files and create KPropertyCore/KPropertyWidgets libs in this phase or prefer to commit the current approach first?

CMakeLists.txt
1

Really needed? KF5 has 2.8.12...

38

Not needed I think: it's defined by simple_option() to ON or OFF

41

Let's use src/config-kproperty.h.cmake.

Please refer to implementation of options in kdb.git/src/CMakeLists.txt and kdb.git/src/config-kdb.h.cmake as examples.

44

Shouldn't be needed if we use the modern variant of target_link_libraries().

50

Like above, shouldn't be needed

src/CMakeLists.txt
3

I'd propose KPROPERTY_WIDGETS, ON by default. Having it OFF would be equivalent of what you defined by KPROPERTY_MOBILE.

44

I propose list(APPEND kproperty_LIB_SRCS ....)

124–147

Really not needed for _any_ mobile?

For the widget-less kproperty variant will be obviously useful too.

src/KProperty.cpp
27

If we switch to config-kproperty.h.cmake, we will want to include config-kproperty.h here. See KDb for examples.

But after all, ideal solution could be that we move all the QWidget code to separate source files and create KPropertyCore/KPropertyWidgets libs. Thus: almost no ifdefs.

This revision now requires changes to proceed.Nov 24 2015, 11:09 PM
piggz added a comment.Nov 25 2015, 7:25 AM

After last night's discussion I'll split the libs first. Only problem I see is kproperryfactory methods to get a composed property. Would the factory want to live in core or widgets?

piggz updated this revision to Diff 1395.Nov 26 2015, 8:12 PM
piggz edited edge metadata.
  • Move to Core/Widget split
piggz added a comment.Nov 26 2015, 8:13 PM

This is not a final solution, but is for comment/suggestions about direction. Thx

CMakeLists.txt
1

sfos has 2.8.11

piggz marked 5 inline comments as done.Nov 26 2015, 8:22 PM
In D594#11301, @piggz wrote:

After last night's discussion I'll split the libs first. Only problem I see is kproperryfactory methods to get a composed property. Would the factory want to live in core or widgets?

Good!

The factory can be splited. I think the extra features of factory dependent on QWidget would live in 'widgets' and the rest in 'core'. Can be achieved via polymorphism.

staniek added inline comments.Nov 26 2015, 10:29 PM
CMakeLists.txt
1

OK, look, I wasn't sure why the 2.8.12 was so often set as minimum (see e.g. https://techbase.kde.org/KF5/Getting_Started/Build/For_Beginners and dozens of other places on the wiki)

I almost said OK but then this changelog tells me something:
https://cmake.org/pipermail/cmake/2013-October/056020.html

Especially this is used by KF5 and us:

CMake: New PUBLIC PRIVATE and INTERFACE options for target_link_libraries

I propose that we keep 2.8.12 in master for now. One solution is to detect SF in cmake and allow for 2.8.11 then.

PS: 2.8.12 is used by Marble too, which is being ported to SF. Would you like to ask Friedrich how he dealis with the issue? See hos blog.

staniek requested changes to this revision.Nov 26 2015, 10:47 PM
staniek edited edge metadata.

Good job!

src/CMakeLists.txt
56

Wouldn't kproperty_qt become kpropertycore_qt?
Then kpropertycore_qt.qm will loaded by the lib.

And if KPROPERTY_WIDGET == ON we need to do the same for kpropertywidgets_qt.qm.

63

<nitpick>

I propose KPropertyWidgets not KPropertyWidget. "Widgets" is used by Qt and KF5 not once.

The same for KPROPERTY_WIDGET -> KPROPERTY_WIDGETS.

64

Should be KPropertyWidgets

67

Don't we want to append KPropertyCore here to indicate dependency?

72

We need to split kproperty_INCLUDE_DIRS to kpropertycore_INCLUDE_DIRS and kpropertywidgets_INCLUDE_DIRS and use the variables accordingly in the target_include_directories() calls.

124

TODO: KPROPERTY_WIDGETS

141

TODO: KPropertyCore here and in many places like this.

This revision now requires changes to proceed.Nov 26 2015, 10:47 PM
piggz updated this revision to Diff 1427.EditedNov 29 2015, 4:31 PM
piggz edited edge metadata.
  • Progress porting to Core/Widgets

    Libs now installed in separate cmake targets Headers installed correctly Tested with KReport, libs/headers found ok
piggz marked 5 inline comments as done.Nov 29 2015, 4:34 PM
piggz added inline comments.
CMakeLists.txt
1

friedrich changed the check to 2.8.11.2 ;)

staniek requested changes to this revision.Nov 29 2015, 10:17 PM
staniek edited edge metadata.

Great refactoring! Minor notes.

CMakeLists.txt
1

But in master?

1

OK, smart one. What do you think about this advice?

https://github.com/smurfy/fahrplan/issues/58#issuecomment-31149002

Just asking.

53

We need this only if KPROPERTY_WIDGETS in ON, right?

KPropertyCore.pc.cmake
7 ↗(On Diff #1427)

let's remove "with editor" here

7 ↗(On Diff #1427)

core -> core library

KPropertyWidgets.pc.cmake
7 ↗(On Diff #1427)

typo -> editor

7 ↗(On Diff #1427)

widgets -> Qt Widgets library

src/KDefaultPropertyFactory.cpp
21 ↗(On Diff #1427)

not needed include

src/editors/combobox.cpp
109 ↗(On Diff #1427)

For simplicity I propose to share the same area 'kpr' for both libs. In 99% of cases if we need debug we need it for the entire KProperty framework, right?

src/editors/utils.h
33 ↗(On Diff #1427)

This clearly belongs to KPropertyWidgets, not Core

src/kpropertywidgets_debug.cpp
21 ↗(On Diff #1427)

Perhaps org.kde.kproperty.widgets is not needed if we rename org.kde.kproperty.core to more generic org.kde.kproperty?

This revision now requires changes to proceed.Nov 29 2015, 10:17 PM
staniek added a comment.EditedNov 29 2015, 10:25 PM

+ missing fix:

--- examples/CMakeLists.txt
+++ examples/CMakeLists.txt
@@ -3,11 +3,11 @@ find_package(KF5WidgetsAddons)
 find_package(KF5GuiAddons)
 
 remove_definitions(
     -DQT_NO_KEYWORDS
     -DQT_NO_SIGNALS_SLOTS_KEYWORDS
     -DQT_NO_CAST_FROM_ASCII
 )
 
 set(kpropertyexample_SRCS main.cpp window.cpp)
 add_executable(kpropertyexample ${kpropertyexample_SRCS})
-target_link_libraries(kpropertyexample Qt5::Widgets KProperty)
+target_link_libraries(kpropertyexample Qt5::Widgets KPropertyWidgets)

+ HeadersTest error:

1: /home/jarek/dev/build/kproperty/autotests/headers/app/KPropertyWidgetsFactory_HeaderTest.cpp:1:35: fatal error: KPropertyWidgetsFactory: No such file or directory
1:  #include <KPropertyWidgetsFactory>
1:                                    ^
1: compilation terminated.
1: /home/jarek/dev/build/kproperty/autotests/headers/app/kpropertycore_export.h_HeaderTest.cpp:1:34: fatal error: kpropertycore_export.h: No such file or directory
1:  #include <kpropertycore_export.h>
1:                                   ^
1: compilation terminated.
1: CMakeFiles/HeadersTest.dir/build.make:230: recipe for target 'CMakeFiles/HeadersTest.dir/KPropertyWidgetsFactory_HeaderTest.cpp.o' failed
1: CMakeFiles/HeadersTest.dir/build.make:278: recipe for target 'CMakeFiles/HeadersTest.dir/kpropertycore_export.h_HeaderTest.cpp.o' failed
1: make[2]: *** [CMakeFiles/HeadersTest.dir/KPropertyWidgetsFactory_HeaderTest.cpp.o] Error 1
1: make[2]: *** Waiting for unfinished jobs....
1: make[2]: *** [CMakeFiles/HeadersTest.dir/kpropertycore_export.h_HeaderTest.cpp.o] Error 1
1: /home/jarek/dev/build/kproperty/autotests/headers/app/kpropertywidgets_export.h_HeaderTest.cpp:1:37: fatal error: kpropertywidgets_export.h: No such file or directory
1:  #include <kpropertywidgets_export.h>
1:                                      ^
1: compilation terminated.
1: CMakeFiles/HeadersTest.dir/build.make:350: recipe for target 'CMakeFiles/HeadersTest.dir/kpropertywidgets_export.h_HeaderTest.cpp.o' failed
1: make[2]: *** [CMakeFiles/HeadersTest.dir/kpropertywidgets_export.h_HeaderTest.cpp.o] Error 1
1: CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/HeadersTest.dir/all' failed
1: make[1]: *** [CMakeFiles/HeadersTest.dir/all] Error 2
1: Makefile:83: recipe for target 'all' failed
1: make: *** [all] Error 2
1/1 Test #1: HeadersTest ......................***Failed    1.10 sec

+reminder: bump KPROPERTY_VERSION to 2.97.0

This revision was automatically updated to reflect the committed changes.

@Adam created branch with fixes for the above stuff. Still there are issues with paths - try to enable headers test or build kreport with this kproperty.

So I did not push to master but to 594-piggz.

staniek edited edge metadata.Dec 1 2015, 12:57 AM
staniek added a subscriber: Kexi-Devel-list.