I made a first patch porting QWebkit to QWebEngine of digiKam core. There are 2 part to port : - welcomepageview : easy. no problem here. - htmlwidget from Geolocation tool : not simple. This last one do not well : - The javascript scan done in background do not work very well. Search tool is broken : try to select an area over the map with CTRL+left mouse button : nothing happen. - The central map view do not show geolocated item from current album. - Same problem with geolocation view from right side bar. Note that items from map search tool are displayed... Strange. This is not homogeneous with the 2 last points... Tests and improvements are welcome... Gilles Caulier Reproducible: Always
Created attachment 99331 [details] patch version 1 first version of patch
Created attachment 100912 [details] portWebEngine.2patch Updated patch. Not yet ready for production use. QWebEngineView crashes with Nouveau driver (I use the NVidia driver for testing). Update items still not perfect, only after clicking in the view.
Created attachment 102076 [details] portWebEngine3.patch Updated patch. Full working Google Maps with QWebEngine now. Problem with Nouveau driver remains. Maik
As i use NVIDIA card GeForce 750 with Nouveau driver, i can check if problem is reproducible here. Q : This problem remain if Qt5 with OpenGL effect is compiled ? Or it's only a dysfunction from QtWebEngine framework ? Gilles
I do not know. KMail with Nouveau driver crashes here as well. It could be due to hardware acceleration. I currently use the NVidia driver. You can enabled for test the line 127 in htmlwidget.cpp. Qt-5.7.0 is minimum for disable WebGL. https://bugreports.qt.io/browse/QTBUG-41242 Maik
MAik, Qt 5.8 have new Chrome engine version included. Perhaps the port will be better with this new release of Qt WebEngine. See release announcement for details : https://wiki.qt.io/New_Features_in_Qt_5.8 Gilles
Created attachment 105300 [details] portWebEngine4.patch Update patch to current git/master. Maik
Maik, I will include this patch in git /master with a dedicated compilation flag in cmake. Like this we will able to deploy an experimental version as AppImage soon... Gilles
Maik, The patch version 4 include minimal changes in backend-googlemaps-js Javascript file, which sound like to be compatible with QWebView and QWebEngine. Do you confirm that file can be patched as well without any side effect for QWebView ? Gilles
No, the file only works after the patch with QWebEngine. QWebEngine does not support read status bar messages anymore. Everything now runs over Javascript messages. Maik
So you mean that we need 2 javascript files, one for QWebView, one for QWebEngine ? If yes, the googlemaps backend from geolocation code need to load right file depending of cmake configuration. Typically, as i can see in backendgooglemaps.cpp::line 208. Gilles
Created attachment 105493 [details] updated patch with git/master code. no changes introduced...
Yes, we need 2 files. Maik
The backend-googlemaps-js.js is loaded by the backend-googlemaps.html. We need a second backend-googlemaps.html and load into backendgooglemaps.cpp the right file. Maik
Maik, Can you update the patch with last current changes from git/master about GeoIface namespace which have been dropped from digiKam core ? No need to have QWebKit and QWebEngine support. The last one must be enough and is more powerful than QWebkit. To be honest, I see the code from this framework, mostly written by Apple, and it don't very well designed. I understand with Qt team switch to QWebEngine forever... Gilles
Created attachment 106199 [details] portWebEngine6.patch updated patch with git/master code. Maik
Ok thanks i test this evening and i give you a feedback Gilles
Maik, I tried the GoogleMaps view with your patch. I found a dysfunction (i don't tested all possibilities yet). It's simple : GoogleMaps work everywhere, excepted on central view when you switch from thumbs view to map view. The view is a google html view as expected but without the map tiles. There is no error on the console. In fact the dysfonction appear only if you is centered on north pole. Switching to marble map, panning a little bit, and go back again to google, fix the problem. Can you reproduce ? Gilles
Gilles, Can you make a screencast? Which graphic driver do you use? I can not reproduce it. Maik
I can reproduce something, it is possible to move the map down out. Then it is no longer visible. Maik
The graphic card is a Virtual Box one. Nothing special here, just a basic one. I tried and to problem is difficult to reproduce in fact. It happen when the coordinates are located to north pole on an image. And it's reproducible on all maps view. And also, it's reproducible with QWebKit engine. So it's not a QWebEngine problem in fact. The drag and drop of image in GPS editor work perfectly with GoogleMaps and QWebEngine. Good point. On Search Map view, the region selection tool do not work with GoogleMap and QWebEngine, but it's fine with Marble. The region is draw, but when selection is completed, nothing happen. Can you reproduce ? Excepted for this dysfunction, all work as expected. Gilles
MAik, I can confirm that QWebEngine+Mapsearch tool is broken is selection mode. I reverted to QWebKit, and it work fine. Gilles
I just compile again because of a big update of my distribution. But the region selection tool works very well. I will take a look at it. Maik
Created attachment 106207 [details] portWebEngine7.patch The problem was in the selectionHasBeenMade() signal (GeoIface -> Digikam). Maik
Maik, I just tested. It work better now. Please commit the patch, we will continue to finalize and hack with git/master until 5.7.0. We have plenty of time while this summer... Gilles
Git commit a2cbf0bbf27793f9d83ff86917e6301b3842eb15 by Maik Qualmann. Committed on 21/06/2017 at 10:21. Pushed by mqualmann into branch 'master'. apply patch #106207 to port QWebKit to QWebEngine FIXED-IN: 5.7.0 M +1 -1 CMakeLists.txt M +2 -1 NEWS M +3 -3 app/CMakeLists.txt M +28 -5 app/views/welcomepageview.cpp M +22 -2 app/views/welcomepageview.h M +5 -15 data/geolocation/geoiface/backend-googlemaps-js.js M +2 -1 utilities/geolocation/geoiface/CMakeLists.txt M +35 -35 utilities/geolocation/geoiface/backends/backendgooglemaps.cpp M +222 -167 utilities/geolocation/geoiface/widgets/htmlwidget.cpp M +44 -14 utilities/geolocation/geoiface/widgets/htmlwidget.h https://commits.kde.org/digikam/a2cbf0bbf27793f9d83ff86917e6301b3842eb15
Some details about the problem seen with QtWebEngine port : 1/ The DK bundles explode. It's increased to 100Mb. QWebEngine need to be optimized at compilation time. There are a lots of possibilities due to the huge dependencies list. Also, QWebkit use 5 .pak files with binary data. It's not a negligible size. Why ? I don't know yet. 2/ QWebEngine use a run time dependency and... dbus. This is really weird. There is a stand alone executable dedicated to preload pages in memory (if i well understand). This make AppImage not really easy to run. I found a solution but i'm not happy with it. It's not really optimum. I'm not alone with this problem. I'm in contact with AppImage lead developer who maintain a CI suite to package application as bundle, and most of Qt5 applications have a problem with this QWebEngine dependency. The quick solution is to remove this dependency and make code optional. This is not acceptable for digiKam. As i understand, a call to Qt to make this framework better compatible with bundle solutions was reported. 3/ There is a crash in QWebEngine when digiKam is closed. It's inside libudev. I don't know why yet... 4/ Marble is not yet ported to QWebEngine and still use QWebkit to render some parts inside. This will cut Marble features, and we don't want to include both framework in the bundles. 5/ The time to compile Qt explode with QWebEngine : 1h36 with QWebkit, 3h15 with QWebEngine. Same VM under Centos 6.8 running on same computer. So i must conclude that we need more time before to use QWebEngine inside digiKam. The best solution is to provide code for both QWebkit and QWebengine and to create a switch between both frameworks at compilation time. QWebkit must be the default one. I propose to make another try with Qt 5.10 and DK 6.0.0 Gilles Caulier
Created attachment 110793 [details] portWebEngine8.patch Updated patch to compile with development/6.0.0. Maik
For some JavaScript operations we need a return value. QWebEngine uses an asynchronous FunctorOrLambda function for it. We are currently using a QEventLoop to wait for the result. It works, but it would be better we implement this more elegantly. Maik
Maik, I saw the lines that used QEventLoop to wait for return from JavaScript functions. But as you said that " we should implement it more elegantly ", I don't really understand your idea. So I thought of 2 options: 1/ We should find another way to implement the waiting loop instead of using QEventLoop. If so, what were the disadvantages of using QEventLoop? 2/ We should find a way to replace the lambda function there so that the codes are more readable. Thanh-Trung Dinh
Thanh, At the moment I have no real idea to solve it "elegantly". It could be a function that collects the results and then allocates the return values to the functions. Waiting is just a bit problematic and could make the geolocation GUI slow with slow internet connection. In principle, WebView now also "waits" for the result internally. I think this has time for later, much later. At the moment I would like to shrink the patch to make it easier to integrate it at compile time: 1. We do not need WelcomePageViewPage::WelcomePageViewPage class. QWebEngineView::page() should be enough. 2. At the moment we need a modified backend-googlemaps-js.js file for QWebEngineView. In the old QWebView, the messages are read via the status bar API. This function is no longer available on QWebEngineView. We send the messages now via the JavaScript console. I think we can do that with QWebView too, so we do not need duplicate code here. 3. There is a BUG (https://bugreports.qt.io/browse/QTBUG-43602) in QWebEngineView that has not been fixed yet. We have to intercept all mouse events via the child widget via an event filter. We can not do anything here. 4. We replace runScript() with setToScript() (no wait, because no return value from Java code) and getFromScript() (wait for return value with QEventLoop). We would not need to make any changes to backendgooglemaps.cpp if all JavaScript calls return a value, a "return true" is enough to leave the QEventLoop. Here then also a change to the backend-googlemaps-js.js file which would be valid for both browser engines. Maik
Maik, About your first point that WelcomePageViewPage class should be deleted, that was right since we can access by QWebEngineView::page(). However, I found that the class was created in order to overload method QWebEnginePage::acceptNavigationRequest(const QUrl &, NavigationType , bool ), so that when a link on the welcome page is clicked, it will open a browser for that link. Since QWebEngineView doesn't support any signal to capture an event linkClicked as QWebView did, overloading that method is the best way to work around. Besides, if we don't open link in another browser, I don't know how to return to welcome page except closing and reopening application. So, I guess we should keep that class? Thanh
Thanh, Yes, that's right, that I did not consider. Of course we keep the class. Maik
Created attachment 111187 [details] portWebEngine_patch Hi Maik, I have created a new patch by shrinking the previous one. In this patch: 1/ I keep the WelcomePageViewPage class as we had discussed. 2/ I didn't modify much the backend-googlemaps-js.js. I kept the 2 js functions used for sending message to old QWebView via status bar, so that this file is still compatible with old QWebView. I added the console.log(...) at the end so that there is no problem either with QWebEngineView. 3/ I understood that if all js functions had a return, we would be able to use only runScript() (in which we would add QEventLoop) as we had used in the old QWebView, and that would help to shrink the patch. However, all functions would become synchronous and we might have worse performance. So, I worked around by using a boolean variable (async) and an "if-else" to turn what we had before (setToScript() and getFromScript()) into one function runScript() and shrink the patch. Besides, it seemed to me that map works much slower with QWebEngineView than with QWebView, didn't it? Thanh.
Git commit 101b1170ef8484df732ccbff235dc28fc91f1a26 by Maik Qualmann. Committed on 28/03/2018 at 20:13. Pushed by mqualmann into branch 'master'. add possibility to use QWebEngine instead of QWebKit with compile flag M +1 -0 bootstrap.linux M +1 -0 bootstrap.macports M +9 -1 core/CMakeLists.txt M +30 -4 core/app/CMakeLists.txt M +3 -0 core/app/utils/digikam_config.h.cmake.in M +8 -3 core/app/views/stackedview.cpp M +7 -1 core/app/views/welcomepageview.cpp C +28 -5 core/app/views/welcomepageview_qwebengine.cpp [from: core/app/views/welcomepageview.cpp - 091% similarity] A +87 -0 core/app/views/welcomepageview_qwebengine.h [License: GPL (v2+)] M +9 -3 core/data/geolocation/geoiface/backend-googlemaps-js.js M +18 -2 core/libs/dialogs/CMakeLists.txt A +228 -0 core/libs/dialogs/webbrowserdlg_qwebengine.cpp [License: GPL (v2+)] A +76 -0 core/libs/dialogs/webbrowserdlg_qwebengine.h [License: GPL (v2+)] M +6 -1 core/libs/widgets/mainview/dxmlguiwindow.cpp M +7 -1 core/utilities/assistants/htmlgallery/wizard/htmlfinalpage.cpp M +18 -2 core/utilities/geolocation/geoiface/CMakeLists.txt M +18 -11 core/utilities/geolocation/geoiface/backends/backendgooglemaps.cpp M +1 -1 core/utilities/geolocation/geoiface/widgets/htmlwidget.cpp M +1 -1 core/utilities/geolocation/geoiface/widgets/htmlwidget.h A +409 -0 core/utilities/geolocation/geoiface/widgets/htmlwidget_qwebengine.cpp [License: GPL (v2+)] C +46 -17 core/utilities/geolocation/geoiface/widgets/htmlwidget_qwebengine.h [from: core/utilities/geolocation/geoiface/widgets/htmlwidget.h - 071% similarity] https://commits.kde.org/digikam/101b1170ef8484df732ccbff235dc28fc91f1a26
(In reply to Thanh Trung Dinh from comment #34) > Besides, it seemed to me that map works much slower with QWebEngineView than > with QWebView, didn't it? I can not say that the Google map works slower with QWebEngine, maybe even faster. Also positive is that the X-Server no longer crashes here with the current free OpenSource Nvidia Nouveau driver. Maik
Maik, any reason to not close this file since patch is applied to 6.0.0 ? Gilles
Yes, we can close it. Some will be simplified again, e.g. WelcomePageView. Maik