optional external viewer support (based on Qt Assistant) in the QtHelp plugin
Needs ReviewPublic

Authored by rjvbb on Feb 7 2017, 7:03 PM.

Details

Reviewers
mwolff
kfunk
Group Reviewers
KDevelop
Summary

Please see the latest revision update message.

This patch provides a means for documentation plugins to use an external browser and inform the caller about that with an implementation for the QtHelp plugin (the standard documentation plugin where the feature makes most sense).
When such an external viewer is used, the embedded documentation toolview isn't opened or even initialised when you use the "Show documentation for" link in a QtHelp context help popup. The external browser is only called when necessary and its window can be configured (or minimised) independently from KDevelop). The result is less interface clutter and a work area that doesn't have to compromise between coding space and space for supporting features, while gaining access to a more powerful documentation browser dedicated to and designed for this task. Users preferring to use an external viewer could thus also build KDevelop without linking to huge dependencies like WebKit or WebEngine; QTextBrowser is powerful enough for the other standard documentation plugins (for manpages and cmake docs).

The Assistant's remote control mode is used, spawning a single instance per session which receives commands through its stdin. I have added a configuration option to en/disable the feature as well as to configure the path to the Assistant executable. This can of course be a wrapper script that adds additional arguments, like -collectionFile or even a different application that understands the same control protocol.

The intended use is via the "Show documentation for" link in QtHelp balloons. Since there is only a single lowlevel showDocumentation method, the external viewer is always triggered, even when you do open the embedded docu toolview and click on a QtHelp link in there. This is in accordance with the UI option name.

Test Plan

works as I'd expect it to on Mac and Linux.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
mwolff added inline comments.Feb 15 2017, 10:25 PM
qthelp/qthelpconfig.cpp
127 ↗(On Diff #11033)

this disqualifies this patch, it makes the feature unusable

if you want to get it in, you'll have to make sure it works without people writing scripts or symlinking stuff into their PATH

qthelp/qthelpproviderabstract.cpp
45 ↗(On Diff #11033)

move this feature into a separate file instead of cluttering the surrounding code

qthelp/qthelpproviderabstract.h
52 ↗(On Diff #11033)

make private, it's not usable outside the .cpp since it's a non-inline template

This revision now requires changes to proceed.Feb 15 2017, 10:25 PM
rjvbb added a comment.Feb 16 2017, 8:36 AM
In D4484#86658, @mwolff wrote:

This is not acceptable, as it would special-case Qt Help, while all other documentation providers would still be visible in the toolview.

Yes, and I haven't been able to deduce why Qt help documentation isn't used for things like CMake files even if the documentation in question is available. Must be some kind of filter on file types or extensions.

rjvbb added inline comments.Feb 16 2017, 8:58 AM
qthelp/qthelpconfig.cpp
127 ↗(On Diff #11033)

With the above comment this makes I'm not going to bother.
But to justify this approach: KDevelop is a tool that requires people to install a whole bunch of things (some of which have to be in the path AFAICT), and one that can safely assume that the intended user population is capable of doing that. So the choices are either to burden the code and UI with a mechanism to select the executable and specify the required arguments, or use a script that can be set up once and that could easily be installed in a generic version.

qthelp/qthelpproviderabstract.cpp
58 ↗(On Diff #11033)

You realise that this is only at application exit, when the GUI has already been taken down?

We don't use QCH for cmake but instead use cmake itself to provide us with the documentation. Similarly, we have plugins for PHP and Python documentation which is not in QCH, nor do I want to standardise on QCH. The format is terrible.

qthelp/qthelpconfig.cpp
127 ↗(On Diff #11033)

I don't understand why this cannot be QStandardPaths::findExecutable("assistant-qt5"), followed by a fallback to "assistant", of course wrapped in QStringLiteral

qthelp/qthelpproviderabstract.cpp
58 ↗(On Diff #11033)

no, but it makes no difference. KDevelop already takes too long to shutdown cleanly.

rjvbb added a comment.Feb 16 2017, 9:07 PM
In D4484#86826, @mwolff wrote:

nor do I want to standardise on QCH. The format is terrible.

What's wrong with it, at least when you're using an external viewer? It's basically HTML documentation, but more or less compressed and in a single file which makes it more space-efficient.

qthelp/qthelpconfig.cpp
127 ↗(On Diff #11033)

I thought about that, but there's a good chance the executable won't be in the path either (at least on MS Windows and Mac OS). And using a script adds the possibility to add a local -collectionFile argument, for instance to point the assistant to a collection that corresponds to KDevelop's QCH help directory.

qthelp/qthelpproviderabstract.cpp
58 ↗(On Diff #11033)

I only put in the waits to avoid getting error messages about terminating a running process. Under normal circumstances the delay should not be noticeable as the assistant will react immediately to a SIGHUP. Making the shutdown asynchronous is complete overkill if you ask me, and may not even make much difference depending on when the ExternalViewer dtor is called.

rjvbb updated this revision to Diff 20597.Oct 11 2017, 3:08 PM

rebased/bump.

Maybe this can serve in an adapted version together with D8211, giving users the option to open relevant documentation in a full-scale and standalone documentation browser. The current implementation has been providing my main interface to Qt's documentation ever since I submitted this patch.

rjvbb set the repository for this revision to R32 KDevelop.Oct 11 2017, 3:09 PM
rjvbb updated this revision to Diff 20740.Oct 14 2017, 2:55 PM

This revision finally implements the feature how I've always wanted it to work: on demand via the context help popup provided by documentation plugins supporting the feature. (Instead of automatically *instead* of opening that popup.)

When use of an external browser is activated the Show documentation for QFoo link in the context help popup now triggers the external browser instead of opening the embedded documentation toolview (OTOH this toolview is of course still used when use of an external viewer is disabled). In fact, the toolview isn't even initialised when an external browser is used, which also means it isn't added to a toolbar when the user removed it.

This requires a minor modification to the IDocumentation class, IDocumentation::viewInExternalBrowser() (implemented only in the QtHelp plugin, where this feature makes most sense).

The patch still uses a single external viewer instance per session which is terminated on exit. The viewer command is still hardwired, but I propose to replace the simple "use: yes/no" checkbox with a widget to select a viewer application (reusing the Path as a state variable to decide whether an external viewer is to be used).

As argued before, I see this as a good complement to an embedded documentation viewer that's based on QTextBrowser, a step beyond what's possible with a floating embedded doc viewer, even if it uses regular windows. Qt's Assistant supports having multiple tabs open for instance, has better search and filtering facilities, and can host a centralised (shared) collection of QHC documentation from various locations, accessible without having to start KDevelop (or add them in KDevelop too).

rjvbb retitled this revision from support Qt's assistant as an optional external Qt help viewer to external viewer support (based on Qt Assistant) in the QtHelp plugin (optional) (WIP).Oct 14 2017, 3:04 PM
rjvbb edited the summary of this revision. (Show Details)
rjvbb set the repository for this revision to R32 KDevelop.
rjvbb updated this revision to Diff 20816.Oct 15 2017, 7:21 PM

refactored to put the external viewer implementation in its own files instead of piggybacking on QtHelpProviderAbstract.

rjvbb edited the summary of this revision. (Show Details)Oct 15 2017, 7:23 PM
rjvbb set the repository for this revision to R32 KDevelop.
kfunk requested changes to this revision.Oct 15 2017, 8:39 PM
kfunk added a subscriber: kfunk.

Just a quick review.

plugins/qthelp/qthelpconfig.cpp
126

This is over-engineered. No one will find or use that feature.

This should call assistant directly, if at all.` The path to assistant can be deduced from the Qt prefix used by the project and use system-wide assistant as a fallack.

plugins/qthelp/qthelpdocumentation.cpp
308

This would be way cleaner if you would just call QtHelpExternalViewer::setSource(QUrl) where QtHelpExternalViewer::setSource(...) would wrap the IPC interface to assistant (i.e. move below code there).

plugins/qthelp/qthelpexternalviewer.cpp
47 ↗(On Diff #20816)

Leaks.

61 ↗(On Diff #20816)

Why is this needed?

65 ↗(On Diff #20816)

You can use nullptr. No need to use 0, NULL, or Q_NULLPTR. Please fixup the rest of the patch, too.

plugins/qthelp/qthelpexternalviewer.h
1 ↗(On Diff #20816)

This class is a wrapper to Qt assistant, but the word 'assistant' appears nowhere. Wouldn't it make sense to rename to e.g. QtAssistantExternalViewer?

32 ↗(On Diff #20816)

This class' design is super convoluted and unusual (It does two things: spawning & killing the external process + offering the API to control the process.).

Just don't inherit from QProcess but QObject and due to the refactoring you'll need to make the architecture will be cleaned up automatically.

41 ↗(On Diff #20816)

If you introduce setSource as indicated in one of my other comments, you don't need these two methods either.

43 ↗(On Diff #20816)

const bool -> bool

46 ↗(On Diff #20816)

Why public?

This revision now requires changes to proceed.Oct 15 2017, 8:39 PM

Some quick reactions and a few requests for clarification.
If you agree with the idea of using a user-selected path to the assistant I'll try to include that in the next revision along with the requested changes.

Thanks for picking this up, btw.

plugins/qthelp/qthelpconfig.cpp
126

Yeah, this works for me and as a PoC, not as a definitive solution.

From having used this patch for months I would now suggest to let the user select the path to the assistant to be used. The checkbox in the config dialog thus becomes a path selector, and not setting that path will then equal not activating external viewer support.

Figuring out the Qt prefix used by the project seems unnecessarily complex (if not only when there are multiple projects in a session), and I don't think there's something like a system-wide assistant even on Plasma desktops. This is even more true on Mac where the assistant executable lives in an app bundle that could be everywhere.

Another thing to keep in mind is that the assistant remote control protocol doesn't allow to change collections on the fly. That's why I currently use a script: it allowed me to set up a help collection for Assistant that corresponds to the one KDevelop works with (which isn't an actual qhc collection as far as I understand). That allows me to a single assistant copy.

plugins/qthelp/qthelpdocumentation.cpp
308

Indeed, that should be trivial.

plugins/qthelp/qthelpexternalviewer.cpp
61 ↗(On Diff #20816)

Because I'm using a single viewer instance. It's under remote control but the user can also quit it, or it could crash. Without this slot you'd have to restart KDevelop to start the external browser anew.

plugins/qthelp/qthelpexternalviewer.h
1 ↗(On Diff #20816)

I suppose it would, if it's OK to introduce a class with a name that doesn't start with QtHelp?

32 ↗(On Diff #20816)

Inherit QObject and then (evidently?) make s_externalViewer a QProcess*?

May I ask what's unusual about this class? I wanted it to represent the external viewer process, behaving as you'd expect a browser to behave when you open a link (= start a copy if not already running). (The DocumentationController does something similar when it adds the Documentation toolview to a toolbar, no?)

46 ↗(On Diff #20816)

that's probably a remnant from an early implementation.

rjvbb marked 8 inline comments as done.Oct 16 2017, 9:05 AM

Waiting for feedback on the application selection and UI implementation.

plugins/qthelp/qthelpdocumentation.cpp
308

Done, but called openUrl following QDesktopServices. setSource would suggest there should be a getter, and I don't think there's a point to that (we can only know which address was last set programmatically, not what the user did afterwards).

plugins/qthelp/qthelpexternalviewer.h
1 ↗(On Diff #20816)

Compromise: QtHelpExternalAssistant. I'd like to keep referring to it as an external viewer (or browser) in the UI though, and thus also in the use state symbols.

kfunk accepted this revision.Oct 16 2017, 9:13 AM
kfunk added inline comments.
plugins/qthelp/qthelpexternalviewer.h
1 ↗(On Diff #20816)

+1. Whatever. Renames are cheap :)

kfunk requested changes to this revision.Oct 16 2017, 9:13 AM

Sorry, didn't intend to mark this as accepted (yet).

rjvbb added a comment.Oct 16 2017, 9:26 AM
Sorry, didn't intend to mark this as accepted (yet).

Hadn't even noticed (yet). Sounds encouraging, though. :) I really appreciate the added comfort of using an external doc browser; Assistant may not be perfect but it's clearly been designed for its task.

little change of mind: I'll keep the checkbox but add an application picker and will be working on that this afternoon. I imagine an additional method, hasExternalViewer() which will check if a user-provided path is set or else tries to locate the command. I'll probably have to add an ObjC module with a Mac-specific locating implementation...

That solution will make it possible to use both the external viewer and the embedded viewer (via an "open in external viewer" context menu action in the embedded viewer). If we're going for flexibility, just as well go as far as reasonable :)

rjvbb updated this revision to Diff 20876.Oct 16 2017, 8:04 PM

Updated as requested and discussed.

There's now a KUrlRequested widget in the QtHelp plugin config dialog (not exactly where I'd have liked it to be, with everything on a single line). It has a short explicative placeholder text which also shows the path to the Assistant executable that would be used (if one is found). My QTextBrowser patch for the documentation toolview shows how this info could be used to add an "open in external viewer" action when a valid executable is configured or found.
The checkbox preserves its previous function, but the tooltip now explains how the external viewer will be used instead of the embedded toolview. The embedded toolview is still used as a fallback, in case something is or goes wrong with the external viewer.

QtHelpExternalAssistant will now use either the executable configured by the user, or the first executable called assistant found on the path. On Mac it has a double fall-back: it will first try to locate a copy of a Qt5 Assistant via its bundle-id, and then any bundle called Assistant.app .

As far as I can tell this is now a feature-complete implementation.

rjvbb edited the test plan for this revision. (Show Details)Oct 16 2017, 8:06 PM
rjvbb set the repository for this revision to R32 KDevelop.
anthonyfieroni added inline comments.
plugins/qthelp/qthelpexternalassistant.cpp
50–59

Why not call s_externalViewerProcess->kill ?

128–130

Here you should deleted s_instance too or when you use like in openUrl

if (instance()) {

you will hit a crash in dereferencing s_externalViewerProcess

rjvbb marked an inline comment as done.Oct 17 2017, 9:10 AM
rjvbb added inline comments.
plugins/qthelp/qthelpexternalassistant.cpp
50–59

I'd stick with QProcess::terminate() if I were to use a one-fits-all approach, but I'm trying to quit the external viewer as gracefully as possible. Traditionally that'd be with a hangup signal (AFAIK that's what the X server still sends to its clients when it is shutting down). The Assistant does have some internal state that's saved under normal operation, so I don't want to kill it off too bluntly.

I've filed a request to add a Quit command to the remote control protocol.

128–130

Actually, s_instance is being deleted at this very point, but I see what you mean. I missed a few other details due to no longer deriving QProcess but using a QProcess member var instead. Should be fixed now.

Just for the record: this dtor is called very late in the shutdown procedure, in practice it always happens after all projects have been closed. And for me the mainwindow is "long" gone by the time projects are being unloaded. You'd really have to try hard to call openUrl() deliberately after this point.

rjvbb updated this revision to Diff 20898.EditedOct 17 2017, 9:52 AM
rjvbb marked an inline comment as done.

addresses a potential race condition/crash : the QtHelpExternalAssistant dtor no longer leaves (potential) stale member pointers around, and the class disables itself when shutdown is in progress.

rjvbb set the repository for this revision to R32 KDevelop.Oct 17 2017, 9:52 AM
mwolff requested changes to this revision.Nov 19 2017, 10:07 PM

Please update the summary for this patch, it's outdated to what the patch does it seems (e.g. kdevelop-qthelp-viewer cannot be found anywhere in the patch).

Also, I personally consider this a bad stop-gap workaround to limitations in KDevelop's toolviews and potentially qthelp plugin. Instead of making the code more complex by adding yet more options, I'd much prefer we solve the actual underlying issues. Can you give us the motivation for _why_ you want to show stuff in an external application? Please do that before attending the code review - I hope we don't need to integrate this patch ever.

kdevplatform/interfaces/idocumentation.h
69

do not add non-pure stuff to an interface

plugins/qthelp/CMakeLists.txt
22

files are not part of the patch, why are they even required? why do you need special code for mac?

plugins/qthelp/qthelp_config_shared.cpp
39

I'd prefer if this setting becomes global, and not per-plugin. Plugins can then decide whether they are capable of external viewing or not.

plugins/qthelp/qthelp_config_shared.h
26

write C++ code, not C

plugins/qthelp/qthelpexternalassistant.h
85

use the static instance, instead of making all the other members static too

better yet, don't make it a singleton at all, but a member of the QtHelp plugin and hand it down to where it's being used

plugins/qthelp/qthelpplugin.cpp
44

not needed

plugins/qthelp/tests/CMakeLists.txt
20

dito

plugins/qthelp/tests/test_qthelpplugin.cpp
61–62

add proper defaults to the struct, so that you don't need to do any default initialization here

This revision now requires changes to proceed.Nov 19 2017, 10:07 PM

Can you give us the motivation for _why_ you want to show stuff in an external application?

I think I made that clear in all the exchanges we've had about rendering the documentation but let me give it another try.
For me there are 3 main reasons. In no particular order:
Using an external, dedicated application means KDevelop

  • doesn't have to link to big, resource-hungry rendering frameworks (WebKit or WebEngine)
  • doesn't have to implement indexing and search features to peruse the available documentation
  • doesn't have to provide the interface - which also means that personal taste questions like "is a floating window the best option" become moot.

For Qt Help there's an additional argument on Mac: all Qt5 versions have an unidentified bug in the QtHelp library that cause severe file descriptor depletion when the help collection becomes too big (the current set of .qhc files shipping with Qt must be close to the limit). This can ultimately lead to crashing - I prefer to see that happen to an external helper than to the IDE itself.

Qt's Assistant isn't the best documentation browser I've ever seen, but still it's clearly designed for the task, and better at it than the implementation provided by the QtHelp plugin. The documentation toolview is hardly more powerful than the documentation pop-ups, in my experience. Those pop-ups are one of the main reasons I use KDevelop, but if I need more I prefer to use their "Show documentation for ..." link to jump to something designed for the task and that has access to the full documentation collection.

I agree that the "use an external viewer" preference could be a global setting, but not necessarily. Each plugin will probably have its own external viewer (if one exists), and I'm not sure if we should force users to use external viewers for all help providers if they want to use one for, say, QtHelp.

WOuldn't this be the kind of feature where you'd want to see how it works and is received in the wild? IOW, can't this be polished up to make it acceptable without requiring a complete overhaul or changes that require modifications to I don't know how many unrelated files? It can always be removed again (esp. if the footprint remains a small as possible).

About the "write C++ not C" remark - I presume that's about the fact I'm using a struct. Structs exist in both languages, I used one here because I think it's more appropriate for something that just acts as a container for a couple of variables. Is there an added benefit to using a class here?

rjvbb added a comment.Nov 20 2017, 1:33 AM

Bummer, I must have forgotten to git-add that file.

The file is needed because finding Qt Assistant (when no viewer is configured) works differently on Mac (will become clear when I add the missing file) and requires using ObjC. It might be possible to use an ObjC++ file for all platforms; I *think* that no ObjC runtime is required when you don't actually use ObjC-style messaging inside an ObjC file. But I'll only be able to verify that on Linux.

rjvbb added inline comments.Nov 20 2017, 2:04 PM
kdevplatform/interfaces/idocumentation.h
69

You mean "[please] don't provide an implementation (not even in idocumentation.cpp)"? I'll defer that to when this is getting ready to be accepted. Until then I'll keep the patch footprint and the number of file-changes I have to maintain as small as possible

Out of curiosity: is there an interest here not to benefit from the obvious opportunity to set sensible defaults (at least for non-critical features), obliging every descendant to implement the full API which can hardly help keeping code and binary size compact?

rjvbb marked 8 inline comments as done.Nov 20 2017, 7:33 PM
rjvbb added inline comments.
plugins/qthelp/qthelp_config_shared.cpp
39

marking that as a TODO for now

plugins/qthelp/qthelpexternalassistant.h
85

Can't say I like the idea of handing the instance down everywhere it's needed, as if there could be multiple external viewers.

I've done something intermediate here (replacing instance with self), but the result is not really elegant either.

rjvbb updated this revision to Diff 22675.Nov 20 2017, 7:34 PM
rjvbb marked an inline comment as done.

updated as discussed in the inline comments.

rjvbb edited the summary of this revision. (Show Details)Nov 20 2017, 7:50 PM
rjvbb set the repository for this revision to R32 KDevelop.
rjvbb added a comment.Dec 14 2017, 9:55 AM

I realised that KHelpCentre can be used very easily as an external viewer, is already used by KDevelop and already handles man:/foo URIs : https://bugs.kde.org/show_bug.cgi?id=387891

KHelpCentre does have a few drawback: there could be an over-abundance of unrelated documentation ("can't see the forest through the trees" effect), it doesn't have tab support and it's typically used in a single instance per desktop session.

Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptDec 14 2017, 9:55 AM
rjvbb retitled this revision from external viewer support (based on Qt Assistant) in the QtHelp plugin (optional) (WIP) to optional external viewer support (based on Qt Assistant) in the QtHelp plugin.Nov 13 2018, 10:42 AM