Remove Qt5::Widgets as a dependency
Needs RevisionPublic

Authored by apol on Nov 7 2017, 11:36 PM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
Summary

Make it a private dependency for now, until we can possibly drop it from the
library

Test Plan

Built the rest of projects, including plasma, will send few patches

Diff Detail

Repository
R278 KWindowSystem
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
apol created this revision.Nov 7 2017, 11:36 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 7 2017, 11:36 PM

One more place -> kselectionowner.cpp:37
Dropping can't be done before KF6 :)

From my side a -1. This is not source compatible. Thus I think we need to wait for Frameworks 6 for it.

I want @dfaure to ack/nack this change.

dfaure requested changes to this revision.Nov 9 2017, 6:37 AM

That is indeed source incompatible (apart from the change to the _p.h headers). You can either just add a "TODO KF6" comment, or provide a cmake option for linking to QtWidgets, but it needs to be on by default to keep source compat.

This revision now requires changes to proceed.Nov 9 2017, 6:37 AM
apol added a comment.Nov 9 2017, 11:18 AM

Qt has this kind of changes all the time and I've never seen a complaint.

dfaure added a comment.EditedNov 9 2017, 1:15 PM

Qt doesn't have files like KF5WindowSystemConfig.cmake.in :)

I guess you mean changing #includes in public headers, but this is also about changing the library dependencies, which I have never seen change in Qt.

Now that you remind me that Qt sometimes changes includes in public headers, it reminds me that SC for this case is indeed debatable, if an app was using Foo it was supposed to include foo.h itself, rather than indirectly.

I guess that the same argument could be made about Qt5::Widgets... if you're using a widget you're supposed to link to that, rather than indirectly via KWindowSystem...

This is making me change my mind. Maybe it's OK after all. It breaks SC in a case where "proper" application code shouldn't be affected.

I guess that the same argument could be made about Qt5::Widgets... if you're using a widget you're supposed to link to that, rather than indirectly via KWindowSystem...

This is making me change my mind. Maybe it's OK after all. It breaks SC in a case where "proper" application code shouldn't be affected.

I'm not totally happy with this as @apol showed with the list of reviews he already put up that this causes compile breakage for many products. I don't mind getting rid of QWidget in KWindowSystem - in fact I would even prefer if we could get rid of it completely and was very sad when I realized we missed the 5.0 slot for it. But I think we shouldn't just break existing software. Especially given the short release cycles we have and the encouragement to distros to ship such versions. It would not be good for us if distros cannot rely on our own code to compile with latest frameworks.

Thus I suggest we do this in two steps: introduce a build option which enforces the old behavior and keep this the default. In half a year change the default and with KF 6 remove it completely. This just gives the whole stack more time to catch up and we hopefully don't run into the situations that things break. Oh and kdesrc-build should be adjusted to enable the build option from the start, so that our own code falls on our feet.

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptMay 29 2021, 4:19 PM