<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/115959/">https://git.reviewboard.kde.org/r/115959/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 1st, 2014, 11:15 a.m. UTC, <b>Alex Merry</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The implementation all looks fine. The only concern I have is that it's an odd location for it; I wouldn't expect to go looking for a method to invoke Help in KConfigWidgets. Although I'm not sure where it would go instead, given the reliance on QtGui and KConfig.
Although, maybe is could go in KConfigGui? It's still a bit out of place, but it sort of fits in KConfig if you think of it as accessing a bit of system/application configuration ("where to find help").</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Yeah I thought about that alternative. We already abuse KConfigGui for kstandardshortcut, kwindowconfig, etc. -- i.e. things that are there for dependencies reasons, not because they are in the expected feature list for the KConfig framework.
So I tried to not keep abusing it...
One scary way to look at the issue is: what if one day we want to replace KConfig with another configuration technology? Then all that stuff we bundle into KConfigGui (i.e. into the KConfig framework itself), will be in the wrong place, since we'll want these APIs to keep existing, just not with kconfig as the underlying technology.
(OTOH KConfigWidgets is a different framework, it's "widgets with a need for configuration", doesn't have to be tied to KConfig as the underlying implementation)
I know, I'm saying here that we did it wrong for kstandardshortcut and kwindowconfig, where I was probably the one selecting the current situation...
I also don't honestly think that moving away from KConfig is a plan (but rather providing any new technology from within the KConfig API instead).
I picked KConfigWidgets using the same logic as to why it was in xmlgui before: because that's where it's needed, at the lowest level.
I can be convinced for KConfigGui, but it's not ideal either, for the reasons above.
</pre>
<br />
<p>- David</p>
<br />
<p>On February 23rd, 2014, 11 a.m. UTC, David Faure wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks and Albert Astals Cid.</div>
<div>By David Faure.</div>
<p style="color: grey;"><i>Updated Feb. 23, 2014, 11 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kconfigwidgets
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">3 commits:
Unittest: make errors readable
--
Resurrect KConfigDialog::setHelp (used to come from KDialog).
It controls the default behavior of showHelp(), which is implemented
using KHelpClient.
REVIEW: 115959
--
Move KHelpClient down from kxmlgui, for use in KConfigDialog.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Compiled all of KF5.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>autotests/kconfigdialog_unittest.cpp <span style="color: grey">(e5322c1782c2a68c15451777066e28a9b7afea23)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(7da7fba0c15153d6dee381c2b8f282e9837eae36)</span></li>
<li>src/kconfigdialog.h <span style="color: grey">(b06efc588c772ed655d581a0e021d92af5e0e280)</span></li>
<li>src/kconfigdialog.cpp <span style="color: grey">(8db48e23f614530cef11a23a182b50d905327405)</span></li>
<li>src/khelpclient.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/khelpclient.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115959/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>