<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="http://git.reviewboard.kde.org/r/107791/">http://git.reviewboard.kde.org/r/107791/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 17th, 2012, 10:38 p.m., <b>Kevin Krammer</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;">IMHO this is wrong.
Not code wise but conceptual. As far as I understand QSettings is basically deprecated, it is just not official marked as such because there is no replacement. This would be porting away from a fully functional, way more advanced and maintained settings.
If the sole goal of this endavor is to remove the KConfig dependency than this ought to be done by either having an interface that can be implemented through KConfig or by passing values as QVariant maps or hashes.
</pre>
</blockquote>
<p>On December 17th, 2012, 10:51 p.m., <b>Oswald Buddenhagen</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;">and where exactly do you see that kconfig maintainer? ;)
it's unfortunate that the chosen config class is part of the API.
judging by uses, would it be reasonable to just disable that part of the API indefinitely?
less drastically, would it be acceptable to pass a file name instead of a config object? that would of course incur some overhead (assuming we are passing the application's main config file).</pre>
</blockquote>
<p>On December 18th, 2012, 8:50 a.m., <b>Kevin Krammer</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;">"it's unfortunate that the chosen config class is part of the API."
Indeed. Most likely out of convenience. Hence the idea to just replace it with a generic key/value object that doesn't do any specific form of I/O and can allowing the using application to decide on persistant storage. But if I understand you correctly you would rather go for the full solution and make those properties directly read-/writable, right?</pre>
</blockquote>
<p>On December 18th, 2012, 9:14 a.m., <b>Oswald Buddenhagen</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 idea with the file name has the advantage that it is most natural, but sort of slow, and it may actually not work - if the app uses kconfig, but sonnet uses qsettings on the same file, you may actually get garbage out of it.
i'd strongly recommend not using a variant map, etc., as using it would require lots of boilerplate code.
if you make it so that instantiating is nothing else than
new Sonnet::ConfigDialog(new KConfigWrapper(new KConfigGroup(KGlobal::config(), "Spellchecking")));
it may be ok. still a bit ... weird.
you could make kconfiggroup directly implement the interface, but then you'd get an asymmetry with qsettings.
also, where would KMapInterface be defined? where would the qsettings wrapper live?
or maybe upstream QMapInterface and make QSettings implement it, too? would it even fit the API?
</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;">What about not exposing any of the config storage details at all? We have the application name, that should be enough to store application specific settings.</pre>
<br />
<p>- Martin Tobias Holmedahl</p>
<br />
<p>On December 17th, 2012, 9:15 p.m., Martin Tobias Holmedahl Sandsmark wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks, kdelibs and David Faure.</div>
<div>By Martin Tobias Holmedahl Sandsmark.</div>
<p style="color: grey;"><i>Updated Dec. 17, 2012, 9:15 p.m.</i></p>
<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;">Ported everything away from KConfig to QSettings.
I couldn't really find any users of the ::save function, so I think the source incompatible change would be worth it. Alternatively we could add a deprecated dummy function that takes in a KConfig object and just ignores it, and uses QSettings.</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;">it builds.</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>kdeui/sonnet/configdialog.h <span style="color: grey">(7c4993b)</span></li>
<li>kdeui/sonnet/configdialog.cpp <span style="color: grey">(625441b)</span></li>
<li>kdeui/sonnet/configwidget.h <span style="color: grey">(023b659)</span></li>
<li>kdeui/sonnet/configwidget.cpp <span style="color: grey">(549d5af)</span></li>
<li>kdeui/sonnet/highlighter.cpp <span style="color: grey">(6cbb14c)</span></li>
<li>kdeui/widgets/ktextedit.cpp <span style="color: grey">(71d2a9f)</span></li>
<li>staging/sonnet/src/core/backgroundchecker.h <span style="color: grey">(f0da3a3)</span></li>
<li>staging/sonnet/src/core/backgroundchecker.cpp <span style="color: grey">(dc05b94)</span></li>
<li>staging/sonnet/src/core/globals.cpp <span style="color: grey">(bf4f504)</span></li>
<li>staging/sonnet/src/core/loader.cpp <span style="color: grey">(887aee5)</span></li>
<li>staging/sonnet/src/core/settings.cpp <span style="color: grey">(59cb593)</span></li>
<li>staging/sonnet/src/core/settings_p.h <span style="color: grey">(e14bad7)</span></li>
<li>staging/sonnet/src/core/speller.h <span style="color: grey">(37dd82f)</span></li>
<li>staging/sonnet/src/core/speller.cpp <span style="color: grey">(f831f55)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107791/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>