Details
- Reviewers
rkflx - Group Reviewers
Gwenview - Commits
- R260:a00eecc78507: Make dependency on KActivities optional
Diff Detail
- Repository
- R260 Gwenview
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 1592 Build 1610: arc lint + arc unit
Thanks for the patch.
In https://git.reviewboard.kde.org/r/106687, there is this comment:
- Make libkactivity a mandatory dependency. It is now widespread enough IMO and I prefer to avoid adding too many optional dependencies: it helps reducing the number of possible compilation setups and avoid breaking build in corner-cases.
However, I guess your patch still makes sense since Gwenview is also supposed to run in non-Plasma environments, and code-wise this does not have too much of an impact. Also, Dolphin's Activities support is optional too.
That said, I've got some inline comments:
CMakeLists.txt | ||
---|---|---|
15–16 | This change is probably fine (but see also my other comment). However, it is not really related to the topic of this patch, if I'm not mistaken? If so, please remove it from your patch. | |
15–16 | This looks related to the ECM change. Should this be removed too? See also 171e602fe2e5. | |
app/CMakeLists.txt | ||
86 | Would it make sense to use config-gwenview.h.cmake instead and add the include where necessary? |
app/CMakeLists.txt | ||
---|---|---|
86 |
It's also somewhat likely to forget to duplicate add_definitions when adding a new target_link_libraries elsewhere ;) I think we should be somewhat consistent in how we handle that throughout Gwenview. Besides, Okular and Dolphin are also using what I propose. To me it seems that config-gwenview.h.cmake is used for most of our feature flags (ignoring the counterexamples people mistakenly added for a moment), so let's go with that. |