Differential D12674

Mark `Phonon4Qt5` dependency as optional in CMakeLists file
AcceptedPublic
Actions

Authored by ntninja on May 2 2018, 10:07 PM.

Details

Reviewers
kde-frameworks-devel
apol
Summary

Usage of Phonon is already optional in the source code; this commit updates the CMake file to reflect this. All tests pass with Phonon missing.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
ntninja created this revision.May 2 2018, 10:07 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 2 2018, 10:07 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
ntninja requested review of this revision.May 2 2018, 10:07 PM
krop added a subscriber: krop.May 3 2018, 8:19 AM
krop added inline comments.
CMakeLists.txt
68

there are no component, why do you use this keyword ? removing 'REQUIRED' was enough

ntninja added inline comments.May 3 2018, 6:59 PM
CMakeLists.txt
68

I added this keyword because Qt5TTS has it too. Anyways, how do I *update* a patch on this thing? Do I just use “Abandon Revision” from the drop-down below and start over?

ltoscano added a subscriber: ltoscano.May 3 2018, 7:33 PM

If you want to update the patch on phabricator, just apply it locally with arc patch Dxyzt, amend the git commit as usual (maybe also rebase on master if needed), and use arc diff again.

ntninja updated this revision to Diff 33575.May 3 2018, 8:11 PM

I used the web interface and I wasn't able to find the „Update Diff“ button (why not update patch or update commit?) in the right box until now. It should be fixed now.

ntninja added a comment.Jul 27 2018, 11:29 PM

Could I get a new review on this please!?

apol added a subscriber: apol.Jul 28 2018, 8:35 PM

You didn't specify for which repository this is.

ntninja set the repository for this revision to R289 KNotifications.Jul 29 2018, 4:06 PM
Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. · View Herald TranscriptJul 29 2018, 4:06 PM
ntninja added a comment.Jul 29 2018, 4:07 PM

Oh, I see! This wasn't obvious to me. Is this better now?

ntninja updated this revision to Diff 46035.Nov 22 2018, 7:54 PM

Updated to latest KF5 version and probably fixed the issue mentioned in review – although I have no idea what it was.

apol accepted this revision.Nov 22 2018, 8:04 PM

Makes sense to me, it's even already if'd!

This revision is now accepted and ready to land.Nov 22 2018, 8:04 PM
cfeck set the repository for this revision to R289 KNotifications.Nov 22 2018, 9:10 PM
davidedmundson added a subscriber: davidedmundson.Nov 23 2018, 8:12 AM
davidedmundson added inline comments.
CMakeLists.txt
79

What about this comment?

ntninja added inline comments.Nov 23 2018, 10:23 AM
CMakeLists.txt
79

Well, it isn't actually required and it compiles just fine with both canberra and phonon missing.

Revision Contents
Changeset List

PathPackages
MCMakeLists.txt (3 lines)

Diff 46035

View Options

CMakeLists.txt

Loading...