Details
Diff Detail
- Repository
- R13 KProperty
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
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... | |
43 | Not needed I think: it's defined by simple_option() to ON or OFF | |
46 | 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. | |
49 | Shouldn't be needed if we use the modern variant of target_link_libraries(). | |
55 | 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. | |
14 | I propose list(APPEND kproperty_LIB_SRCS ....) | |
175 | 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. |
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?
This is not a final solution, but is for comment/suggestions about direction. Thx
CMakeLists.txt | ||
---|---|---|
1 | sfos has 2.8.11 |
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.
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: Especially this is used by KF5 and us:
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. |
Good job!
src/CMakeLists.txt | ||
---|---|---|
25 | Wouldn't kproperty_qt become kpropertycore_qt? And if KPROPERTY_WIDGET == ON we need to do the same for kpropertywidgets_qt.qm. | |
32 | <nitpick> I propose KPropertyWidgets not KPropertyWidget. "Widgets" is used by Qt and KF5 not once. The same for KPROPERTY_WIDGET -> KPROPERTY_WIDGETS. | |
33 | Should be KPropertyWidgets | |
36 | Don't we want to append KPropertyCore here to indicate dependency? | |
56 | 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. | |
175 | TODO: KPROPERTY_WIDGETS | |
192 | TODO: KPropertyCore here and in many places like this. |
- Progress porting to Core/Widgets
Libs now installed in separate cmake targets Headers installed correctly Tested with KReport, libs/headers found ok
CMakeLists.txt | ||
---|---|---|
1 | friedrich changed the check to 2.8.11.2 ;) |
Great refactoring! Minor notes.
CMakeLists.txt | ||
---|---|---|
1 | But in master? | |
1–7 | OK, smart one. What do you think about this advice? https://github.com/smurfy/fahrplan/issues/58#issuecomment-31149002 Just asking. | |
55 | We need this only if KPROPERTY_WIDGETS in ON, right? | |
KPropertyCore.pc.cmake | ||
7 | let's remove "with editor" here | |
7 | core -> core library | |
KPropertyWidgets.pc.cmake | ||
7 | typo -> editor | |
7 | widgets -> Qt Widgets library | |
src/KDefaultPropertyFactory.cpp | ||
21 | not needed include | |
src/editors/combobox.cpp | ||
109 | 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 | This clearly belongs to KPropertyWidgets, not Core | |
src/kpropertywidgets_debug.cpp | ||
21 | Perhaps org.kde.kproperty.widgets is not needed if we rename org.kde.kproperty.core to more generic org.kde.kproperty? |
+ 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
@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.