Add initial Qt WebEngine support #1542
Conversation
vedgy
force-pushed
the
webkit-or-webengine
branch
2 times, most recently
from
6375c05
to
3bd22b2
Compare
vedgy
force-pushed
the
webkit-or-webengine
branch
2 times, most recently
from
5a0fb68
to
eafd467
Compare
vedgy
force-pushed
the
webkit-or-webengine
branch
from
4503e2e
to
372e0e3
Compare
vedgy
force-pushed
the
webkit-or-webengine
branch
2 times, most recently
from
57c5d56
to
7215e83
Compare
|
Hi @Abs62. Apparently no one is particularly interested in testing this pull request. Could you please review it or the first 4 commits in #1552? I can only guess what blocks the review... Are you concerned about regressions when building against Qt 4.6-4.7? Are you waiting until more |
vedgy
force-pushed
the
webkit-or-webengine
branch
from
7215e83
to
83a79a8
Compare
|
Don't know what's keeping the merging but this and xiaoyifang's fork are the only ways to run gd on a modern system that dropped WebKit. xiaoyifang, however, breaks other features and basic functionality (like dict indexing format and pop-up on global hotkey) while refusing to fix it, so this is the only viable solution for distro packaging. Tested it now and offline dicts seem to work and look like the original. In fact, the only improvement for me would be it following system's dark palette (light fonts on dark background) but this is not in the original either. |
Thanks for testing!
What are these modern systems? The only such distro I know of is Gentoo and possibly its derivatives. Building Qt5 WebKit manually, e.g. using the dropped package sources, should be easy to a Gentoo user. So this should be more like inconvenience and wasting time rather than impossibility. Though using WebSite dictionaries in the Qt WebKit version could be a significant security risk, seeing as Qt WebKit does not receive any meaningful security updates. |
openSUSE dropped it for a while, i think, and tried pushing Qt6 but now it returned into main repo (it is inconvenience but a big fat crusty one, so it's nice not to have it). And there is not much point in moving all Qt apps to Qt6 until KDE does. xiaoyifang's fork does seem to work with Qt6 the same way, by the way, haven't tested this one because that way it only stuck with Qt's built-in Fusion theme. |
I do not think I have changed that. |
|
This pull request is stuck supporting Qt 4, so Qt 6 support is absent for the time being. |
vedgy
force-pushed
the
webkit-or-webengine
branch
2 times, most recently
from
52c890b
to
c7fc5e0
Compare
vedgy
force-pushed
the
webkit-or-webengine
branch
from
c7fc5e0
to
e3123c8
Compare
vedgy
force-pushed
the
webkit-or-webengine
branch
from
e3123c8
to
a64d1bb
Compare
vedgy
force-pushed
the
webkit-or-webengine
branch
from
d725e2a
to
b7e0452
Compare
vedgy
force-pushed
the
webkit-or-webengine
branch
from
d4565f1
to
6d0e91f
Compare
|
What is preventing this from being merged? That fork, despite significant issues, is gaining popularity. Merging this would reduce the motivation for some people to adopt the fork. Also, is there any way the fork can be forced to be renamed to prevent confusion with this project? Based on comments written elsewhere, it seems the developer/users of the fork are attempting a hostile takeover of goldendict. |
Only one maintainer has been active in the last 10 years. Either other maintainers return to GoldenDict development or @Abs62 reviews this pull request. I guess @Abs62 doesn't review this PR for one or both of these reasons:
To begin with, GoldenDict maintainers would have to ask the fork developers to rename. I don't expect the single active maintainer to do that any time soon. |
The fork appears to introduce many more new bugs than it fixes. The new data races are much more difficult to fix than the old bugs. Also I am pretty sure there are many Qt-WebEngine-porting-related bugs in the fork, which are absent from this pull request due to much more careful development. |
|
Have you ever looked at those diffs and the review comments? The reviews got nowhere, because @xiaoyifang couldn't even bother to reread and clean up the diff he proposed to merge. My impressions stem from all the times I reviewed @xiaoyifang's commits in order to borrow something. I had to implement almost everything differently, because there were no clean, bug-free commits to borrow. It's clear that our opinions on the fork's quality differ substantially. This pull request's comments is not the place for bickering, which will achieve nothing. Any interested C++ developer can review the changes and make his/her own conclusions. |
No, it's not. I already said that I had to abandon the fork because it broke my config and the dev refused to even acknowledge their broken-by-design format change. Besides, anyone with any experience can immediately tell from looking at codebase that the fork does not pass basic quality control rules for code maintenance and CVS commit formation, like as if it's some kind Google, W3C/WHATWG or Khronos project. Which is a shame because their effort could be directed into constructive way with proper practices. |
Add a new custom CSS property --gd-page-background-color into built-in
CSS files and require this property to be specified in all user-defined
CSS files, except for print-only ones. Show a UI warning message at each
GoldenDict start if the variable is unspecified in one or more CSS files
or if the color value is invalid. The prominent and detailed warning
message should annoy most users into adding the CSS property to their
custom style sheets in short order.
Even though the current code reads CSS files line by line until the CSS
property specification is found, and the property is specified close to
the top of each CSS file, the overall size of a CSS file still
determines the search time. The time spent in QFile::open() depends on
the file size and dominates the search time for some reason. Neither
QIODevice::Text nor QIODevice::Unbuffered flags passed to QFile::open()
have a noticeable impact on this time.
The search for page background color was and remains slowest when the
Default display style is configured. That's because article-style.css is
the largest of the article CSS files, and all built-in styles specify
the page background color; so when another built-in style is configured,
article-style.css is never opened.
This commit reduces the longest search time on my GNU/Linux system from
about 15 ms to less than 0.4 ms.
I have tried an alternative method of finding the page background color:
1. Embed a script into articles right after appending CSS style sheets.
2. The script retrieves the page's background color with this code:
window.getComputedStyle(document.documentElement).backgroundColor
3. The script sends the result to ArticleView, which forwards it to
MainWindow.
4. MainWindow compares the received background color with a previously
stored one. If the colors differ, the new color is stored and passed
to each existing article view, as well as each article view created
in the future.
5. Each ArticleView passes the updated background color to
QWebEnginePage::setBackgroundColor().
Important advantages of this alternative method are:
1) it is accurate and reliable;
2) does not require changing user-defined article styles.
However, the alternative method also has significant drawbacks:
1. If the "Start to system tray" option is off (the default), a white
background flash appears in the initial Welcome! page, because the
page background color is determined and set after the page becomes
visible.
2. Computing the document element's or the body's style in JavaScript
takes more than 2 ms on average, which is 5 times slower than the
worst-case scenario at this commit.
3. The CPU time waste is even worse if the background-color-computing
script is embedded into each page. Embedding the script only in the
initial Welcome! page and possibly into blank pages causes background
flashes when a different Display style is selected or a user-defined
article style is modified while GoldenDict is running.
In the end I decided against the alternative method, because its
drawbacks are permanent. Its advantages exist only until user-defined
styles are adjusted, and after that - only while the styles are being
tweaked, which shouldn't happen often.
This way no persistent web data or web cache is stored on disk by default, just like in the Qt WebKit version. The new default is consistent with GoldenDict's clearing of network cache on exit by default. And the same justifications apply: most users probably don't load the same online articles after restarting GoldenDict; storing the persistent web data and web cache on disk indefinitely by default would be a new and unexpected to the users privacy risk (compared to the Qt WebKit version).
Requesting a new translation already hides searchFrame or ftsSearchFrame (whatever is visible), because ArticleView::showDefinition() calls closeSearch(). But going back or forward in web page history does not hide any search frame. Keeping the regular search frame visible might be useful to peek into a previous page, then go forward and resume searching. Keeping ftsSearchFrame visible does not make sense and does not work correctly. It has no resume-search benefit either, because going back or forward to the FTS search page hides then shows ftsSearchFrame anyway. Note: going back or forward from one FTS search page to another FTS search page hides then shows ftsSearchFrame. This scenario worked correctly even before this commit. The Qt WebKit version printed the following errors when Next or Previous FTS search button was pressed on a non-FTS-search page: JS: ReferenceError: Can't find variable: sr_5625c81edc70 (at undefined:1) JS: IndexSizeError: DOM Exception 1: Index or size was negative, or greater than the allowed value. (at undefined:1)
Correct vertical scroll position is now restored when the user goes back to a page where searchFrame was visible.
If the looked-up word does not change, the searched-for text is likely still relevant.
Before this commit, only closing the inspected tab or destroying the inspected scan popup (e.g. by opening Dictionaries or Preferences configuration dialog) saved the inspector geometry.
const_cast is error- and undefined-behavior-prone. The title "Don’t cast away const" is among C++ Core Guidelines.
Including the inspected page's title in a dev. tools view's title helps to distinguish among multiple open dev. tools views. In the Qt WebKit version the web inspector's title includes the inspected page's URL rather than title. In the Qt WebEngine dev. tools view an editable URL of the inspected page is already present right under the window title, so no point in repeating it.
The Qt WebEngine version is quite usable now, if one doesn't need any of the missing features.
The code didn't take this possibility into account before. It is very easy to activate an article that is being loaded in the Qt WebEngine version: just request a huge MediaWiki article, e.g. "Paris" from English Wikipedia, and click on an empty place within once it appears. Doing the same in the Qt 4 WebKit version is much more difficult: click on an empty place within a huge article just before the article becomes visible. I wasn't able to activate a not-fully-loaded article in the Qt 5 WebKit version at all. The wrong behavior after such an early activation before this commit: * in the Qt WebEngine version no dictionary row is selected in Results Navigation Pane; the target article or fragment is scrolled to; * in the Qt WebKit version no dictionary row is selected in Results Navigation Pane if the activated article is topmost; the topmost article's dictionary row remains selected in Results Navigation Pane otherwise.
The new order looks more logical and simplifies the code.
[Un]setting QWebEngineView's cursor does not affect the actual mouse pointer appearance. [Un]setting its widget child's cursor makes the mouse pointer look the same as in the Qt WebKit version. Don't fall back to hidden QWebEngineView's equivalents in the added cursor member functions if childWidget is null. If QWebEngineView's cursor is set to Qt::WaitCursor before childWidget becomes nonnull, and childWidget's cursor is "unset" after childWidget becomes nonnull, the QWebEngineView's cursor will remain WaitCursor indefinitely. A visible effect would be: when an article is loaded, the cursor does not restore the default Qt::ArrowCursor shape, Qt::WaitCursor is displayed until the user vigorously moves the mouse pointer. The described issue occurs in practice only if the assignment to childWidget is moved into a lambda invoked by a queued connection in ChildAdded event. But even a possibility of this issue surfacing after a future code change is enough to avoid the fallback.
The implementation of ExtLineEdit::buttonPixmap() is incorrect, because ExtLineEdit::pixmaps is never modified, and so always contains null pixmaps. Fortunately this member function is unused.
The primary motivation for this new feature is to mitigate QTBUG-106580. When a page is not fully loaded, loadFinished() is never invoked. So the loading-state indicator remains visible until a next page replaces this one and finishes loading. The user has a chance to notice the stuck loading indicator, conclude that something went wrong and realize that the page may be incomplete. And then, hopefully, the user would reload the page, e.g. by going back and forward in web history. The loading-state indicator's placement in the UI is dictated by the following requirements: 1) always present in the main window and the scan popup UI, no matter how the user configures GoldenDict's UI; 2) does not waste valuable UI space; 3) does not disrupt any dictionary usage workflows. I have implemented a proof-of-concept animated icon version, but decided against it because of potentially significant CPU time waste. Plus the animation could be distracting. Pages are loaded often enough that the users should quickly learn to recognize the static loading-state icon. I have implemented an alternative mitigation via overriding the application cursor with a busy cursor instead of the current setting of a wait cursor over the article web view. A cursor set via QWidget::setCursor() is replaced with a regular cursor on mouse move. An overridden application cursor stays until restored or overridden again. However I rejected this approach, because it does not work if the user prefers keyboard navigation and does not keep the mouse pointer over GoldenDict windows. The added icon loading.gif is distributed under the GPLv3 license and can be downloaded at https://github.com/shlinux/faenza-icon-theme/blob/master/emesene/themes/Faenza/loading.gif
When the user hovers the mouse pointer over the collapse-article icon while the article is expanded or over the article name while it is collapsed, a "(Collapse|Expand) article" tooltip is displayed. This tooltip creates a temporary QTipLabel child of ArticleWebView. QTipLabel is a private Qt class derived from QLabel. This transitory widget child of ArticleWebView replaces the RenderWidgetHostViewQtDelegateWidget child in the childWidget pointer. Afterwards ArticleWebView's cursor member functions do not update RenderWidgetHostViewQtDelegateWidget's cursor and thus the cursor no longer changes shape to a wait cursor when an article starts loading. Don't look for RenderWidgetHostViewQtDelegateWidget directly, because 5d1ef38f9f6815807596d0606cf7ed06b7930aac replaced this class with WebEngineQuickWidget in Qt WebEngine 6.4. Both the old class and its replacement inherit QQuickWidget. Since GoldenDict does not use Qt Quick directly, I don't expect any more wrong childWidget values with this implementation. quickwidgets Qt module dependency has to be added to the Qt WebEngine version of GoldenDict to use qobject_cast< QQuickWidget * >. This should not cause any issues to end users, because webenginewidgets module itself privately depends on quickwidgets.
The selectWordBySingleClick preference value is common to all pages. Instead of inserting and updating a script that defines the gdSelectWordBySingleClick JavaScript variable in each article web page's script collection, insert and update it in the shared web engine profile's script collection. Name the shared script "ProfilePreferences" to facilitate extending it by injecting other shared preferences in the future.
gdvideo:// links are playable in the Qt WebEngine version, but not in the Qt WebKit version. ArticleResourceReply is a sequential device and thus does not support seeking. Qt WebEngine or Chromium internals attempt and fail to seek in the video device unless instructed to preload the entire video. Force the preloading to prevent errors and allow playing the video. The rest of the article is loaded and displayed before the video is preloaded, therefore this commit still improves performance compared to previous blocking of article loading while getCachedFileName() saves the video file in a temporary directory. The video data is stored inside a dictionary file on disk, which is subject to operating system page cache. Therefore, eliminating the intermediate cache in a temporary directory shouldn't slow down repeated loading of large videos. b10cdf6 introduced the getCachedFileName() workaround and made a widely used in GoldenDict code class Mutex recursive. The primary motivation for this commit is that disabling the workaround allows to make Mutex nonrecursive again in the Qt WebEngine version. This improves overall GoldenDict performance by eliminating the recursiveness overhead. It also eliminates QMutex::Recursive deprecation warnings and paves the way to Qt 6 porting. Disable the warning in the Qt WebKit version via a GCC pragma, because Qt WebKit is incompatible with Qt 6, where the deprecated feature is no longer available.
The former minimum zoom factor value is less than the minimum zoom factor of QWebEngineView 0.25. The first two zooming-in steps starting from that minimum value have no visible effect in the Qt WebEngine version. This may scare a user into thinking that reducing zoom factor to its minimum value cannot be undone. The new minimum is not 0.25, because 1.0-0.25 is not divisible by 0.1 (the added comment explains why it should be divisible). Article text is still unreadable when zoom factor is set to its new minimum value.
Standard operators don't work well for imprecise floating-point numbers. For example, zooming out from an almost-minimum zoom factor, then zooming in to the default zoom factor does not disable the zoomBase (Normal Size) action. Replacing '<' and '>' with fuzzy inequality works, because the code above clamps zoomFactor into [minZoomFactor, maxZoomFactor].
vedgy
force-pushed
the
webkit-or-webengine
branch
from
548c005
to
2fbca0a
Compare
FreeBSD is dropping it as well, and GoldenDict port had been marked deprecated today. I maintain the port and now unsure how to proceed. One way is to follow AUR and switch to Igor's fork, another is to pull some patches off GitHub and apply on top of the official version 1.5.0 I'm currently tracking. Ideally, of course, I'd love to see them merged to the main trunk sooner and new version tagged, e.g. 1.5.1. |
|
My AUR package merges master into the branch we/webkit-or-webengine, which is equivalent to this PR's branch, except it is not force-pushed into. Other packagers can either merge these two branches as well or merge/rebase this PR's branch into master, which will produce the same end result.
I have proposed a path for this to happen in #1542 (comment). But as has been the case for years, GoldenDict lacks users with requisite development experience and motivation for major contributions. |
I have tested these changes with Qt 4.8.7 and Qt 5.15.5 (both WebKit and WebEngine) on Manjaro GNU/Linux.
DSL, StarDict and MediaWiki dictionaries work. Also briefly tested Morphology and Programs at some point - they worked OK. Haven't tested WebSite dictionaries as I don't use and don't care about them at all. WebSite dictionaries probably don't work in this initial Qt WebEngine version.
This pull request shows the big picture, but it is probably too large for a proper review. @Abs62, would you prefer smaller consecutive pull requests containing groups of related commits from here?
Improvements in the Qt WebKit version:
A likely "regression" is that something, e.g. some JavaScript code, may no longer work in Qt 4.6-4.7 correctly. But who would build GoldenDict against these ancient Qt versions if Qt 4.8 was released more than 10 years ago? If anyone builds against and cares about such Qt versions, please test the Qt WebKit version and report.
Qt WebEngine TODOs:
scrollToGdAnchor()to JavaScript. A sample MDict dictionary with gdanchor links between articles is needed, preferably w/o hieroglyphs. [file scripts/webengine_blocking.js]In order to test the Qt WebEngine-based version of GoldenDict, pass
"CONFIG+=use_qtwebengine"toqmake. For example:qmake "CONFIG+=use_qtwebengine"Would anyone volunteer to test this pull request?
Preconditions for merging this pull request to master
No GoldenDict developer other than myself has expressed any interest in reviewing or testing this pull request. No one has raised any objections either. Anyone who cares about merging Qt WebEngine support into master and has requisite skills can contribute one or more items in the following precondition list:
Building and testing the Qt WebEngine version on Windows and macOS is optional, because the new version is useful at least on one platform and its presence does not impair the existing Qt WebKit functionality on other platforms. Building and testing on macOS is optional, because no active GoldenDict developer or contributor cares about it. vedgy/pull/1 indicates that the Qt WebEngine version is buildable and works on macOS with homebrew.
Don't forget to merge in the branch master before building.
If a person who tests a binary version does not trust the person who has built it, the testing can be performed in a virtual machine, or one can create a separate temporary user without permission to access system and real user files.
After testing, please post a comment specifying the operating system and its version and outlining what has been tested and how well it works. Also post a screenshot of GoldenDict's Help=>About window on the target platform.