QWebkit to QWebEngine Conversion #2512
Conversation
|
Checks fail because I dunno how to use Jarvis, code works fine with latest pyqt5.qtwebengine |
|
@Dotrar - This is really great. I found this message in the Travis build log:
Perhaps the version of Qt we are using at the moment is too old. (version 5.2.1) QWebEngine didn't exist until later versions of Qt. @ferdnyc - Should we move to QWebEngine now that someone has done the work for it? 😆 |
|
|
||
| # Find position from javascript | ||
| js_position = self.eval_js(JS_SCOPE_SELECTOR + ".GetJavaScriptPosition(" + str(position.x()) + ");") | ||
| self.eval_js(JS_SCOPE_SELECTOR + ".GetJavaScriptPosition(" + str(position.x()) + ");",self.store) | ||
| js_positoin = self.consume() |
There was a problem hiding this comment.
Typo here, should be js_position.
| storeChanged = pyqtSignal(str) | ||
|
|
||
| def store(self,val): | ||
| log.error('storing val {}'.format(val)) |
There was a problem hiding this comment.
This is clearly not an error. log.info() please.
| loop.exec_() | ||
|
|
||
| ret = self._last_js_var | ||
| log.error('returning val {}'.format(ret)) |
| @@ -154,27 +157,51 @@ | |||
| MENU_SPLIT_AUDIO_MULTIPLE = 1 | |||
|
|
|||
|
|
|||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Remove these two blank lines, please — the two above are plenty.
|
|
||
| from PyQt5.QtWebEngineWidgets import QWebEngineView as QWebView | ||
| from PyQt5.QtWebChannel import QWebChannel | ||
|
|
There was a problem hiding this comment.
Please remove the blank lines between these imports.
| self._last_js_var = None | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Please remove all three (extra) blank lines, they're in the middle of the __init__ method.
|
One general question: Do we really need to import |
|
Yeah, fair, I'll fix up the formatting when I get back from my holidays on the 14th. The pull request was an afterthought. And there still is the bug with regards to the timeline issue. Where would one look to fix that up? |
And to be fair, I haven't even tried to run it yet, that was just quick by-inspection stuff. I'll be able to give better impressions once I've tried it. If it works, then... great! Ultimately that's the test. I'm a little worried about two things:
Yeah, that... well....
The JavaScript The spots to worry about, if I'm understanding correctly, would be the ones where JavaScript passes the results of openshot-qt/src/timeline/js/controllers.js Lines 285 to 288 in 9fb791f That third argument becomes openshot-qt/src/windows/views/timeline_webview.py Lines 2493 to 2504 in 9fb791f But here's the thing: As far as I can tell, |
(In fact, |
|
I've gotta say, this is a little scary, when running the code. Even assuming they're all non-issues, I think we've gotta find some way to make them go away. Properties without notify signal logspam |
Yeah, this patch on top of your branch fixes click-to-seek for the playhead, and eliminates any complaints about type conversion: From 89d872cef0c1fdcbceb1eb70996db28add1432f4 Mon Sep 17 00:00:00 2001
From: "FeRD (Frank Dana)" <ferdnyc@gmail.com>
Date: Fri, 11 Jan 2019 01:16:39 -0500
Subject: [PATCH] Don't pass JS time_code to PyQt
---
src/timeline/js/controllers.js | 2 +-
src/timeline/js/functions.js | 2 +-
src/windows/main_window.py | 2 +-
src/windows/views/timeline_webview.py | 8 ++++----
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/timeline/js/controllers.js b/src/timeline/js/controllers.js
index 3f75d53a..220bb1f0 100644
--- a/src/timeline/js/controllers.js
+++ b/src/timeline/js/controllers.js
@@ -284,7 +284,7 @@ App.controller('TimelineCtrl',function($scope) {
// Update GUI with position (to the preview can be updated)
if ($scope.Qt) {
- timeline.PlayheadMoved(position_seconds, frame, secondsToTime(position_seconds, $scope.project.fps.num, $scope.project.fps.den));
+ timeline.PlayheadMoved(position_seconds, frame);
}
};
diff --git a/src/timeline/js/functions.js b/src/timeline/js/functions.js
index e8feb7e7..dd10321f 100644
--- a/src/timeline/js/functions.js
+++ b/src/timeline/js/functions.js
@@ -154,7 +154,7 @@ function secondsToTime(secs, fps_num, fps_den)
var day = day % 7;
var frame = Math.round((milli / 1000.0) * (fps_num / fps_den)) + 1;
- return { "week":padNumber(week,2), "day":padNumber(day,2), "hour":padNumber(hour,2), "min":padNumber(min,2), "sec":padNumber(sec,2), "milli":padNumber(milli,2), "frame":padNumber(frame,2) }.toString();
+ return { "week":padNumber(week,2), "day":padNumber(day,2), "hour":padNumber(hour,2), "min":padNumber(min,2), "sec":padNumber(sec,2), "milli":padNumber(milli,2), "frame":padNumber(frame,2) };
}
// Find the closest track number (based on a Y coordinate)
diff --git a/src/windows/main_window.py b/src/windows/main_window.py
index 2e3a1727..6bda7d33 100644
--- a/src/windows/main_window.py
+++ b/src/windows/main_window.py
@@ -838,7 +838,7 @@ class MainWindow(QMainWindow, updates.UpdateWatcher):
win = Cutting(f, preview=True)
win.show()
- def previewFrame(self, position_seconds, position_frames, time_code):
+ def previewFrame(self, position_seconds, position_frames):
"""Preview a specific frame"""
# Notify preview thread
self.previewFrameSignal.emit(position_frames)
diff --git a/src/windows/views/timeline_webview.py b/src/windows/views/timeline_webview.py
index 10c29d8d..b3d53967 100644
--- a/src/windows/views/timeline_webview.py
+++ b/src/windows/views/timeline_webview.py
@@ -2520,8 +2520,8 @@ class TimelineWebView(QWebView, updates.UpdateInterface):
# Seek to frame
self.window.SeekSignal.emit(frame_number)
- @pyqtSlot(float, int, str)
- def PlayheadMoved(self, position_seconds, position_frames, time_code):
+ @pyqtSlot(float, int)
+ def PlayheadMoved(self, position_seconds, position_frames):
# Load the timeline into the Player (ignored if this has already happened)
self.window.LoadFileSignal.emit('')
@@ -2531,7 +2531,7 @@ class TimelineWebView(QWebView, updates.UpdateInterface):
self.last_position_frames = position_frames
# Notify main window of current frame
- self.window.previewFrame(position_seconds, position_frames, time_code)
+ self.window.previewFrame(position_seconds, position_frames)
@pyqtSlot(int)
def movePlayhead(self, position_frames):
@@ -2818,7 +2818,7 @@ class TimelineWebView(QWebView, updates.UpdateInterface):
# Find position from javascript
self.eval_js(JS_SCOPE_SELECTOR + ".GetJavaScriptPosition(" + str(position.x()) + ");",self.store)
- js_positoin = self.consume()
+ js_position = self.consume()
# Loop through clips on the closest layer
possible_clips = Clip.filter(layer=closest_layer)
--
2.20.1(Also attached): ETA: Corrected, I was missing a chunk the first time. |
I'm not sure that's it... (a) I'm not sure that travis is actually using Qt 5.2.1 (the AppImage builds are, but those are still done at gitlab, maybe?); and (b) I'm pretty sure QtWebEngine existed in 5.2.1, at least in "technology preview" mode. I'm pretty sure the issue is that QtWebEngine simply isn't installed in the travis environment. Both the QtWebKit and QtWebEngine Python modules are typically packaged separate from the rest of PyQt, and the travis environment will only have what's installed during setup. Which would be: Lines 4 to 11 in 9fb791f So I think something like |
|
@Dotrar : If you're comfortable giving me commit access to your fork, I can commit my patch above to fix up that string-conversion issue, and try to unbreak travis for the PR. Up to you. |
|
Well, that didn't work. Hmm, there must be documentation on this environment somewhere... |
|
Wow, you've gotta be kidding me. Not only is QtWebEngine apparently not in Ubuntu Trusty (the current build environment, which as @DylanC correctly noted is still running Qt 5.2.1), but it's apparently not in Ubuntu Xenial (Travis's newer option) either! It doesn't show up until Ubuntu Bionic (18.04)! Presumably there's a way to add it to at least Xenial, and possibly even Trusty, via third-party PPA, but what a crapfest. #FrickinUbuntu |
|
OK, this PPA contains Qt 5.8 for Trusty, complete with QtWebEngine — but no PyQt. Still, one step closer... |
|
So, qutebrowser is "A keyboard-driven, vim-like browser based on PyQt5." which supports QtWebEngine and uses Travis CI. AFAICT what they're doing is building inside a Docker image (actually, multiple docker images), including one that's based on arch (instead of any Ubuntu), and includes PyQt5.QtWebEgine. The rest don't use WebEngine, presumably because its availability on Debian/Ubuntu is so spotty and recent. "Fun!" I had actually found, in my travels before I even learned of this, a Docker image which purports to supply "An Ubuntu Xenial based Qt 5.7 + QtWebEngine + Python 2.7 + PyQt5 container" (which is great except for the Python 2.7 part)... but it doesn't appear that there are any available builds of that container. Anyway, I'm still exploring Docker, since I've never used it before today but now at least it's installed on my local machine. |
|
@ferdnyc -
I found Qt 5.2.1 mentioned in the logs and I'm aware its the default for Trusty because I recently had to make my build run on Trusty for wide AppImage compatibility on my own project. I remember during development (with my own project) I went from Qt 5.2.x to 5.12. During Qt 5.4 I was using a webkit based tool to show available updates. I remember the tool needed converting to QtWebEngine but QtWebEngine was not widely available. The dependency on this Qt web module became so annoying I just dropped it altogether. |
|
@Dotrar I've added an official branch called webengine-convert. Would you mind changing this PR to point at that branch instead of develop. This will allow me to run a bunch of tests with all our build servers, and see what breaks. 😄 |
|
Also, looks like we have a conflict now, since timeline_webview.py has been updated on the |
|
Alright, I've returned from holidays and I'll have a look at this tonight. |
|
I can't dev for the time being because of the switch over to "Settings" in #2520 |
|
Ok got the PPA to work, I'll resume work on this now. |
|
@jonoomph Thanks for the branch! :D The latest changes to
the very first line of that mentions the need for the webchannel.js. but I mean, I'm not in love with the file, so if you find a way to get rid of it, by all means. It opens the WebChannel creation
The "Property XYZ" logspam is actually from the QT object. The only way to remove it, that I could think of, would be to have the
I'm not sure if it's an issue and I'm not sure how the old way worked. Could there be issues? theoretically yes. Should we be worried? Not for the time being. Most of the calls are very simple "get value" from the JS engine; if the JS engine takes a long time to do those, then I think there are bigger issues at play.
Part of the problem in keeping old software around is no one wants to be the first on the new version, which means things are built on older versions, and the cycle continues :P |
True, but I was more focused on the very next sentence, which is: "For clients run inside Qt WebEngine, you can load the file via I'm just not totally clear on how to actually apply that in practice, since right now it's being loaded in Aha! Or, from their own example code: QFileInfo jsFileInfo(QDir::currentPath() + "/qwebchannel.js");
if (!jsFileInfo.exists())
QFile::copy(":/qtwebchannel/qwebchannel.js",jsFileInfo.absoluteFilePath());That's... pretty freakin' goofy, actually. But it would allow us to avoid including the file, and it'd mean that any updates or bugfixes in newer versions of QTWebEngine would automatically be picked up. I suppose we could copy ...Also, that's their "standalone" example, which runs the client in a web browser — so, the file definitely has to be available externally. We don't even want to do that. Since we DO want to run everything inside QtWebEngine, seems like there may still be a way to use the resource path directly. But the standalone example is also the only example, annoyingly... so if that is possible, it seems they're not going to make it too easy to figure out how. Anyway, I'll keep poking around. Not a big deal either way.
(...Condensed to address a single set of related points) Understood. And those are the sort of things I was getting at...
It seems like, with the right properties connected up between the Python and JS sides, the data the Python side needs chould be available to it without having to query it synchronously from the JS side. And passing data to the JS side should ideally just be a matter of updating the Qt properties in Python, then firing off an async "refresh" to render the changes. No need for callbacks, no need for sync queries. ...But those are all bigger issues that don't need to be addressed in this PR, and they're all issues that very much exist in the existing QtWebKit code as well. (In fact, to some extent they probably exist because of QtWebKit.) The conversion to QtWebEngine gets us on the road to being able to address them, which is the important thing. Hopefully we can sort out availability of the module universally enough, it'd definitely be great to have. Thanks again for putting all the effort into this. |
|
Maybe not a right place to ask, but is anybody has thoughts to make your own timeline widget in Qt? And get rid of all web viewing functions by single move? Like https://github.com/csuft/VideoTimeline (just an example!) or so? |
|
@DylanC , do you afraid that it fails in some step? Or really it requires too much effort to spend it on the solution that will be almost not visible to end user? I understand that Timeline is like 50% of video editor success. And @jonoomph and others spent a lot of time to improve it (it still has number of bugs, but at least it works). |
@SuslikV - Well the current solution does work. A new timeline would be far better but its hard to justify its development since it would be such a huge project of its own really. To most users all this time spent will just give them the same functionality. To more advanced users or developers its a great change for the project.
@SuslikV - I would say I do have the interest but I do not come from a python background, so maybe not the right skills. I have implemented changes for the project previously (for v2.4.3) but on much smaller changes. Time is something I do lack right now though. I have a busy life at the moment and I run my own project too. As I said I am interested in this but interest without time or possibly skills wouldn't really be good enough to get it done. |
|
@ferdnyc - What is the status of this? Are we to reject the PR or merge in the future? |
|
I'd still like to find a path other than "reject', because it's potentially very useful. We're light years away from even considering a merge, but I'd like to see it made available for more testing, and if possible by more people. I suppose there are two ways of going about that:
The first option would also potentially enable @jonoomph to set up an additional Daily Build stream for the code on that branch, if it ever progressed that far — you know, assuming we ever managed to successfully convince the CI farm to build it in the first place. (Baby steps.) The second option would require that we keep the two I still just think there's good stuff here that I'd hate to reject outright, but at the same time committing to a switch from QtWebKit to QtWebEngine isn't even on the horizon, when we're still not even able to build OpenShot with it on all of our build platforms, currently. Oh, and then there are other things, like this from a while back in the conversation:
I just checked, and that's all still true, and I have no idea why I never bothered to submit a patch to the main repo removing those arguments. They're totally unnecessary, so why would we keep passing them back and forth for no reason? So, I'll put that PR together now, as it makes sense independent of anything to do with this PR. ...As far as this PR goes, given that the requesting branch no longer exists in GitHub I guess we should really consider it withdrawn. |
It's funny you should mention that, as the widget in question is implemented (in its There's really no getting away from web-based rendering even in desktop applications, anymore. (Similarly, Gnome Shell leans heavily on a JavaScript engine for its interface.) It's just a question of different approaches towards integrating the web-based components with the rest of the toolkit. Qt's first two versions of that integration were QWebKit and QWebEngine, and that evolved into their third-generation web based platform QML. (Actually, QML initially evolved alongside them. So we're probably in at least the fourth generation, by now.) The impression I get is that QML is a very natural fit with Qt's native language(s), QtQuick and C++ (which that timeline widget is written in), so it's a no brainer using it there. I'm less sure how well it integrates with Python and PyQt. Riverbank assure us that it works "in exactly the same way", but sound suspiciously like a used car salesman when doing so. (I mean, it only takes them 6 bullet points before they've backpedaled so far that they insert this scary-admonition box):
And the caveats get progressively more scary as you make your way down the rest of that page. As @DylanC said, it's something we've discussed at length in the past. (#1864) However, it never progressed beyond discussions, because as he said the current code is working and replacing it would be a massive task. (I also suspect that the undertaking is assumed to have guaranteed benefits in terms of speed or stability, that in reality would neither be as significant nor as easily achievable as people imagine.) Speaking only for myself, it's a moot point because I'm not currently open to undertaking a project of that magnitude. But, feel free. |
|
@ferdnyc Only you may think to use someones code in this project... I said JUST AN EXAMPLE!!! If you can't write your own widgets in Qt - you shouldn't modify this part of OpenShot completely. The idea behind this was exactly to get rid of the |
|
@SuslikV I realize the code was "JUST AN EXAMPLE!!!" — and as I pointed out, it was an example of a JavaScript-based, QML-and-C++ widget, a very common structure in recent Qt development.
That is all incorrect. Most of those libraries are required for any Qt GUI development, they are not specific to Qt5WebKit or Qt5WebKitWidgets at all.
Again, simply not correct. |
|
@SuslikV I do not think that you and I should have any further discussions regarding application development. They are not productive for either side. |
|
Closing this as withdrawn, with the original source repo deleted and nobody to work on it, it's unlikely to progress. |
this closes
#2144
#2057
plus another bug that I've lost the reference to.
it was easier to fix this problem than to fix my computer telling me:
python3-pyqt5.qtwebkit is already the newest version (5.11.3+dfsg-1+b3)Converts QWebkit to QWebEngine; as per what is recommended.
Only 1 known problem with this patch, I don't know where
secondsToTimeis called in the QT code, and there was a bug with ability to convert QJson -> QString, so I edited it in functions.js instead, it seems the only thing that had problems with that wasruler.js; - if someone wants to fix this up.By all means, it works completely as expected. QtWebkitWidgets isn't coming back. The fact that it's still there in some distrubitions isn't to be abused because it won't be there forever.
Regards.
What works:
What doesn't work:
/timeline/js/directives/ruler.js:179