KDevelop/Documentation : implementation of a QTextBrowser-based viewer
Needs ReviewPublic

Authored by rjvbb on Oct 8 2017, 9:02 PM.

Details

Reviewers
None
Group Reviewers
KDevelop
Summary

I've long thought it overkill to embed an almost full-fledged webbrowser inside KDevelop just for browsing Qt help (and even more so for the simpler CMake docs and man pages).

Qt's Assistant uses QtTextBrowser by default; I have been adapting its implementation to KDevelop's document viewer.

As far as I can tell it works more than well enough to peruse the documentation the way I would in an embedded viewer. The layout is somewhat simpler, more in line with use in an embedded viewer; I find it distracts less from the source.

Using QTextBrowser is implemented as a fallback, you have to disable WebEngine and WebKit support (or not have either component installed) to activate this mode. I would suggest to make this the default mode since it doesn't require extra dependencies, and focus on adding proper support for opening links in an external browser of choice at least for the QtHelp documentation. I currently have a working implementation that adds a contextmenu action to achieve this, using a remotely controlled instance of Qt's Assistant. I am looking into modifying the "Show documentation for" action in the context-help popups to trigger that external viewer also.

Test Plan

HTML rendering aside this seems to work as well as the QtWebKit-based mode. Links to different QtHelp sections (.qhc files) don't work in either mode (e.g. the link to the QMake docs in the QtCore "Getting Started" section). I think that's simply a limitation of the QtHelp plugin design.

Testing has been done on Mac and Linux, with all 3 rendering backends.

Backend comparison (QWK and QWE behaviour isn't modified by this patch):

  • QWK: links aren't followed; the contextmenu "Copy" action copies the link text, not the link address.
  • QWE: links are followed, contextmenu idem QWK. Tested on Mac only, where I get continuous errors that suggest inappropriate cross-thread calling, warnings about rejected JavaScript resources and a systematic crash on exit (also without this patch).
  • QTB: links work except across .qch files; links can be copied to the clipboard. "Lean and mean"; the QtHelp plugin is prewired to support opening links in an external full-fledged doc browser.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
rjvbb created this revision.Oct 8 2017, 9:02 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptOct 8 2017, 9:02 PM
apol added a subscriber: apol.Oct 8 2017, 9:41 PM

TBH... why bother.
You have the risk that it's not properly rendered and adds more code to maintain...

rjvbb added a comment.Oct 8 2017, 11:38 PM

I'm tempted to think that anything too complex to be rendered correctly is probably not really meant to be used in a simple embedded browser anyway.

Qt have considered QTextBrowser sufficient to be used as the default render backend in the Assistant. In practice, the only things in QtHelp documentation I've encountered that didn't render properly with QTB were the inline copies of source files.
If that's a concern, well, just use QWE or QWK instead of QTB...

BTW, don't the quick help tooltips use QTB?

Why did I bother? Because the idea of running what's essentially a full-blown webbrowser bothers me, knowing that KDevelop is already resource hungry enough as it is and that the documentation in question can be rendered well enough with QTB.

The implementation is indeed a bit more complicated than I thought it would be in the beginning but can probably be optimised. And as far as I'm concerned we could remove the QtWebKit support if this goes in.

@arichardson had a previous patch on similar lines: https://git.reviewboard.kde.org/r/126156/

Very strong +1 from me - it's ridiculous to have what's essentially a complete web browser as a dependency, for a small improvement of a side feature. Thanks to https://bugreports.qt.io/browse/QTBUG-54172 , the webengine-based thing doesn't actually work well in any case.

And yes, anyone producing API documentation that relies on advanced rendering features is insane.

rjvbb added a comment.Oct 9 2017, 8:30 AM

Thanks for speaking my mind (too) stronger than I dared :)

Here's what I meant with collaborative: maybe those of us who'd like to see this get in could try the patch for a while, see what works, what doesn't, and what can be improved? Note that I've focussed on the QtHelp documentation, have done only a few quick checks of CMake docs and didn't try the manpage plugin at all as yet.

I see that the RB ticket mentions how the patch under review doesn't allow to follow links in QtHelp docs. Figuring out how to do that (without ending up in an unending reload loop) was what took me the most time, and is why I'm deriving QTextBrowser. The real tricky thing is that, ideally, StandardDocumentationView would need direct access to QHelp* APIs to fetch the content of qthelp: URLs.

Basically, you need to override QTextBrowser::loadResource(type,url) with a method that can invoke QHelpEngine::fileData(url) and return that content. I am currently working around that by using a new StandardDocumentView::load(url,content) method in the QtHelp plugin. I think that's safe because (I think) we shouldn't ever be in a situation where we cannot do that. The obvious other approach would be to provide StandardDocumentationView with a callback to a QtHelpDocumentation method which does the work, for instance a closer clone of the Assistant's

QVariant HelpViewer::loadResource(int type, const QUrl &name)
{
    QByteArray ba;
    if (type < 4) {
        const QUrl url = HelpEngineWrapper::instance().findFile(name);
        ba = HelpEngineWrapper::instance().fileData(url);
        if (url.toString().endsWith(QLatin1String(".svg"), Qt::CaseInsensitive)) {
            QImage image;
            image.loadFromData(ba, "svg");
            if (!image.isNull())
                return image;
        }
    }
    return ba;
}

(Having support for some types of images could be nice, but is it useful?)

rjvbb updated this revision to Diff 20519.Oct 9 2017, 3:15 PM
rjvbb edited the test plan for this revision. (Show Details)

I've been cleaning up the code a bit, getting rid of unnecessary methods and improving things:

  • external links are now handed off to QDesktopServices::openUrl() to be opened by the user's webbrowser (like Qt's assistant does)
  • clicking on a link that fails to load now triggers a reload of the current URL (e.g. the "qmake" link, qthelp://org.qt-project.qtcore.580/qmake/qmake-manual.html on the QtCore doc homepage, qthelp://org.qt-project.qtcore.580/qtcore/qtcore-index.html)

That reload is delayed, the only way I found to ensure that clicking on a relative link to another topic in the current section (e.g. the "QPointer" link) will be resolved w.r.t. the correct base URL. It turns out to be tricky to determine whether QTextBrowser::setSource() will have the intended effect; I think that the call has no effect when a load is still in progress.

The only obvious improvement I see is to add a way for HelpViewer::loadResource() to load other kinds of resources. I'm thinking of a callback that one hands off to StandardDocumentationViewer. How generic should such a callback have to be (should the client also provide a QPointer to itself, for instance?) because how useful could such a function be for other dependent plugins? I notice for instance that the manpage plugin fails to display its navigation buttons.

rjvbb set the repository for this revision to R32 KDevelop.Oct 9 2017, 3:15 PM
rjvbb updated this revision to Diff 20536.Oct 9 2017, 7:42 PM

This revision implements my idea of using an optional callback that allows StandardDocumentationView (rather, its actual rendering instance) to call back into the documentation plugin to obtain content for URLs it would otherwise not know how to handle.

I've implemented such a callback for the QtHelp plugin.

I wasn't really expecting it, but this allows the documentation toolview not only to load images, it also to render documentation using the intended CSS. The aforementioned RB ticket had a remark that QTextBrowser wouldn't work with docs generated via doxygen; I'm attaching screenshots (including of the ECM docs created with the Kapidox framework) that prove otherwise.

rjvbb set the repository for this revision to R32 KDevelop.Oct 9 2017, 7:42 PM
rjvbb added a comment.Oct 9 2017, 7:48 PM

A bunch of screenshots.
Not to beat myself on the chest, but I think this looks pretty damn good, more than good enough to be a valid replacement for WebKit (or WebEngine if the problems Francis cited are bound to remain).

rjvbb updated this revision to Diff 20575.Oct 10 2017, 4:24 PM

Updated patch which drops the callback in favour of a virtual StandardDocumentationView::loadResource() method that can be overridden (e.g. by QtHelpDocumentationView::loadResource()).

In addition this revision splits out the QTextBrowser implementation from standarddocumentationview.cpp into standarddocumentationview_qtb.cpp and divides the former file into a section with shared code and a section with the WebKit/WebEngine code. Evidently that part could go into its own file too.

I tested the build using WebKit as the backend with and without my patch (not with the very latest version) and didn't notice any difference. I do notice that it won't follow most (if not all) links in the documentation (also without my patch), is there something wrong with my build there?

rjvbb set the repository for this revision to R32 KDevelop.Oct 10 2017, 4:24 PM
rjvbb updated this revision to Diff 20596.Oct 11 2017, 3:02 PM

Subclassing StandardDocumentationView also makes it trivial to provide a tailored context menu in the QtHelp documentation plugin when using the QTextBrowser-based viewer. This one is based on the menu Qt's assistant provides, and allows to copy links.

I've included outline code showing how one could provide an option to open the displayed documentation in an external viewer directly; see D4484.

rjvbb retitled this revision from KDevelop/Documentation : support using QTextBrowser (WIP/PoC) to KDevelop/Documentation : implementation of a QTextBrowser-based viewer.Oct 11 2017, 3:09 PM
rjvbb added a comment.Oct 11 2017, 3:18 PM

I think that despite the simpler rendering engine we have now gone beyond feature-parity with the QtWeb*-based viewer backends.

Here's a screenshot of the context menu after right-clicking on the QString::iterator link:

(external viewer support is functional in my personal builds)

In D8211#154109, @rjvbb wrote:

I tested the build using WebKit as the backend with and without my patch (not with the very latest version) and didn't notice any difference. I do notice that it won't follow most (if not all) links in the documentation (also without my patch), is there something wrong with my build there?

I observe this as well. Did you find the issue?

Not specifically, but I can guess now that I know it doesn't affect me only.

Most links in QHC documentation appear to be relative, and must thus be resolved w.r.t. the URL of the current document. QTextBrowser can't do that, and I thus had to overload QTextBrowser::loadResource() in the QtHelp plugin to take care of that resolving. (loadResource() gets called after using QTextBrowser::setSource().)

Something similar must be going on with QtWebKit: clicking a link probably produce an error which is simply being ignored.

It's surprising though, why would QtWebKit support qhelp: URLs but not resolving relative links against them?

rjvbb updated this revision to Diff 20673.Oct 13 2017, 10:45 AM

Patch cleaned up and some minor improvements.

rjvbb set the repository for this revision to R32 KDevelop.Oct 13 2017, 10:45 AM
rjvbb updated this revision to Diff 20676.Oct 13 2017, 1:37 PM

Fixes the build against QtWebKit/Engine

rjvbb edited the summary of this revision. (Show Details)Oct 13 2017, 1:50 PM
rjvbb edited the test plan for this revision. (Show Details)
rjvbb set the repository for this revision to R32 KDevelop.
rjvbb updated this revision to Diff 20877.Oct 16 2017, 8:15 PM
rjvbb edited the summary of this revision. (Show Details)

updated to match D4484

rjvbb set the repository for this revision to R32 KDevelop.Oct 16 2017, 8:15 PM
kossebau added inline comments.
kdevplatform/documentation/standarddocumentationview.cpp
419 ↗(On Diff #20877)

Can be removed, useless with current content of source file, nothing needs to be moc'ed here.

kossebau added inline comments.Oct 19 2017, 3:31 PM
kdevplatform/documentation/standarddocumentationview.h
74 ↗(On Diff #20877)

Why is this method not in #ifdef USE_QTEXTBROWSER?

76 ↗(On Diff #20877)

KDevelop::StandardDocumentationView is part of the public KDevPlatformDocumentation API, standarddocumentationview.h is an installed header.

Having a part of the public API only created depending on build-time config settings needs this settings also be deployed with the installed headers.

rjvbb added inline comments.Oct 19 2017, 4:03 PM
kdevplatform/documentation/standarddocumentationview.cpp
419 ↗(On Diff #20877)

I'll double-check. This will also contain to-be-moc-ed things from standarddocumentationview.h .

kdevplatform/documentation/standarddocumentationview.h
74 ↗(On Diff #20877)

I've hesitated about that. It can be of course. I've left it here for now, in case anyone wants to extends the QWE or QWK backends in a similar way with a callback into the QtHelp (or other) plugin.
(I won't be the one doing that, so I don't care where we put this.)

76 ↗(On Diff #20877)

You're saying I have 2 simple choices here:

  • remove #ifdef in the headerfile and provide (stub) implementations for the QtWeb* configuration
  • add the USE_QTEXTBROWSER flag in a generated headerfile so that the setting becomes available publicly. This would probably also mean that the flag best become under direct control of a cmake option.

Come to think of it, the 2nd option is probably the way to go in case there are contributed documentation plugins (are there?).

Do we agree on the name or should I use something else?

To be honest, I very much dislike the exposure of that implementation detail: whether using QWebEngine, QWebKit, or QTextBrowser ideally would not be leaked. Because it needs any 3rd-party which uses the documentation API to provide some own documentation to also support all options in the code.

BTW, have you tested already how this change works with kdev-php?

And all the additional ifdefs make life worse:

  • adds another variant which is not covered by CI
  • developers fixing code will need to rebuild for any ifdef variant to test their changes
  • code is more complicated to understand, which it already is

As developer, I actually would prefer it all three variants could be built and installed in parallel (given deps are available). And as developer I could select by some env var which variant is used at runtime. That would allow CI to build all variants, unit tests to be run for all (if ever written) and developers having less work to build and test things.

So from a maintenance POV the given patch & approach sadly gets a -1 from me as co-contributor.

kdevplatform/documentation/standarddocumentationview.cpp
419 ↗(On Diff #20877)

standarddocumentationview.h is covered by cmake's automoc already, given it's matching standarddocumentationview.cpp -> standarddocumentationview.h

See https://cmake.org/cmake/help/v3.0/prop_tgt/AUTOMOC.html

kdevplatform/documentation/standarddocumentationview.h
74 ↗(On Diff #20877)

Given we are flexible with the API per minor kdevelop/kdevplatform version, there is no need to prepare already for possible future extensions.

So let's keep the API as minimal as needed.

76 ↗(On Diff #20877)

I tried to stay abstract with "deployed with the installed headers"as myself I don't know best practices from the top of my head. Needs others to comment on.
Perhaps the solution is also something which already works for the qthelp plugin, without needing to rely on the same CMake vars used for kdevplatfom/documentation.

89 ↗(On Diff #20877)

This name is too generic and needs to be more explicit, add to it what it restores.

102 ↗(On Diff #20877)

What is the requirement to be a supported scheme? The current hard-coded implementation seems rather made-up. What does "supported" mean? By whom?

So "man", "help", "about", why are they supported? Why are others like "https" not?

rjvbb added a comment.Oct 19 2017, 6:04 PM
To be honest, I very much dislike the exposure of that implementation detail: whether using QWebEngine, QWebKit, or QTextBrowser ideally would not be leaked.

Ok, in that case I'll make the new API available for all backends, and provide stub implementations for those where it makes no sense.
Once that's done (next revision) I'll expect users of external documentation plugins to check if and how those are affected. (Will be a couple of days, probably.)

BTW, have you tested already how this change works with kdev-php?

No, I don't "do" php. Doc plugins don't have to use the new API, in which case they get basically the same toolview behaviour as with the QtWebKit backend. This is what happens for the manpage and cmake documentation plugins (there are some minor rendering differences for the former, none that I noticed for the latter).

  • adds another variant which is not covered by CI

That shouldn't be hard to fix for someone who knows how to do that and has the required access.

As developer, I actually would prefer it all three variants could be built and installed in parallel (given deps are available).

This could be a secondary step, because it'll require separating the backend code into something properly standalone. I don't like the additional complexity though. Why would you want to have QWK and QWE backends available, for instance?

As a developer and user I'd much rather see only a QTextBrowser backend. As Friedrich implied, it's more than sufficient for embedded documentation viewing. Anything requiring more advanced rendering code is probably best perused in a dedicated browser.

In D8211#157173, @rjvbb wrote:
BTW, have you tested already how this change works with kdev-php?

No, I don't "do" php. Doc plugins don't have to use the new API, in which case they get basically the same toolview behaviour as with the QtWebKit backend. This is what happens for the manpage and cmake documentation plugins (there are some minor rendering differences for the former, none that I noticed for the latter).

Sorry, no excuse :) You do not need to do "php" to test the documentation. By default the PHP documentation plugin uses online docs from http://php.net.

  • adds another variant which is not covered by CI

That shouldn't be hard to fix for someone who knows how to do that and has the required access.

"Should not"... can you tell if it is? If so, how would it be fixed IYHO?

As developer, I actually would prefer it all three variants could be built and installed in parallel (given deps are available).

This could be a secondary step, because it'll require separating the backend code into something properly standalone. I don't like the additional complexity though. Why would you want to have QWK and QWE backends available, for instance?

Hu? Please re-read slowly again what you replied to, I gave the reason there.

As a developer and user I'd much rather see only a QTextBrowser backend. As Friedrich implied, it's more than sufficient for embedded documentation viewing. Anything requiring more advanced rendering code is probably best perused in a dedicated browser.

Unless there is another "Friedrich" involved, the one writing here would have only said the opposite., and surely did: QTextBrowser is _not_ sufficient for anything generated by Doxygen (sadly) or online/web-centric docs like the PHP ones.
And "more advanced rendering code" still makes sense in an integrated development environment, at least to me, I see rendering power orthogonal to being part of IDE. Actually I consider a separate general browser application an inferior solution, as it does not allow to be properly integrated into kdevelop project/session management (project/session specific bookmarks, browsing history, tight control about location in UI etc).

rjvbb added a comment.Oct 19 2017, 7:43 PM
Sorry, no excuse :)

Look, I'm doing this to help make KDevelop lighter and make a huge external dependency optional so I'm not really motivated to install all kinds of new stuff just to do a few tests. It's been a lot of work already, it won't be hard for someone who uses kdev-php to apply the patch and see what happens. I may need to step up if there are issues (that can be resolved), but as I've said from the onset, I really think this change can be collaborative. With the turn things are taking now it begins to look like this is another thing I've been doing just for my own benefit.

If your claim is correct that online documentation won't render correctly, then a QW backend will need to remain for users who rely on such documentation.

I gave the reason there.

You did not give a reason why YOU would want QWE and QWK backends available. It's an either/or choice already (in the order listed) and that makes sense given that QWK is deprecated.

Unless there is another "Friedrich" involved, the one writing here would have only said the opposite., and surely did: QTextBrowser is _not_ sufficient for anything generated by Doxygen (sadly) or online/web-centric docs like the PHP ones.

Sorry, I meant Francis.
Your claim is incorrect: the KF5 framework documentation built as QCH with kapidox renders just fine as you can see from the screenshot of the ECM docs. What doesn't work is the rendering of embedded sourcefiles. I think that's a limitation we can live with, esp. with the possibility to open a link in an external Qt Assistant viewer.

That's what I mean with an external browser, btw: something controlled from the IDE (evidently). That means opening links (the "Show documentation for" link in the contextual help popups) but can also mean opening bookmarks stored in the IDE, or going back in the history. Tight control about location in the UI is something I want too: somewhere outside, independent from the main window = on my secondary screen where it can take all the space it needs. With its single window approach KDevelop's UI is cramped enough as it is.

You're really positioning yourself as someone who should testdrive this patch and upload a few screenshots showing failures (I was going to add "how easy it is to induce failures" but you can't really see that from a picture, can you :)).

In D8211#157206, @rjvbb wrote:
Sorry, no excuse :)

Look, I'm doing this to help make KDevelop lighter and make a huge external dependency optional so I'm not really motivated to install all kinds of new stuff just to do a few tests. It's been a lot of work already, it won't be hard for someone who uses kdev-php to apply the patch and see what happens. I may need to step up if there are issues (that can be resolved), but as I've said from the onset, I really think this change can be collaborative.

Fetching, building and installing kdev-php is a matter of minutes in this case. There are no complicated extra deps, quick check at https://cgit.kde.org/kdev-php.git/tree/CMakeLists.txt
Discussing this here takes more time.

Sorry but it has to be you to test this, as the other persons interested in this build option seem to have no time. Such is life, do not complain to me :) I already invested some of my time to help you, despite having my own projects with higher priority.

I gave the reason there.

You did not give a reason why YOU would want QWE and QWK backends available. It's an either/or choice already (in the order listed) and that makes sense given that QWK is deprecated.

"That would allow CI to build all variants, unit tests to be run for all (if ever written) and developers having less work to build and test things."

Unless there is another "Friedrich" involved, the one writing here would have only said the opposite., and surely did: QTextBrowser is _not_ sufficient for anything generated by Doxygen (sadly) or online/web-centric docs like the PHP ones.

Sorry, I meant Francis.
Your claim is incorrect: the KF5 framework documentation built as QCH with kapidox renders just fine as you can see from the screenshot of the ECM docs.

My not-so-incorrect claim is based on months of usage experience with and development work on doxygen generated docs. There are functional elements in the document (driven by JavaScript), e.g, the "Properties inherited from" & Co. stuff. Content behind such elements is hidden/visible by random after loading (from what I experienced), and cannot be toggled. Then with newer doxygen version the default CSS results in unreadable titles e.g. on method sections. Which makes the docs rather unusable.

The screenshot of the ECM docs linked above shows that content is rendered, indeed. But really, does it qualify as "fine"? Please take another look and even more try to make use of the document during development. I doubt you will keep that qualifier. Besides, kapidox does some custom processing and if those are docs fetched via GHNS, they are from api.kde.org server which runs a rather old doxygen version. So not representative of average doxygen generated docs.
For some QCH file with recent doxygen version with no custom CSS or processing see e.g. https://share.kde.org/index.php/s/UhVPFAy2cTb8cBL and class docs for e.g. KUiServerJobTracker

For more background info grep "Doxygen bugs" at https://frinring.wordpress.com/2017/06/19/adding-api-dox-qch-files-generation-to-kde-frameworks-builds/ and see especially the first two bugs in that list.

rjvbb added a comment.Oct 19 2017, 9:44 PM
Sorry but it has to be you to test this, as the other persons interested in this build option seem to have no time.

Let's see if someone steps up, then.

It'll just be to confirm that documentation links open in the user's web browser, as that's what the HelpViewer::setSource() override currently dictates.
IIRC I took that shortcut when I understood that QTB won't do any remote fetching. In fact, isUrlSchemeSupported() method is called something like isLocalUrl() in the Assistant code.

Then with newer doxygen version the default CSS results in unreadable titles e.g. on method sections. Which makes the docs rather unusable.

Then please post screenshots because I don't see any of that.
The contextual help popups already use QTB, do you see any issues with those?

The screenshot of the ECM docs linked above shows that content is rendered, indeed. But really, does it qualify as "fine"? Please take another look and even more try to make use of the document during development.

It's more than good enough for me - if I use the embedded doc toolview. By now it should be clear that I don't, most of the time, but instead send the links to open to a remote-controlled Assistant copy.

I build everything myself, including the documentation (using doxygen 1.8.13 which is the latest as far as I can see).

R.

croick added a subscriber: croick.Oct 19 2017, 9:44 PM

I can confirm. I'm using a dark color theme and was positively surprised about the dark background at first. But Qt documentation headings seem to have their own style. So that's what a random doc page looks like:


Not sure, if this would be easy to fix. It's similar for manpages.

rjvbb added a comment.EditedOct 19 2017, 9:55 PM

KUIServerJobTracker documentation, here in a detached window on Mac using QTB:

Text is too large and layout too spacey for my taste, but that aside the documentation is perfectly usable.

And Christoph's feedback is exactly why this shouldn't be a one-man job: who'd have thought of testing with another colour palette? Now I understand why the original code imposes a simple B&W CSS sheet. Getting QTB to do the same shouldn't be hard: I just need to return the desired CSS from the loadResource() overload, instead of the actual CSS.

@croick: is that your desktop palette and if so, how does Qt's Assistant render that same documentation (if it's QCH-based)?

And "more advanced rendering code" still makes sense in an integrated development environment, at least to me, I see rendering power orthogonal to being part of IDE. Actually I consider a separate general browser application an inferior solution, as it does not allow to be properly integrated into kdevelop project/session management (project/session specific bookmarks, browsing history, tight control about location in UI etc).

Just my two cents: I don't care that much about bookmarks or having the documentation within the IDE itself. I consider the most important feature that I can hover over a function in the code, do one click and directly get to the documentation, without copying the name, without typing anything.

The main reason why the integrated viewer makes sense for me is to view man pages, which I can't view in the browser. At least not offline. So that is nice. Not only because I don't have to type man ... in the terminal, but also because it looks nicer than on a terminal. But fully-fledged HTML docs, possibly with advanced CSS and JavaScript? I don't see any reason to not use an external web browser. Browsers are specialized to display web pages, and it's really hard to do a better job there, regarding the actual display, interactivity, or additional features like bookmarks and history.

For me the purpose of integrating something is not to avoid leaving the window, but to streamline the workflow. Take compilation as an example: when there's an error, I click on it and get to the piece of code that caused it. This makes fixing compiler errors a lot easier compared to compiling from the terminal. Similar for debugging: I find a mistake, and fix it directly. I don't need to search for a file and scroll to the problematic line, I'm already there.

But what is the value of integrating documentation, apart from not having to leave the window? The integration with the code: being able to quickly get the documentation for a piece of API I'm using. But that doesn't exclude using an external application to actually view the documentation.

I'm getting a bit off-topic here, so to sum it up: Regardless of the technical merits of QWE and QWK, I don't see a problem with having a somewhat simplified documentation renderer within KDevelop itself, possibly referring to external applications when it gets too hard.

In D8211#157208, @rjvbb wrote:
Then with newer doxygen version the default CSS results in unreadable titles e.g. on method sections. Which makes the docs rather unusable.

Then please post screenshots because I don't see any of that.

Sorry, my bad here in uploading an old QCH file which missed half of the meat, i.e. did not have subclass information (thus missing all the "* inherited from" sections which are the biggest pain).
Please download and try again, newer version has been uploaded to same place.

And preview here what I mean (left Qt Assistant with QTB, right KDevelop with QWE):

Sections cannot be toggled, are open/closed randomly after loading (no pattern seen over the time):

Method titles are partially hidden, tags like virtual,visibility,abstract hard to read due to missing spacing ()

I had tried to use Qt Assistant (so QTB) with that a few weeks, but it was a pain. It is better than nothing as one can draw some info out of the docs, but it is not a joyful experience, so I often escaped to the online docs when I had network. Until I fixed the embedded documentation viewer to wok as I need it (still need to clean-up and push the rest of it, only merged the bit for the search so far).

So as documentation power-user my strong feedback is: QTB sucks at least for doxygen-created documentation material.

The contextual help popups already use QTB, do you see any issues with those?

That is not really related, due to not being affected by JavaScript and CSS, unless the extraction done by IDocumentation::description() implementations pulls out too much non-content stuff.

The screenshot of the ECM docs linked above shows that content is rendered, indeed. But really, does it qualify as "fine"? Please take another look and even more try to make use of the document during development.

It's more than good enough for me - if I use the embedded doc toolview. By now it should be clear that I don't, most of the time, but instead send the links to open to a remote-controlled Assistant copy.

Reads to me like "it is good enough for me because I do not use it (and thus do not care but you are invited to collaborate)". This approach is not really "collaborative", or?

So again, why this work? To me it seems you want to build KDevelop without QWebEngine or QWebKit, and also only use external documentation viewers, no embedded ones. Which is fine in itself, I see no issue with supporting that as option.
But then the approach in this patch is only an indirect solution to achieve your goal, while making life of those more complicated who want to maintain and develop the embedded documentation viewer further.
And as someone who has some WIP to improve the current embedded documentation viewer (e.g. with bookmarks or multiple views), I really am not happy about having in the future also to care for another code variant, which itself might not really be used by many.

Let's read now again the reasoning for this patch :
"I've long thought it overkill to embed an almost full-fledged webbrowser inside KDevelop just for browsing Qt help (and even more so for the simpler CMake docs and man pages)."
This seems flawed to me. Software is developed to serve a purpose. one collects requirements, then gets out to find a solution, ideally be reusing existing products.
But here the rationale is some "thinking", without numbers to prove some "overkill". And by listing simply a few examples which are claimed to fit that "thinking". No detailed analysis given.
Purpose of this patch seems: get rid of some dependency.

"Qt's Assistant uses QtTextBrowser by default; "
Build system says something else: http://code.qt.io/cgit/qt/qttools.git/tree/src/assistant/assistant/assistant.pro uses QtWebKit is present, only if not falls back to QTextBrowser

"As far as I can tell it works more than well enough to peruse the documentation the way I would in an embedded viewer. The layout is somewhat simpler, more in line with use in an embedded viewer; I find it distracts less from the source."
If the layout is simpler, this is not by design but only by accidentally unsupported CSS features. Which means the layout is not completely designed and thus might be broken, lhard to read and look shitty. Also why should the same documentation content look more complex in a stand-alone viewer app? This does not make sense to me, I expect content to be simple to parse in both cases.

"I would suggest to make [using QTextBrowser] the default mode since it doesn't require extra dependencies, and focus on adding proper support for opening links in an external browser of choice at least for the QtHelp documentation."
The default mode means, the recommended mode for everyone. QWebEngine and/or QWebKit are available on most platforms and usually already installed (linux-centric thinking, but then closed source platforms are second class). QTextBrowser as default mode also means reduced and for some documentation broken functionality, so not a proper default. Runtime costs have yet to be measured, so the price is unknown. So "[being] extra dependencies" is only interesting for niche operating systems or custom development setups and should not dominate.
And making something default where parts of the work ("proper support for opening links in an external browser") are yet to be solved is premature.

KDevelop is by definition a "feature-full, plugin extensible IDE for C/C++ and many other programming languages.". So there could be lots of documentation material besides the "simpler CMake docs and man pages" and which might need more than QTB can support. Even QCH files, given there is no real specification, can in theory contain full HTML5-powered webpages with interactive WebGL examples. (There is a reason why Qt Assistant/Qt Creator had QtWebKit as default implementation for documentation viewer when available, until that came deprecated as module itself).

IMHO either there is a proper embedded documentation viewer or documentation viewing is delegated completely to external applications, But having a crippled embedded documentation viewer by design does not serve a real purpose. KDevPlatform is built on the idea of plugins with interfaces. And when we want to have a plugin option for the delegation of documentation viewing to external applications, that should be done by designing a new interface to abstract the invocation of documentation display, where the current documentation tool view could be then coming from one plugin serving that interface (which then has the QWK or QWE dependencies), and delegation to external applications be done by another one. Right now instead this patch feels like pushing for crippling the existing embedded documentation viewer in favour of using external applications. That's not "collaborative" to me.

kfunk added a subscriber: kfunk.Oct 20 2017, 7:57 AM
In D8211#157208, @rjvbb wrote:
Then with newer doxygen version the default CSS results in unreadable titles e.g. on method sections. Which makes the docs rather unusable.

Then please post screenshots because I don't see any of that.

Sorry, my bad here in uploading an old QCH file which missed half of the meat, i.e. did not have subclass information (thus missing all the "* inherited from" sections which are the biggest pain).
Please download and try again, newer version has been uploaded to same place.

And preview here what I mean (left Qt Assistant with QTB, right KDevelop with QWE):

Sections cannot be toggled, are open/closed randomly after loading (no pattern seen over the time):

{F5439139}

Method titles are partially hidden, tags like virtual,visibility,abstract hard to read due to missing spacing ()

I had tried to use Qt Assistant (so QTB) with that a few weeks, but it was a pain. It is better than nothing as one can draw some info out of the docs, but it is not a joyful experience, so I often escaped to the online docs when I had network. Until I fixed the embedded documentation viewer to wok as I need it (still need to clean-up and push the rest of it, only merged the bit for the search so far).

So as documentation power-user my strong feedback is: QTB sucks at least for doxygen-created documentation material.

The contextual help popups already use QTB, do you see any issues with those?

That is not really related, due to not being affected by JavaScript and CSS, unless the extraction done by IDocumentation::description() implementations pulls out too much non-content stuff.

The screenshot of the ECM docs linked above shows that content is rendered, indeed. But really, does it qualify as "fine"? Please take another look and even more try to make use of the document during development.

It's more than good enough for me - if I use the embedded doc toolview. By now it should be clear that I don't, most of the time, but instead send the links to open to a remote-controlled Assistant copy.

Reads to me like "it is good enough for me because I do not use it (and thus do not care but you are invited to collaborate)". This approach is not really "collaborative", or?

So again, why this work? To me it seems you want to build KDevelop without QWebEngine or QWebKit, and also only use external documentation viewers, no embedded ones. Which is fine in itself, I see no issue with supporting that as option.
But then the approach in this patch is only an indirect solution to achieve your goal, while making life of those more complicated who want to maintain and develop the embedded documentation viewer further.
And as someone who has some WIP to improve the current embedded documentation viewer (e.g. with bookmarks or multiple views), I really am not happy about having in the future also to care for another code variant, which itself might not really be used by many.

Let's read now again the reasoning for this patch :
"I've long thought it overkill to embed an almost full-fledged webbrowser inside KDevelop just for browsing Qt help (and even more so for the simpler CMake docs and man pages)."
This seems flawed to me. Software is developed to serve a purpose. one collects requirements, then gets out to find a solution, ideally be reusing existing products.
But here the rationale is some "thinking", without numbers to prove some "overkill". And by listing simply a few examples which are claimed to fit that "thinking". No detailed analysis given.
Purpose of this patch seems: get rid of some dependency.

"Qt's Assistant uses QtTextBrowser by default; "
Build system says something else: http://code.qt.io/cgit/qt/qttools.git/tree/src/assistant/assistant/assistant.pro uses QtWebKit is present, only if not falls back to QTextBrowser

"As far as I can tell it works more than well enough to peruse the documentation the way I would in an embedded viewer. The layout is somewhat simpler, more in line with use in an embedded viewer; I find it distracts less from the source."
If the layout is simpler, this is not by design but only by accidentally unsupported CSS features. Which means the layout is not completely designed and thus might be broken, lhard to read and look shitty. Also why should the same documentation content look more complex in a stand-alone viewer app? This does not make sense to me, I expect content to be simple to parse in both cases.

"I would suggest to make [using QTextBrowser] the default mode since it doesn't require extra dependencies, and focus on adding proper support for opening links in an external browser of choice at least for the QtHelp documentation."
The default mode means, the recommended mode for everyone. QWebEngine and/or QWebKit are available on most platforms and usually already installed (linux-centric thinking, but then closed source platforms are second class). QTextBrowser as default mode also means reduced and for some documentation broken functionality, so not a proper default. Runtime costs have yet to be measured, so the price is unknown. So "[being] extra dependencies" is only interesting for niche operating systems or custom development setups and should not dominate.
And making something default where parts of the work ("proper support for opening links in an external browser") are yet to be solved is premature.

KDevelop is by definition a "feature-full, plugin extensible IDE for C/C++ and many other programming languages.". So there could be lots of documentation material besides the "simpler CMake docs and man pages" and which might need more than QTB can support. Even QCH files, given there is no real specification, can in theory contain full HTML5-powered webpages with interactive WebGL examples. (There is a reason why Qt Assistant/Qt Creator had QtWebKit as default implementation for documentation viewer when available, until that came deprecated as module itself).

IMHO either there is a proper embedded documentation viewer or documentation viewing is delegated completely to external applications, But having a crippled embedded documentation viewer by design does not serve a real purpose. KDevPlatform is built on the idea of plugins with interfaces.

Thanks for your thoughts Friedrich. Couldn't agree more on basically everything you said.

To me, it's kind of puzzling why people mind the dependency to either QtWebKit or QtWebEngine *at all*. People seem to have problems with using a full-featured web browser backend "just because!"
For developers: There are prebuilt Qt packages for all major platforms out there -- install it, use it, be done.
For users: On Linux it's a no-brainer with distro-packages, on any other platform we ship it with the installer/bundle anyway.

Ending up with a half-crippled version of the documentation viewer plugin (using QTB) is a no-go, given that this will only lead to degraded user experience and bug reports where we need to tell people to "use the correct build option".

Plus, yet to say, this is another #ifdef cascade we as the core developers need to maintain in case this patch ever hits the repositories. Given your (Friedrich) recent poor experiences with Assistant based on QTB and the amount of code this patch adds (for no world-changing reason, and just to make a handful of developer's life more easy) I'm very reluctant to merge this at all, to be honest.

And when we want to have a plugin option for the delegation of documentation viewing to external applications, that should be done by designing a new interface to abstract the invocation of documentation display, where the current documentation tool view could be then coming from one plugin serving that interface (which then has the QWK or QWE dependencies), and delegation to external applications be done by another one. Right now instead this patch feels like pushing for crippling the existing embedded documentation viewer in favour of using external applications. That's not "collaborative" to me.

rjvbb added a comment.Oct 20 2017, 9:18 AM
But what is the value of integrating documentation, apart from not having to leave the window? The integration with the code: being able to quickly get the documentation for a piece of API I'm using. But that doesn't exclude using an external application to actually view the documentation.

Exactly (also the bits about hovering over a function - if that didn't exist I wouldn't even be bothering trying to improve the whole documentation viewer interface). The drawback of an integrated viewer in the kind of UI KDevelop puts up is that most of the time it's going to interfere with the very workflow it aims to streamline. Either you get a docked view which is probably too small while impeding too much on your code area (and thus the text flow in there) OR you get an external "floating" window which cannot easily be put exactly where you want it where it doesn't cover the main window. Not to mention when you have 2 sessions open and want to refer in session 1 to documentation displayed in session 2.
It's not for nothing that Qt Creator, Xcode and (IIRC) MSVC don't propose an embedded view (or not by default) and use a regular window.

There are other advantages to an external browser (controlled via the IDE) but that's indeed OT here.

As to the manpage plugin: I never use it. I see Aaron's point, but for me it's much faster to switch to a terminal window (using keyboard shortcuts) than to mouse to the manpage toolview and find the documentation I want in there. Most of the time reading a manpage involves using apropros first, anyway.

Sections cannot be toggled, are open/closed randomly after loading (no pattern seen over the time):

What you seem to be getting at here is using KDevelop as a documentation browser. I don't know what kind of (huge?) screen you use, but if it's not some crazyhighres affair used with 10-12pt fonts by a user with eagle sight there cannot be much space left for actual code. It seems to me that when application time is spent clicking on documentation layout gimmicks (excusez le mot), that application misses its goal when it wants to be something other than a documentation browser.

FWIW, Qt Creator's documentation browser is nice, a pleasure to use as far as I know it. Sadly it's integrated into something that's not a snap to load - and that applies to all those fancy embedded documentation browsers. (The one in Xcode is even worse.)

Method titles are partially hidden, tags like virtual,visibility,abstract hard to read due to missing spacing ()

I'm not seeing this, not to the same extent at least, and as I said, my KF5 qch documentation is generated with the latest doxygen, kapidox (currently 5.38.0) and Qt 5.8.0 . I'd have to see how this works when I strip support for the embedded CSS, which apparently I'll have to do anyway.

Re: Qt Assistant and its rendering backend: my recollection was a bit off. In practice we're both right: it'll use QWK if available and QTB otherwise. When you're building Qt from scratch (e.g. using the exhaustive tarball) QWK won't be available (in the build tree) and you'll thus get the QTB backend. Took me a while to figure that out, a couple of Qt releases back. I'm now making sure that I build this component with QWK available (and probably force the .pro file to use it.
Building a copy of Assistant that has QWK support isn't hard at all, and takes minutes only, so that shouldn't be an obstacle for KDevelop's target user population. We can provide documentation how to do that, too. I'm a bit surprised Qt haven't yet implemented a QWE backend, btw.

So, for the record, I'm dropping the idea that KDevelop might make the QTB backend the only or the default backend. I'm fine with the cmake implementation as it is, where you get QTB only when QWK and QWE are either disabled or unavailable.

still need to clean-up and push the rest of it

Oh joy, more refactoring work for me ahead... And:

to wok as I need it

(sic). I do presume you won't push that without proper review - I'm not the only user who can presume his doc viewing needs can be projected as is on the whole user population.

Reads to me like "it is good enough for me because I do not use it (and thus do not care but you are invited to collaborate)". This approach is not really "collaborative", or?

And "I want you to add this, modify that, and in general dot the i's exactly as I tell you" is so much more collaborative? We're all volunteering our work on this, and I think it's perfectly reasonable to expect that everyone works on what s/he cares most about. It's a pity that phab doesn't allow multiple users to work on a single patch directly. But I'll test and consider all serious contributions to this patch. Including one that makes runtime selection of a backend possible. (If we're starting to think about using an env. var to select the backend we might just as well implement runtime switching, so that plugins can -or have to- provide their preferred backend.)
BTW, @Friedrich: maybe you can integrate your idea of selectable backends with your pending changes - the CI arguments you advanced should apply to your changes too. My current patch should give you a pretty good idea about the few API additions that are required for QTB support. I can then see how I can refactor my patch to integrate with yours with minimal #ifdefs. I'd have to do that anyway.

As to 3rd party plugins: does it shock if they don't all support all possible backends, in particular a QTB backend?

Purpose of this patch seems: get rid of some dependency.

That was indeed the initial reason and is still the main reason. All the other work done after that became possible (as a fallback solution) was to make this fallback mode work better. There.
I remain convinced that dependencies as big as QWK and esp. QWE come with significant resource use (once you actually use them), but you're welcome to disregard that argument since I'm not providing proof.

People seem to have problems with using a full-featured web browser backend "just because!"

I cannot speak for all those people, but a full-featured browser backend makes sense in a browsing application, i.e. one where browsing is the main application. Apparently that even requires integrated bluetooth support nowadays, but I digress. KDevelop will need to make choices if it wants to maintain a remain manageable for both developers and users.

Don't forget that not all users have access to pre-built packages (you can't just grab QWE from Qt's builds and use it with a random "system" Qt build).

I tried the QWE backend, to verify if my patch affected anything. Activating it felt like turning on the airco on an older car with too tiny an engine, and IIRC gave me systematic crashes on exit on Mac. (I ignored them because I won't be using that backend.)

(for no world-changing reason, and just to make a handful of developer's life more easy)

All due respect, but that applies to just about all changes made to KDevelop. Just how many core developers are there, and what proportion of invasive changes gets put up for review with a honest attempt to obtain feedback from the entire user community?

To conclude: the purpose of this patch is to provide a fallback backend that works reasonably well for those of us who have their reasons to want a lean-as-possible KDevelop build. Despite what my choice of words in earlier posts may have suggested (suggestions to drop the other backends have been taken back). It's not intended to push for the use of an external browser. I'll see about mimimising the #ifdefs, and how we can get the rendered version to look like in the contextual popups (= simplified, not crippled). Documentation doesn't necessarily have to look good (= fancy), but it sure has to be readable.

Is it still an obligation to have a documentation viewer beyond those contextual help popups? Those would be enough for me, once something like D4484 gets committed.

rjvbb added a comment.Oct 20 2017, 9:41 AM
Sections cannot be toggled, are open/closed randomly after loading (no pattern seen over the time):
 F5439139: Spectacle.hc7421.png <https://phabricator.kde.org/F5439139>

I'm not seeing any differences across several reloads of the same page but it's indeed hard to tell whether a section can be toggled (= if there's anything in it).

Method titles are partially hidden, tags like virtual,visibility,abstract hard to read due to missing spacing ()
F5439130: Spectacle.PE7421.png <https://phabricator.kde.org/F5439130>

The result doesn't look exactly the same on my end (font choice or the fact I have Freetype+Fontconfig with the Infinality patches, on Linux?). I get empty bluish header bars over and well separated from the method name. The bar still serves its purpose as a visual separator, and you actually get to see only the exact method signature, not the approximation from the header seen on the right in the above screenshot. Of course the display on the right looks slicker, but the one on the left is usable enough - for a fallback rendering backend.

As said, we'll have to see how this changes when the embedded CSS is ignored and replaced with the simple B&W sheet. That's the kind of rendering result to which my earlier remark applied about it being less distracting.

The discussion is getting really nasty and I don't think it leads anywhere.

KDevelop is by definition a "feature-full, plugin extensible IDE for C/C++ and many other programming languages."

Maybe this is the way to go. Can't there be a documentation viewer interface, with multiple plugins implementing it, maybe some built by default, others if the necessary dependencies are available? Then some people can use a QWebKit-based viewer, others a QWebEngine-based viewer, and so on. I agree regarding the #ifdefs, this is really nasty and can't be the solution.

The common denominator seems to be that we want access to documentation from our code. If different developers implement different variants for doing that, why not? But they shouldn't interfere with each other. Ideally, none of the implementations should need to be aware of any of the others. That is not the case here.

If some implementation ends up being unmaintained for a long time, and nobody uses it anymore, then it may go the way of all obsolete code.

rjvbb added a comment.Oct 20 2017, 9:25 PM
Maybe this is the way to go. Can't there be a documentation viewer interface, with multiple plugins implementing it, maybe some built by default, others if the necessary dependencies are available?

I was thinking something similar over dinner: make the part that does the actual rendering a plugin. That's almost what Friedrich suggested, the way rendering backends are selected aside. The code currently shared with the contextual help popup subsystem would remain in a regular library that's always loaded (including the methods I tweak for external browser support). One or two rendering plugins could remain in KDevelop, but the QTB plugin could be an external project.

Maybe more can be put in a plugin, but then probably with a thorough API reorganisation; in that case support for an external browser could also be provided by an external plugin instead of by the host application.

This would make it easier to explore other (hypothetical) html/rich content renderers that are somewhere between QTB and QWE.

The problem is that different rendering backends may need different information from the documentation plugins. Cf. QTB: I need to obtain the HTML myself from the QtHelp engine/provider. That probably means you cannot just put the renderer in a plugin, not unless you accept extending the API when required.

By the time I was finishing my desert it was clear to me that I'd probably be breaking my last teeth on such a project, so I'm not volunteering to start working on it =)

Discussion has dried up a bit here, but got picked up (unintentionally from my part) on Qt's interest ML.

That raised the question: how suitable would KHtml be as a rendering backend? Initial tests using Konqueror5's khtml backend using non-qthelp-compressed doxygen apidocs suggest it renders that documentation as it should.

And something quite interesting I just picked up:

http://qtwebkit.blogspot.fr/2016/08/qtwebkit-im-back.html

You may be wondering, why anyone would like to use QtWebKit in 2016, when shiny new QtWebEngine is available? There is a number of reasons:

- When used in Qt application, QtWebKit has smaller footprint because it shares a lot of code with Qt. For example, it uses the same code paths for drawing and networking that your regular Qt code uses.
rjvbb updated this revision to Diff 22661.Nov 20 2017, 3:29 PM

rebased on 5.2 HEAD

rjvbb set the repository for this revision to R32 KDevelop.Nov 20 2017, 3:30 PM