Needed for D28608
Details
Details
Diff Detail
Diff Detail
- Repository
- R85 PIM: Kontact Interface
- Branch
- EXPORT_KONTACT_PLUGIN_WITH_JSON
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 24964 Build 24982: arc lint + arc unit
| KF5KontactInterfaceConfig.cmake.in | ||
|---|---|---|
| 6 | You might also consider using https://api.kde.org/ecm/module/ECMSetupQtPluginMacroNames.html now over manual code, given you depend on new enough ECM. | |
Comment Actions
Use ECMSetupQtPluginMacroNames.
Not sure I see the point in writing 6+1 lines in order to generate 3
(or 3+1 vs 3 with less wrapping), but OK :)
Comment Actions
Yes, it makes more sense in projects which use the macro also in the same project. Here I proposed it more for the sake of consistency, and promoting its existance, so others just copying code would learn about it.
| CMakeLists.txt | ||
|---|---|---|
| 43 | You wanted "EXPORT_KONTACT_PLUGIN_WITH_JSON" here :) | |
Comment Actions
Actually, (been some time, only remembered now) the macro ensures that one does not overlook the usefulness to also have CMAKE_AUTOMOC_DEPEND_FILTERS set to help automoc to track the depenedency to the JSON file when used, like with the new macro, as moc should be rerun when the JSON file changed.... ;)
Comment Actions
Ah, changes to the json file not being taken into account, that's indeed a real problem with the old way. You're saying the new way fixes that?
Seems so indeed. Great!
Comment Actions
"Looks" okay to me from pure code reading for what ecm_setup_qtplugin_macro_names matters. Cannot test though, no PIM devel setup sadly.
| CMakeLists.txt | ||
|---|---|---|
| 38 | I would group the include(ECMSetupQtPluginMacroNames) with the other includes for consistency. | |
| src/plugin.h | ||
| 67–69 | Perfect would be to document the paramters also :) | |
| 71–80 | What is the purpose of pluginname? | |
| src/plugin.h | ||
|---|---|---|
| 71–80 | Wow, unused. Must be a leftover.... I'll remove it. Well spotted. | |