[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: [RFC] KConfig API stuff
From: "Thomas Braxton" <kde.braxton () gmail ! com>
Date: 2007-10-21 16:58:58
Message-ID: 928c3f1b0710210958h7c2f663ej867d06f326f42a1b () mail ! gmail ! com
[Download RAW message or body]
On 10/21/07, Oswald Buddenhagen <ossi@kde.org> wrote:
>
> moin,
>
> as you probably know, puthuhn (bernhard loos) and me are fixing KConfig.
> while i'm pretty confident that nobody cares for the backend stuff we
> are doing, ;) there are also some API changes in the works.
> it's "a bit" late in the release process, so we made sure to get a
> preliminary OK from allen in advance.
> the additional effort for people not involved should be almost zero, as
> we do the complete porting - and this time you can expect quality work -
> after all, *i* am on it. ;)
> however, as this grows into unexpected dimensions, i'd like to discuss
> the changes in detail before we pursue this any further:
since you seem to think I'm a less than capable coder, I'll leave it to you.
After all you're the one with the kde.org address. But, I'll reply to this
email then I won't touch KConfig* again.
- kill kconfigbase, make kconfig inherit kconfiggroup.
It wasn't my idea, puthunh convinced me to readd it to the heirarchy.
kconfigbase was gone from my branch.
- rationale: it is The Right Thing To Do (TM). the current asymmetry
> of kconfig offering the group functions but no entry functions is
> just awkward. the kconfig object itself is the <default> group (the
> root group, or rather, the non-group) and should behave as such.
actually I would say that the kconfig object is not the default group, but
is rather the collection of *all* groups. groups also don't care about the
backend. this seems like making a collection of items a subclass of the
item. that's why kconfig offers the group functions, it is a collection of
groups. a group is a collection of entries, that's why kconfiggroup offers
the entry functions. the reason kconfiggroup also offers the group functions
is because a group can have subgroups.
- problem: while at first it seemed like porting would be a trivial
> search&replace in most cases, a "slight" problem turned up: kconfig
> becomes implicitly shared, as kconfiggroup is. as a consequence, the
> kconfiggroup constructors would take const KConfigGroup &master
> instead of KConfigBase *master and a lot of explicit memory management
> would need to be adapted.
> so while as such this is definitely a feature, the porting effort is
> just horrendous.
> => without more helping hands, this is a no-go
> - alternatives? maybe like this: kconfigbase dies and thus group
> functions become unavailable via kconfig. no big loss: one can
> already create a separate root group object - it would simply become
> the only way. the porting effort should be *very* moderate.
> - problems: it's uglier than the first variant
> => no-go unless first option is unfeasible
> - renames in kconfiggroup: name to groupName, exists to groupExists,
> rename in kconfig: name to configName
> - rationale: clearly necessary with the changed hierarchy. does it
> make sense without it?
if you say so. if they aren't going to have a common ancestor, the current
names are still valid, why would you ask a group what it's groupName is?
group.name() makes more sense than group.groupName(). config.name() makes
more sense than config.configName(). is there another name that they are
going to return in another function that requires the longer names?
> => ?
> - rename kconfiggroup::clean back to to rollback.
> - rationale: whoever renamed it had no idea of database terminology
you're right I don't know database terminology, but I know enough to know
that to rollback a db means to return to some previous state, which this
doesn't do. That's why I thought the name should be changed, to reflect what
it actually does and not pretend it does more.
=> go
> - remove kconfig::setGroup & kconfig::group
> - rationale: the api sucks and is deprecated about forever.
> additionally, in the current trunk it is outright defunct.
> - problem: still some users left to port, but within reason
> => still on go
agree
- remove kconfiggroup::changeGroup
> - rationale: the api sucks - it is about the same as kconfig::setGroup
> - problem: quite some users
> => prolly no-go, will stay with deprecation
agree
- remove kconfiggroup::group
> - rationale: it is just a weird way to call the constructor:
Actually I was thinking of going the other way, to make read-only groups
work correctly. make all the constructors protected except KConfigGroup() &
KConfigGroup(const KConfigGroup&).
cg = KConfig(...).group("blah") => cg = KConfigGroup( KConfig(...),
> "blah" )
> KConfigGroup cg( KConfig(...).group("blah") ) => KConfigGroup cg(
> KConfig(...), "blah" )
> - problem: quite some users
> => prolly no-go, will go with deprecation
> - remove kconfiggroup::sync
- rationale: the api suggests a granularity that is simply not given.
> cg.config().sync() is the proper call.
kinda agree
- problems:
> - quite some users. manageable, though.
> - more verbose code. do we want it?
> => ?
> - remove kconfiggroup::getConfigState
Again, another addition from puthuhn.
- rationale: the api suggests a granularity that is simply not given.
> cg.config().getConfigState() is the proper call.
> => go
> - rename kconfig::getConfigState to accessMode
> - rationale: states: noAccess, readOnly, readWrite ... no comment
agree
=> go
> - rename kconfiggroup::entryIsImmutable to isEntryImmutable
> - rationale: better, right?
works either way.
=> go
> - move kconfiggroup::setReadDefaults to kconfig
> - rationale: all users (mostly kconfigskeleton derivatives) deal with
> the
> entire file at once anyway
- problem: it's ugly and cries "i'm not reentrant"
another problem, how do you ensure that it is set on every group that comes
from a config object that is supposed to be reading defaults only?
=> no-go?
> - remove kconfig::setForceGlobal
> - rationale: only user is kfile/kfilewidget.cpp. the alternative api
> is passing the KConfigGroup::Global flag to writeEntry calls.
actually it's also set whenever "kdeglobals" is opened explicitely, so that
whoever is using it doesn't have to pass KConfigGroup::Global to every
writeEntry call.
- alternatively, one might view it just like setReadDefaults and move
> it to kconfiggroup instead.
> => go?
> - remove kconfiggroup::NLS
> - rationale: it is an alias for Localized. the internal KEntry uses
> the Localized terminology, too, so for the sake of consistency ...
> => go
either way.
- remove kconfig OnlyLocal and NoGlobals
> - rationale: they are aliases for SimpleConfig resp. CascadeConfig.
> it's more to learn and the negative meanings are potentially
> confusing.
That's why I changed the names.
=> go
> - remove the separator argument from {read,write}Entry for lists
> - rationale: the separator is a backend internal detail. with some
> backends, it might be plain unchangeable.
agree
- problems: none
> - non-problems:
> - there are some abuses of the separator argument and some plain
> pointless uses. easily fixable.
> - it was suggested that this would imply changing the kde-wide
> separator back to the semicolon, as .desktop files use the
> semicolon and kdesktopfile is in fact a kconfig, so we'd have no
> way to get it right otherwise.
> in fact, we might provide an api (possibly accessible by
> kdesktopfile only) to put the entire file into .desktop mode -
> that makes most sense anyway.
> however, if somebody wants to go with semicolons kde-wide (hi
> maelcum ;), the same as in my mail "KConfig list entries vs. kde3
> vs. desktop entry spec" applies.
> => clear go
> - remove kconfiggroup::entryMap
> - rationale: the api sucks. it wants to return all values as strings,
> which might be simply impossible without quite some additional magic
> with some backends.
The only *hard* problem that I know of right now with returning strings is
when a stringlist has an entry that has backslash as it's last char confuses
KConfigGroup into thinking the separator is escaped when it shouldn't
be. I've tried to fix this multiple times, but you thought each was
sub-optimal.
I was thinking about changing this to return QMap<QString,QVariant>, when I
changed KEntry to use QVariant instead of QByteArray, but since I'm out of
this...
- problems:
> - the replacement api is keyList() plus readEntry(). however, when
> one uses keyList() in the first place, it might mean that he has
> no idea what type to expect from the entry. anyway,
> returning/printing errors on individual readEntry() calls is
> probably better than returning a partial map.
> - the replacement is slower. hardly a problem, realistically
> speaking.
> - there are some users. probably sub-critical.
> => go, maybe only deprecate instead.
>
> the code can be found under branches/work/kconfig_new_take2/ .
> note that not all changes are implemented yet and nothing beyond kdelibs
> is guaranteed to compile.
>
> --
> Hi! I'm a .signature virus! Copy me into your ~/.signature, please!
>
> --
> Chaos, panic, and disorder - my work here is done.
>
[Attachment #3 (text/html)]
<br><br><div><span class="gmail_quote">On 10/21/07, <b \
class="gmail_sendername">Oswald Buddenhagen</b> <<a \
href="mailto:ossi@kde.org">ossi@kde.org</a>> wrote:</span><blockquote \
class="gmail_quote" style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex"> moin,<br><br>as you probably know, puthuhn (bernhard loos) \
and me are fixing KConfig.<br>while i'm pretty confident that nobody cares for \
the backend stuff we<br>are doing, ;) there are also some API changes in the works. \
<br>it's "a bit" late in the release process, so we made sure to get \
a<br>preliminary OK from allen in advance.<br>the additional effort for people not \
involved should be almost zero, as<br>we do the complete porting - and this time you \
can expect quality work - <br>after all, *i* am on it. ;)<br>however, as this grows \
into unexpected dimensions, i'd like to discuss<br>the changes in detail before \
we pursue this any further:</blockquote><div><br class="webkit-block-placeholder"> \
</div><div>since you seem to think I'm a less than capable coder, I'll leave \
it to you. After all you're the one with the <a href="http://kde.org">kde.org</a> \
address. But, I'll reply to this email then I won't touch KConfig* again. \
</div><br><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc solid;padding-left:1ex">- kill \
kconfigbase, make kconfig inherit kconfiggroup.</blockquote><div><br \
class="webkit-block-placeholder"> </div><div>It wasn't my idea, puthunh convinced \
me to readd it to the heirarchy. kconfigbase was gone from my \
branch. </div><br><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc solid;padding-left:1ex"> \
- rationale: it is The Right Thing To Do (TM). the current \
asymmetry<br> of kconfig offering the group functions but no \
entry functions is<br> just awkward. the kconfig object itself \
is the <default> group (the <br> root group, or rather, \
the non-group) and should behave as such.</blockquote><div><br \
class="webkit-block-placeholder"></div><div>actually I would say that the kconfig \
object is not the default group, but is rather the collection of *all* groups. groups \
also don't care about the backend. this seems like making a collection of items a \
subclass of the item. that's why kconfig offers the group functions, it is a \
collection of groups. a group is a collection of entries, that's why kconfiggroup \
offers the entry functions. the reason kconfiggroup also offers the group functions \
is because a group can have subgroups. </div><br><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex"> - problem: while at first it seemed like porting \
would be a trivial<br> search&replace in most cases, a \
"slight" problem turned up: kconfig <br> becomes \
implicitly shared, as kconfiggroup is. as a consequence, \
the<br> kconfiggroup constructors would take const \
KConfigGroup &master<br> instead of KConfigBase *master \
and a lot of explicit memory management <br> would need to be \
adapted.<br> so while as such this is definitely a feature, \
the porting effort is<br> just \
horrendous.<br> => without more helping hands, this is a \
no-go<br> - alternatives? maybe like this: kconfigbase dies and thus group \
<br> functions become unavailable via kconfig. no big loss: \
one can<br> already create a separate root group object - it \
would simply become<br> the only way. the porting effort \
should be *very* moderate.<br> - problems: it's uglier \
than the first variant <br> => no-go unless first option is \
unfeasible<br>- renames in kconfiggroup: name to groupName, exists to \
groupExists,<br> rename in kconfig: name to configName<br> - \
rationale: clearly necessary with the changed hierarchy. does it \
<br> make sense without it?</blockquote><div><br \
class="webkit-block-placeholder"></div><div> if you say so. if they aren't going \
to have a common ancestor, the current names are still valid, why would you ask a \
group what it's groupName is? group.name() makes more sense than \
group.groupName(). config.name () makes more sense than config.configName(). is there \
another name that they are going to return in another function that requires the \
longer names?</div><div><br> </div><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc solid;padding-left:1ex"> \
=> ?<br>- rename kconfiggroup::clean back to to \
rollback.<br> - rationale: whoever renamed it had no idea of database \
terminology</blockquote><div><br \
class="webkit-block-placeholder"></div><div>you're right I don't know \
database terminology, but I know enough to know that to rollback a db means to return \
to some previous state, which this doesn't do. That's why I thought the name \
should be changed, to reflect what it actually does and not pretend it does more. \
</div><br><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex"> => go<br>- remove kconfig::setGroup & \
kconfig::group<br> - rationale: the api sucks and is deprecated about \
forever. <br> additionally, in the current trunk it is \
outright defunct.<br> - problem: still some users left to port, but within \
reason<br> => still on go</blockquote><div><br \
class="webkit-block-placeholder"></div><div>agree </div><br><blockquote \
class="gmail_quote" style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex">- remove kconfiggroup::changeGroup<br> - \
rationale: the api sucks - it is about the same as kconfig::setGroup \
<br> - problem: quite some users<br> => prolly no-go, will \
stay with deprecation</blockquote><div><br \
class="webkit-block-placeholder"></div><div>agree</div><br><blockquote \
class="gmail_quote" style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex">
- remove kconfiggroup::group<br> - rationale: it is just a weird way to \
call the constructor:</blockquote><div><br \
class="webkit-block-placeholder"></div><div>Actually I was thinking of going the \
other way, to make read-only groups work correctly. make all the constructors \
protected except KConfigGroup() & KConfigGroup(const KConfigGroup&). \
</div><br><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex"> cg = \
KConfig(...).group("blah") => cg = KConfigGroup( KConfig(...), \
"blah" ) <br> KConfigGroup cg( \
KConfig(...).group("blah") ) => KConfigGroup cg( KConfig(...), \
"blah" )<br> - problem: quite some users<br> => \
prolly no-go, will go with deprecation<br>- remove kconfiggroup::sync \
</blockquote><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex"> - rationale: the api suggests a granularity that \
is simply not given.<br> cg.config().sync() is the proper \
call. </blockquote><div><br class="webkit-block-placeholder"></div><div>kinda \
agree</div><br><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex"> - problems:<br> - quite \
some users. manageable, though. <br> - more verbose code. do \
we want it?<br> => ?<br>- remove \
kconfiggroup::getConfigState</blockquote><div><br \
class="webkit-block-placeholder"></div><div>Again, another addition from \
puthuhn.</div><br><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc solid;padding-left:1ex"> \
- rationale: the api suggests a granularity that is simply not \
given.<br> cg.config().getConfigState() is the proper \
call.<br> => go<br>- rename kconfig::getConfigState to \
accessMode<br> - rationale: states: noAccess, readOnly, readWrite ... no \
comment </blockquote><div><br \
class="webkit-block-placeholder"></div><div>agree</div><br><blockquote \
class="gmail_quote" style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex"> => go<br>- rename \
kconfiggroup::entryIsImmutable to isEntryImmutable <br> - rationale: \
better, right?</blockquote><div><br class="webkit-block-placeholder"></div><div>works \
either way.</div><br><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc solid;padding-left:1ex"> \
=> go<br>- move kconfiggroup::setReadDefaults to \
kconfig<br> - rationale: all users (mostly kconfigskeleton derivatives) \
deal with the<br> entire file at once \
anyway</blockquote><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc solid;padding-left:1ex"> \
- problem: it's ugly and cries "i'm not \
reentrant"</blockquote><div><br \
class="webkit-block-placeholder"></div><div>another problem, how do you ensure that \
it is set on every group that comes from a config object that is supposed to be \
reading defaults only? </div><br><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex"> => no-go?<br>- remove \
kconfig::setForceGlobal<br> - rationale: only user is \
kfile/kfilewidget.cpp. the alternative api <br> is passing the \
KConfigGroup::Global flag to writeEntry calls.</blockquote><div><br \
class="webkit-block-placeholder"></div><div>actually it's also set whenever \
"kdeglobals" is opened explicitely, so that whoever is using it doesn't \
have to pass KConfigGroup::Global to every writeEntry call. </div><br><blockquote \
class="gmail_quote" style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex"> - alternatively, one might view it just like \
setReadDefaults and move<br> it to kconfiggroup instead. \
<br> => go?<br>- remove kconfiggroup::NLS<br> - rationale: \
it is an alias for Localized. the internal KEntry uses<br> the \
Localized terminology, too, so for the sake of consistency ...<br> => \
go</blockquote><div> <br class="webkit-block-placeholder"></div><div>either \
way. </div><br><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc solid;padding-left:1ex">- \
remove kconfig OnlyLocal and NoGlobals <br> - rationale: they are aliases \
for SimpleConfig resp. CascadeConfig.<br> it's more to \
learn and the negative meanings are \
potentially<br> confusing.</blockquote><div><br \
class="webkit-block-placeholder"></div> <div>That's why I changed the \
names. </div><br><blockquote class="gmail_quote" \
style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex"> => go<br>- remove the separator argument from \
{read,write}Entry for lists <br> - rationale: the separator is a backend \
internal detail. with some<br> backends, it might be plain \
unchangeable.</blockquote><div><br \
class="webkit-block-placeholder"></div><div>agree </div><br><blockquote \
class="gmail_quote" style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex"> - problems: none<br> - \
non-problems:<br> - there are some abuses of the separator \
argument and some plain<br> pointless uses. easily \
fixable.<br> - it was suggested that this would imply changing \
the kde-wide <br> separator back to the semicolon, \
as .desktop files use the<br> semicolon and \
kdesktopfile is in fact a kconfig, so we'd have \
no<br> way to get it right \
otherwise.<br> in fact, we might provide an api \
(possibly accessible by <br> kdesktopfile only) to \
put the entire file into .desktop mode -<br> that \
makes most sense anyway.<br> however, if somebody \
wants to go with semicolons kde-wide \
(hi<br> maelcum ;), the same as in my mail \
"KConfig list entries vs. kde3 <br> vs. \
desktop entry spec" applies.<br> => clear go<br>- remove \
kconfiggroup::entryMap<br> - rationale: the api sucks. it wants to return \
all values as strings,<br> which might be simply impossible \
without quite some additional magic <br> with some \
backends.</blockquote><div><br class="webkit-block-placeholder"></div><div>The only \
*hard* problem that I know of right now with returning strings is when a stringlist \
has an entry that has backslash as it's last char confuses KConfigGroup into \
thinking the separator is escaped when it shouldn't be. I've tried to \
fix this multiple times, but you thought each was sub-optimal. </div><div><br \
class="webkit-block-placeholder"></div><div>I was thinking about changing this \
to return QMap<QString,QVariant>, when I changed KEntry to use QVariant instead \
of QByteArray, but since I'm out of this... </div><br><blockquote \
class="gmail_quote" style="margin:0;margin-left:0.8ex;border-left:1px #ccc \
solid;padding-left:1ex"> - problems:<br> - the \
replacement api is keyList() plus readEntry(). however, \
when<br> one uses keyList() in the first place, it \
might mean that he has <br> no idea what type to \
expect from the entry. \
anyway,<br> returning/printing errors on \
individual readEntry() calls is<br> probably \
better than returning a partial map.<br> - the replacement is \
slower. hardly a problem, realistically \
<br> speaking.<br> - there \
are some users. probably sub-critical.<br> => go, maybe only deprecate \
instead.<br><br>the code can be found under branches/work/kconfig_new_take2/ \
.<br>note that not all changes are implemented yet and nothing beyond kdelibs <br>is \
guaranteed to compile.<br><br>--<br>Hi! I'm a .signature virus! Copy me into your \
~/.signature, please!<br><br>--<br>Chaos, panic, and disorder - my work here is \
done.<br></blockquote></div><br>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic