Make dependency on KActivities optional
ClosedPublic

Authored by volkov on Jul 31 2018, 2:31 PM.

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
volkov requested review of this revision.Jul 31 2018, 2:31 PM
volkov created this revision.
rkflx requested changes to this revision.Aug 1 2018, 9:38 PM
rkflx added a subscriber: rkflx.

Thanks for the patch.

In https://git.reviewboard.kde.org/r/106687, there is this comment:

  1. 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?

This revision now requires changes to proceed.Aug 1 2018, 9:38 PM
volkov added inline comments.Aug 6 2018, 3:41 PM
CMakeLists.txt
15–16
15–16
app/CMakeLists.txt
86

I prefer add_definitions because it's easy to forget to include the header file and this will not lead to compilation errors.

volkov updated this revision to Diff 39229.Aug 6 2018, 7:44 PM

rebased

rkflx added inline comments.Aug 6 2018, 7:45 PM
app/CMakeLists.txt
86

it's easy to forget to include

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.

volkov updated this revision to Diff 39248.Aug 7 2018, 10:48 AM

use config-gwenview.h.cmake instead of add_definitions

rkflx accepted this revision.Aug 7 2018, 5:24 PM

Thanks, LGTM now.

This revision is now accepted and ready to land.Aug 7 2018, 5:24 PM
volkov closed this revision.Aug 7 2018, 9:02 PM