<table><tr><td style="">kossebau added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D8211" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>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.</p>

<p>BTW, have you tested already how this change works with kdev-php?</p>

<p>And all the additional ifdefs make life worse:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">adds another variant which is not covered by CI</li>
<li class="remarkup-list-item">developers fixing code will need to rebuild for any ifdef variant to test their changes</li>
<li class="remarkup-list-item">code is more complicated to understand, which it already is</li>
</ul>

<p>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.</p>

<p>So from a maintenance POV the given patch & approach sadly gets a -1 from me as co-contributor.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D8211#inline-35517" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rjvbb</span> wrote in <span style="color: #4b4d51; font-weight: bold;">standarddocumentationview.cpp:419</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I'll double-check. This will also contain to-be-moc-ed things from standarddocumentationview.h .</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">standarddocumentationview.h is covered by cmake's automoc already, given it's matching standarddocumentationview.cpp -> standarddocumentationview.h</p>

<p style="padding: 0; margin: 8px;">See <a href="https://cmake.org/cmake/help/v3.0/prop_tgt/AUTOMOC.html" class="remarkup-link" target="_blank" rel="noreferrer">https://cmake.org/cmake/help/v3.0/prop_tgt/AUTOMOC.html</a></p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D8211#inline-35519" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rjvbb</span> wrote in <span style="color: #4b4d51; font-weight: bold;">standarddocumentationview.h:74</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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.<br />
(I won't be the one doing that, so I don't care where we put this.)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Given we are flexible with the API per minor kdevelop/kdevplatform version, there is no need to prepare already for possible future extensions.</p>

<p style="padding: 0; margin: 8px;">So let's keep the API as minimal as needed.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D8211#inline-35520" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">rjvbb</span> wrote in <span style="color: #4b4d51; font-weight: bold;">standarddocumentationview.h:76</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">You're saying I have 2 simple choices here:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">remove #ifdef in the headerfile and provide (stub) implementations for the QtWeb* configuration</li>
<li class="remarkup-list-item">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.</li>
</ul>

<p style="padding: 0; margin: 8px;">Come to think of it, the 2nd option is probably the way to go in case there are contributed documentation plugins (are there?).</p>

<p style="padding: 0; margin: 8px;">Do we agree on the name or should I use something else?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.<br />
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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D8211#inline-35524" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">standarddocumentationview.h:89</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d">     */</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">void</span> <span style="color: #004012">restore</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#endif</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This name is too generic and needs to be more explicit, add to it what it restores.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D8211#inline-35525" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">standarddocumentationview.h:102</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #74777d">/**</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d">     * is @param url one using a supported scheme?</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d">     */</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">What is the requirement to be a supported scheme? The current hard-coded implementation seems rather made-up. What does "supported" mean? By whom?</p>

<p style="padding: 0; margin: 8px;">So "man", "help", "about", why are they supported? Why are others like "https" not?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D8211" rel="noreferrer">https://phabricator.kde.org/D8211</a></div></div><br /><div><strong>To: </strong>rjvbb, KDevelop<br /><strong>Cc: </strong>kossebau, aaronpuchert, flherne, arichardson, apol, kdevelop-devel, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger<br /></div>