changing the parent of a KConfigGroup

[kde.braxton at gmail.com]

On 1/3/08, Oswald Buddenhagen <ossi at kde.org> wrote:
> On Wed, Jan 02, 2008 at 09:07:23AM -0600, Thomas Braxton wrote:
> > Ok, this is a new version with tests.
>
> > Index: config/kconfiggroup.h
> > ===================================================================
> > --- config/kconfiggroup.h	(revision 755974)
> > +++ config/kconfiggroup.h	(working copy)
> > @@ -128,6 +128,21 @@
> >
> > +    void copyTo(KConfigBase *other) const;
>
> > +    /**
> > +     * Changes the group that this group belongs to.
> > +     *
> > +     * @param parentGroup the group to place this group under. A value of
> 0 will cause it to be
> > +     *                  promoted to a top-level group.
> > +     */
> > +    void reparent(KConfigGroup* parentGroup);
> > +
> >
> that's bad api.
> - asymmetric with copyTo
>   - seemingly arbitrary restriction to in-file reparents
> - magic null value
> make parent a KConfigBase* and all these problems disappear.

just copying what Aaron did, but you're right that would be better
since this isn't going in until 4.1 I'll change it.

> > +++ config/kconfig.cpp	(working copy)
> > @@ -113,6 +113,42 @@
> >      return true;
> >  }
> >
> > +void KConfigPrivate::copyGroup(const QByteArray& source, const
> QByteArray& destination,
> > +                                KConfigGroup *otherGroup) const
> > +{
> > +    KEntryMap& otherMap = otherGroup->config()->d_ptr->entryMap;
> > +
> > +    const QByteArray prefix = source + '\x1d'; // prefix for sub-groups
> > +
> > +    if (destination == source) {
> > +         // simple optimization for when copying a group to a different
> > +         // parent without changing the name
> >
> i tend to think that this optimization is pretty pointless in practice,
> and as it adds complexity ...

it's for the case of copying a group to another config file but still
using the same name, it avoids replacing the name with itself that it
would do in the other loop.

> > +        foreach (const KEntryKey& key, entryMap.keys()) {
> > +            const QByteArray group = key.mGroup + '\x1d'; // this makes
> finding entries easier
> >
> as this is done in every iteration, purely reading access would be much
> faster. after the startsWith matches, check whether the group name
> continues with \x1d or ends.
>
> > +            if (group.contains(prefix)) {
> >
> just wrong. startsWith().
>
> > +                KEntry entry = entryMap[key];
> > +                entry.bDirty = true;
> > +                otherMap[key] = entry;
> > +            }
> > +        }
> > +    } else {
> > +        const int len=source.length();
> > +
> surround operators with spaces.
>
> > +        foreach (const KEntryKey& key, entryMap.keys()) {
> > +            const QByteArray group = key.mGroup + '\x1d';
> >
> inefficient. s.a.
>
> > +            if (!group.contains(source)) // nothing to do
> >
> wrong. s.a.
I had already changed all these for the same reasons you pointed out :)

> > +                continue;
> > +
> > +            KEntryKey newKey = key;
> > +            newKey.mGroup.replace(source, destination);
> > +
> no point in doing a string search when the offset and length are already
> known.

yeah, I think my first version was better here.

> > +            KEntry entry = entryMap[key];
> > +            entry.bDirty = true;
> > +            otherMap[newKey] = entry;
> > +        }
> > +    }
> > +}
> > +
> >  KConfig::KConfig( const QString& file, OpenFlags mode,
> >                    const char* resourceType)
> >    : d_ptr(new KConfigPrivate(KGlobal::mainComponent(), mode,
> resourceType))
> > Index: config/kconfiggroup.cpp
> > ===================================================================
> > --- config/kconfiggroup.cpp	(revision 755974)
> > +++ config/kconfiggroup.cpp	(working copy)
> > @@ -1270,3 +1270,35 @@
> >
> >      return config()->isGroupImmutable(d->fullName(b));
> >  }
> > +
> > +void KConfigGroup::copyTo(KConfigBase* other) const
> > +{
> > +    Q_ASSERT_X(isValid(), "KConfigGroup::copyTo", "accessing an invalid
> group");
> > +    Q_ASSERT(other != 0);
> > +
> > +    if (dynamic_cast<KConfigGroup*>(other)) {
> > +        KConfigGroup * otherGroup = dynamic_cast<KConfigGroup*>(other);
> >
> repeating the dyncast isn't particularly efficient.
> really nice c++ looks like this:
>   if (KConfigGroup *otherGroup = dynamic_cast<KConfigGroup*>(other)) {

this is better, I don't know what I was thinking :(

> if you want to live more on the readable side, make the assignments
> outside the ifs and use nested ifs instead of an else-if cascade.
>
> > +        config()->d_func()->copyGroup(d->fullName(),
> otherGroup->d->fullName(), otherGroup);
> > +    } else if (dynamic_cast<KConfig*>(other)) {
> > +        KConfig *otherConfig = dynamic_cast<KConfig*>(other);
> same.
>
> > +        KConfigGroup newGroup = otherConfig->group(d->fullName());
> > +        otherConfig->d_func()->copyGroup(d->fullName(), d->fullName(),
> &newGroup);
> > +    } else {
> > +        Q_ASSERT_X(false, "KConfigGroup::copyTo", "unknown type of
> KConfigBase");
> > +    }
> > +}
>

attached is a new version with your suggestions

>
> as this is already 4.1 material, i think it would make sense to consider
> a truly hierarchical entry map at this point. i have some ideas if you
> want to start on that.

sure, send your ideas and we'll see what we can make of them :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reparent3.diff
Type: text/x-diff
Size: 7633 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20080103/0de3396b/attachment.diff>