[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: changing the parent of a KConfigGroup
From: kde.braxton () gmail ! com
Date: 2008-01-03 20:04:11
Message-ID: 928c3f1b0801031204h6609d9dl9e3b104737fd0299 () mail ! gmail ! com
[Download RAW message or body]
On 1/3/08, Oswald Buddenhagen <ossi@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 :)
["reparent3.diff" (text/x-diff)]
Index: tests/kconfigtest.cpp
===================================================================
--- tests/kconfigtest.cpp (revision 755974)
+++ tests/kconfigtest.cpp (working copy)
@@ -846,6 +846,47 @@
QCOMPARE(basegrp2.readEntry("New Entry Base Only", ""), QString("SomeValue"));
}
+void KConfigTest::testCopyTo()
+{
+ KConfig cf1("kconfigtest");
+ KConfigGroup original = cf1.group("Enum Types");
+
+ KConfigGroup copy = cf1.group("Enum Types Copy");
+ original.copyTo(©); // copy from one group to another
+ QCOMPARE(copy.entryMap(), original.entryMap());
+
+ KConfig cf2("copy_of_kconfigtest", KConfig::SimpleConfig);
+ QVERIFY(!cf2.hasGroup(original.name()));
+ QVERIFY(!cf2.hasGroup(copy.name()));
+
+ KConfigGroup newGroup = cf2.group(original.name());
+ original.copyTo(&newGroup); // copy from one file to another
+ QVERIFY(cf2.hasGroup(original.name()));
+ QVERIFY(!cf2.hasGroup(copy.name())); // make sure we didn't copy more than we \
wanted + QCOMPARE(newGroup.entryMap(), original.entryMap());
+}
+
+void KConfigTest::testReparent()
+{
+ KConfig cf("kconfigtest");
+ const QString name("Enum Types");
+ KConfigGroup group = cf.group(name);
+ const QMap<QString, QString> originalMap = group.entryMap();
+ KConfigGroup parent = cf.group("Parent Group");
+
+ QVERIFY(!parent.hasGroup(name));
+
+ QVERIFY(group.entryMap() == originalMap);
+
+ group.reparent(&parent); // see if it can be made a sub-group of another group
+ QVERIFY(parent.hasGroup(name));
+ QCOMPARE(group.entryMap(), originalMap);
+
+ group.reparent(&cf); // see if it can make it a top-level group again
+// QVERIFY(!parent.hasGroup(name));
+ QCOMPARE(group.entryMap(), originalMap);
+}
+
void KConfigTest::testKAboutDataOrganizationDomain()
{
KAboutData data( "app", 0, ki18n("program"), "version",
@@ -876,3 +917,4 @@
} while(!line.isEmpty());
return lines;
}
+
Index: tests/kconfigtest.h
===================================================================
--- tests/kconfigtest.h (revision 755974)
+++ tests/kconfigtest.h (working copy)
@@ -53,6 +53,8 @@
void testGroupEscape();
void testRevertAllEntries();
void testChangeGroup();
+ void testCopyTo();
+ void testReparent();
void testKdeglobals();
void testSubGroup();
void testAddConfigSources();
Index: config/kconfiggroup.h
===================================================================
--- config/kconfiggroup.h (revision 755974)
+++ config/kconfiggroup.h (working copy)
@@ -128,6 +128,21 @@
KDE_DEPRECATED void changeGroup( const QString &group );
KDE_DEPRECATED void changeGroup( const char *group);
+ /**
+ * Copies the entries in this group to another config object.
+ * @param other The other config object to copy this group's entries to. @note \
@p other + * @em can be either another group or a different file.
+ */
+ void copyTo(KConfigBase *other) const;
+
+ /**
+ * Changes the group that this group belongs to.
+ *
+ * @param parent the config object to place this group under. If @p parent is a \
KConfig it will be + * promoted to a top-level group.
+ */
+ void reparent(KConfigBase* parent);
+
/**
* @reimp
*/
Index: config/kconfig_p.h
===================================================================
--- config/kconfig_p.h (revision 755974)
+++ config/kconfig_p.h (working copy)
@@ -57,6 +57,9 @@
void putData(const QByteArray& group, const char* key, const QByteArray& value,
KConfigBase::WriteConfigFlags flags, bool expand=false);
QStringList groupList(const QByteArray& group) const;
+ // copies the entries from @p source to @p otherGroup changing all occurrences
+ // of @p source with @p destination
+ void copyGroup(const QByteArray& source, const QByteArray& destination, \
KConfigGroup *otherGroup) const;
protected:
KSharedPtr<KConfigBackend> mBackend;
Index: config/kconfig.cpp
===================================================================
--- config/kconfig.cpp (revision 755974)
+++ config/kconfig.cpp (working copy)
@@ -113,6 +113,51 @@
return true;
}
+void KConfigPrivate::copyGroup(const QByteArray& source, const QByteArray& \
destination, + KConfigGroup *otherGroup) const
+{
+ KEntryMap& otherMap = otherGroup->config()->d_ptr->entryMap;
+ const int len = source.length();
+
+ if (destination == source) {
+ // simple optimization for when copying a group to a different
+ // parent without changing the name
+ foreach (const KEntryKey& key, entryMap.keys()) {
+ const QByteArray& group = key.mGroup;
+ if (!group.startsWith(source)) // nothing to do
+ continue;
+
+ // don't copy groups that start with the same prefix, but are not \
sub-groups + if (group.length() > len && group[len] != '\x1d')
+ continue;
+
+ qDebug() << __PRETTY_FUNCTION__ << "copying...\n\tkey = [" << group << \
',' << key.mKey << ']'; + KEntry entry = entryMap[key];
+ entry.bDirty = true;
+ otherMap[key] = entry;
+ }
+ } else {
+ foreach (const KEntryKey& key, entryMap.keys()) {
+ const QByteArray& group = key.mGroup;
+ if (!group.startsWith(source)) // nothing to do
+ continue;
+
+ // don't copy groups that start with the same prefix, but are not \
sub-groups + if (group.length() > len && group[len] != '\x1d')
+ continue;
+
+ qDebug() << __PRETTY_FUNCTION__ << "copying...\n\told key = [" << group \
<< ',' << key.mKey << ']'; + KEntryKey newKey = key;
+ newKey.mGroup.replace(0, len, destination);
+ qDebug() << "\tnew key = [" << newKey.mGroup << ',' << key.mKey << ']';
+
+ 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,30 @@
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 (KConfigGroup *otherGroup = dynamic_cast<KConfigGroup*>(other)) {
+ config()->d_func()->copyGroup(d->fullName(), otherGroup->d->fullName(), \
otherGroup); + } else if (KConfig* otherConfig = dynamic_cast<KConfig*>(other)) {
+ 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");
+ }
+}
+
+void KConfigGroup::reparent(KConfigBase* parent)
+{
+ Q_ASSERT_X(isValid(), "KConfigGroup::reparent", "accessing an invalid group");
+ Q_ASSERT(parent != 0);
+
+ KConfigGroup oldGroup(*this);
+
+ d = KConfigGroupPrivate::create(parent, d->mName, \
parent->isGroupImmutable(d->mName), false); + oldGroup.copyTo(this);
+ oldGroup.deleteGroup(); // so that the entries with the old group name are \
deleted on sync +}
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic