D8211: KDevelop/Documentation : implementation of a QTextBrowser-based viewer

[Friedrich W. H. Kossebau]

kossebau added a comment.


  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.

INLINE COMMENTS

> rjvbb wrote in standarddocumentationview.cpp:419
> I'll double-check. This will also contain to-be-moc-ed things from standarddocumentationview.h .

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

> rjvbb wrote in standarddocumentationview.h:74
> 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.)

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.

> rjvbb wrote in standarddocumentationview.h:76
> 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?

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.

> standarddocumentationview.h:89
> +     */
> +    void restore();
> +#endif

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

> standarddocumentationview.h:102
> +    /**
> +     * is @param url one using a supported scheme?
> +     */

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?

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D8211

To: rjvbb, #kdevelop
Cc: kossebau, aaronpuchert, flherne, arichardson, apol, kdevelop-devel, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20171019/f0336a88/attachment.html>