[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: Re: Review Request: port Sonnet to QSettings
From: "Oswald Buddenhagen" <ossi () kde ! org>
Date: 2012-12-18 9:13:52
Message-ID: 20121218091352.28580.20755 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Dec. 17, 2012, 10:38 p.m., Kevin Krammer wrote:
> > IMHO this is wrong.
> > Not code wise but conceptual. As far as I understand QSettings is basic=
ally 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 th=
an this ought to be done by either having an interface that can be implemen=
ted through KConfig or by passing values as QVariant maps or hashes.
> >
> =
> Oswald Buddenhagen wrote:
> 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 a=
re passing the application's main config file).
> =
> Kevin Krammer wrote:
> "it's unfortunate that the chosen config class is part of the API."
> =
> Indeed. Most likely out of convenience. Hence the idea to just replac=
e 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?
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 s=
onnet 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 req=
uire lots of boilerplate code.
if you make it so that instantiating is nothing else than
new Sonnet::ConfigDialog(new KConfigWrapper(new KConfigGroup(KGlobal::con=
fig(), "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 wrapp=
er live?
or maybe upstream QMapInterface and make QSettings implement it, too? would=
it even fit the API?
- Oswald
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107791/#review23643
-----------------------------------------------------------
On Dec. 17, 2012, 9:15 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> =
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107791/
> -----------------------------------------------------------
> =
> (Updated Dec. 17, 2012, 9:15 p.m.)
> =
> =
> Review request for KDE Frameworks, kdelibs and David Faure.
> =
> =
> Description
> -------
> =
> Ported everything away from KConfig to QSettings.
> =
> I couldn't really find any users of the ::save function, so I think the s=
ource incompatible change would be worth it. Alternatively we could add a d=
eprecated dummy function that takes in a KConfig object and just ignores it=
, and uses QSettings.
> =
> =
> Diffs
> -----
> =
> kdeui/sonnet/configdialog.h 7c4993b =
> kdeui/sonnet/configdialog.cpp 625441b =
> kdeui/sonnet/configwidget.h 023b659 =
> kdeui/sonnet/configwidget.cpp 549d5af =
> kdeui/sonnet/highlighter.cpp 6cbb14c =
> kdeui/widgets/ktextedit.cpp 71d2a9f =
> staging/sonnet/src/core/backgroundchecker.h f0da3a3 =
> staging/sonnet/src/core/backgroundchecker.cpp dc05b94 =
> staging/sonnet/src/core/globals.cpp bf4f504 =
> staging/sonnet/src/core/loader.cpp 887aee5 =
> staging/sonnet/src/core/settings.cpp 59cb593 =
> staging/sonnet/src/core/settings_p.h e14bad7 =
> staging/sonnet/src/core/speller.h 37dd82f =
> staging/sonnet/src/core/speller.cpp f831f55 =
> =
> Diff: http://git.reviewboard.kde.org/r/107791/diff/
> =
> =
> Testing
> -------
> =
> it builds.
> =
> =
> Thanks,
> =
> Martin Tobias Holmedahl Sandsmark
> =
>
[Attachment #5 (text/html)]
<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>
</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;">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>
<br />
<p>- Oswald</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>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic