Differential D5041

Port away from QtWebKit to QtWebEngine
ClosedPublic
Actions

Authored by apol on Mar 14 2017, 1:51 AM.

Details

Reviewers
mwolff
kfunk
Group Reviewers
KDevelop
Commits
R33:2486228ccee2: Port away from QtWebKit to QtWebEngine
R32:2486228ccee2: Port away from QtWebKit to QtWebEngine
Summary

QtWebKit is unmaintained

Test Plan

manual testing

Diff Detail

Repository
R33 KDevPlatform
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol created this revision.Mar 14 2017, 1:51 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptMar 14 2017, 1:51 AM
antonanikin added a subscriber: antonanikin.EditedMar 14 2017, 2:00 AM

Hi, Aleix. This patch seems to be incorrect - on my kubuntu 16.04 with kde-backports (qt 5.6.1) it's unable to find Qt5WebEngineWidgetsConfig.cmake

https://bugs.launchpad.net/ubuntu/+source/qtbase-opensource-src/+bug/1579265

mwolff requested changes to this revision.Mar 19 2017, 1:19 PM
mwolff added a subscriber: mwolff.

QtWebKit is actually more alive nowadays than in a long time, so I still think the best way forward would be to wrap it in a thin KF5-material wrapper and use that here

This revision now requires changes to proceed.Mar 19 2017, 1:19 PM
apol updated this revision to Diff 12628.Mar 20 2017, 2:06 AM
apol edited edge metadata.

abstract out the webkit view

mwolff requested changes to this revision.Mar 20 2017, 9:12 AM
mwolff added inline comments.
documentation/CMakeLists.txt
5

I think this should be:

if (NOT PREFER_QTWEBKIT)
... // find webengine
endif()

if (NOT QT5WEBENGINEWIDGETS_FOUND)
... // find webkit
endif()

or?

documentation/standarddocumentationview.cpp
42

all of this code here is now lost, don't we need it anymore?

This revision now requires changes to proceed.Mar 20 2017, 9:12 AM
apol updated this revision to Diff 12637.Mar 20 2017, 11:43 AM
apol edited edge metadata.

Leave the code that workarounds integration issues in QtWebKit

apol added a comment.Mar 20 2017, 11:26 PM

@mwolff I think it's fine like this, I wouldn't want people compiling against qtwebkit because they forgot to pull a dependency, it should be a conscious decision.

kfunk added a subscriber: kfunk.Mar 21 2017, 7:57 AM
kfunk added inline comments.
documentation/CMakeLists.txt
31

PREFER_WEBKIT -> USE_WEBKIT.

This is not about preference. There's actually no other choice :)

kfunk added a comment.Mar 21 2017, 7:59 AM
In D5041#96472, @apol wrote:

@mwolff I think it's fine like this, I wouldn't want people compiling against qtwebkit because they forgot to pull a dependency, it should be a conscious decision.

I agree with Milian here. There's still several systems where QtWebKit is the default. Let's not make it more difficult to build KDevelop on these. We don't care about whether we use WebKit or WebEngine. It's up to the packagers.

You could add a note to the package properties text of WebKit telling that WebEngine is preferred, though.

apol updated this revision to Diff 12665.Mar 21 2017, 2:26 PM

Fallback to qtwebkit if there's no qtwebengine

kfunk accepted this revision.Mar 21 2017, 2:47 PM

We'll probably need to tweak the CMake code a bit more.

IIUC, right now if neither WK nor WE are installed, CMake will just tell you to get WK. It should give you both options.

documentation/CMakeLists.txt
5

FYI: That basically renders TYPE REQUIRED redundant. As Qt5WebEngineWidgets is already found here.

Closed by commit R33:2486228ccee2: Port away from QtWebKit to QtWebEngine (authored by apol). · Explain WhyMar 21 2017, 10:27 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 12673

View Options

CMakeLists.txt

Loading...
View Options

documentation/CMakeLists.txt

Loading...
View Options

documentation/standarddocumentationview.h

Loading...
View Options

documentation/standarddocumentationview.cpp

Loading...