changing the parent of a KConfigGroup

[Oswald Buddenhagen]

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.

> +++ 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 ... 

> +        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.

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

> +            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)) {
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");
> +    }
> +}


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.

-- 
Hi! I'm a .signature virus! Copy me into your ~/.signature, please!
--
Confusion, chaos, panic - my work here is done.