<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/119997/">https://git.reviewboard.kde.org/r/119997/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 31st, 2014, 12:57 a.m. EDT, <b>Matthew Dawson</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks for taking a look at this. It appears KConfigBase isn't available on api.kde.org as it isn't documented, as kapidox hides such classes by default. As KConfigBase is used outside of KConfig, I'd prefer if KConfigBase gained a quick blurb to copying its documentation. I think something describing it as a way for a function to accept either a KConfig or a KConfigGroup and be able load/persist its settings would be good.</p></pre>
</blockquote>
<p>On August 31st, 2014, 7:38 a.m. EDT, <b>Martin Klapetek</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KConfigBase is very much documented, see [1]. I don't know why it's not showing up...nevertheless, if that method is reimplementation of the base class method, it should imho has its own documentation altogether (it's *reimplementation* after all). What I did here is just a quick fix to improve that...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">[1] - http://quickgit.kde.org/?p=kconfig.git&a=blob&h=3d00d98a1eb565df1a6eea2eba165bfeda17978b&hb=5941a608038d799244ba2dfdceb2da8bf1685fc1&f=src%2Fcore%2Fkconfigbase.h</p></pre>
</blockquote>
<p>On August 31st, 2014, 12:25 p.m. EDT, <b>Matthew Dawson</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">While class members of KConfigBase are documented, Doxygen doesn't consider it documented as the class itself does not have a description attached. Attaching even a brief line convinces Doxygen otherwise.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Regarding each method having its own documentation, I took a quick look through the other code I have checked out on my machine, and it appears most classes don't copy the documentation from their parent. I couldn't find any policy on this either from KDE's website. If we want each function in frameworks to be fully documented, without having to click a reference, then I'm fine with this patch. This should then probably get documnted on the community wiki, so others know for the future to include it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">However, if this patch came from the fact KConfigBase is not on api.kde.org, then I rather fix this by throwing a quick description on KConfigBase. Then the sync method's documentation does show up, and a link is generated on KConfig's sync method making it easy to read. Maybe something like:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">\brief Interface to interact with configuration.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KConfigBase allow a component of an application to persist its configuration, without having to care if it persists its data into a top level KConfig, or a KConfigGroup inside a KConfig.</p></pre>
</blockquote>
<p>On September 1st, 2014, 6:36 a.m. EDT, <b>Martin Klapetek</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">it appears most classes don't copy the documentation from their parent.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Right; my point is that a reimplementation of an abstract class method/interface method is usually a specific implementation of the broad parent abstract class meaning.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If we want each function in frameworks to be fully documented... if this patch came from the fact KConfigBase is not on api.kde.org</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In the light of what I wrote above, my intention was a little bit of both.</p></pre>
</blockquote>
<p>On September 1st, 2014, 6:52 a.m. EDT, <b>Aleix Pol Gonzalez</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Well, if @reimp is passed, it would mean that the parent documentation is good enough, no?</p></pre>
</blockquote>
<p>On September 1st, 2014, 12:34 p.m. EDT, <b>Martin Klapetek</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I thought that "@reimp" in doxygen marks the method as reimplemented from the parent class, ie. it adds a link to it. No?</p></pre>
</blockquote>
<p>On September 6th, 2014, 6:32 p.m. EDT, <b>Matthew Dawson</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I agree that if the specific implementation have further notes that the parent's documentation does not address, it would be good to document it. However, the current patch only copies the documentation from KConfigBase. KConfigBase isn't on api.kde.org it is not considered documented by Doxygen (regardless of what Doxygen tags are attached now). Attaching a quick couple lines to KConfigBase would make it visible, avoiding the copied comment. If my suggestion above is fine, then I'll commit it. If there is further documentation that should be attached to KConfig::sync, this RR can be updated appropriately.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I do think it would be good to attach some extra documentation to KConfigGroup::sync, mentioning it syncs the entire file the group is from, as that might surprise some people. That can be included either in this RR, or I can add something with KConfigBase's documentation commit.</p></pre>
</blockquote>
<p>On September 11th, 2014, 8:44 a.m. EDT, <b>Martin Klapetek</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">However, the current patch only copies the documentation from KConfigBase</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, I wanted to do a "quick fix" and as I don't know much about the internals, I just copied the base class' docs, which seems fitting enough.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ultimately, you're the maintainer, so it's up to you if you add docs to KConfigBase or extend the sync() comment properly. I mostly wanted to point out the missing docs and have it fixed quickly. I do agree that it's not the bestest proper fix ever and I'm happy to discard it for more favorable solution :)</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Alright, I've pushed a commit (http://commits.kde.org/kconfig/fe300454292c519ae5aa4edfcb1b94f3636e59bb) so KConfigBase shows up (and a link now exists between KConfig::sync and KConfigBase::sync). KConfigGroup::sync already had a note, so I didn't change it for now. If you feel it can be helped (the KConfig::sync link isn't obvious, unfortunately), please update this RR. Otherwise please discard this one.</p></pre>
<br />
<p>- Matthew</p>
<br />
<p>On August 29th, 2014, 5:42 p.m. EDT, Martin Klapetek wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Martin Klapetek.</div>
<p style="color: grey;"><i>Updated Aug. 29, 2014, 5:42 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kconfig
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The docs at api.kde.org shows just "Reimplemented from superclass." for the sync() method, however the superclass, KConfigBase, is not available at the api.kde.org. So this copies the documentation from KConfigBase to KConfig so the text can actually be visible at api.kde.org</p></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>src/core/kconfig.h <span style="color: grey">(d7d4b7d)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119997/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>