[PATCH]KConfigGroup::groupList() kind of broken

[Andreas Pakulat]

On 19.11.07 23:12:19, Oswald Buddenhagen wrote:
> On Mon, Nov 19, 2007 at 10:12:25PM +0100, Andreas Pakulat wrote:
> > it seems that KConfigGroup::groupList() doesn't work really well with
> > subgroups.
> >
> yes. i noticed that before. i'm somewhat reluctant to fix it now, though
> - it could break code silently. otoh, it's just wrong and probably not
> relied much upon.

I wonder wether nested groups are used that much at all, I mean its a
pretty "new" feature and its not advertised in the api dox.

> more on that matter:
> - kconfiggroup::name() returns the full path, too. while that can be
>   useful, it should be a separate function.

Indeed, and in fact one of the Private classes already has "fullName()".
I'm not sure wether name() should be changed now and then a new function
added to KConfigGroup in 4.1... Opinions?

> - does kconfiggroup::keyList() behave as it should (i.e., not including
>   keys from subgroups)?

Test added to kconfigtest, shows that keyList() behaves as it should.

> - and does kconfiggroup::entryMap()?

Same thing here.

> >      foreach (const KEntryKey& key, entryMap.keys())
> >          if (key.mKey.isNull() && key.mGroup.startsWith(group) && key.mGroup != group)
> > +	{
> > +            QString groupname = QString::fromUtf8(key.mGroup.mid(group.length()+1));
> 
> > +            groups << groupname.mid(0, groupname.indexOf("/"));
> >
> the function is called left(). ;)

Right and also about the naive approach. Updated patch attached (without
changing the behaviour of name()).

Andreas

-- 
Your boss is a few sandwiches short of a picnic.
-------------- next part --------------
Index: tests/kconfigtest.cpp
===================================================================
--- tests/kconfigtest.cpp	(Revision 738799)
+++ tests/kconfigtest.cpp	(Arbeitskopie)
@@ -65,6 +65,7 @@
 #define VARIANTLISTENTRY2 (QVariantList() << POINTENTRY << SIZEENTRY)
 #define HOMEPATH QDir::homePath()+"/foo"
 #define HOMEPATHESCAPE QDir::homePath()+"/foo/$HOME"
+#define SUBGROUPLIST (QStringList() << "SubGroup" << "SubGroup1" << "SubGroup2")
 
 void KConfigTest::initTestCase()
 {
@@ -708,6 +709,15 @@
     QVERIFY(subcg2.readEntry( "substring", "") == QString("somevalue") );
     KConfigGroup subcg3( &cg, "SubGroup/3");
     QVERIFY(subcg3.readEntry( "sub3string", "") == QString("somevalue") );
+
+    QVERIFY(cg.groupList() == SUBGROUPLIST );
+
+    QVERIFY(subcg3.keyList() == (QStringList() << "sub3string"));
+    QVERIFY(cg.keyList() == (QStringList() << "parentgrpstring"));
+
+    QVERIFY(cg.entryMap().keys() == (QStringList() << "parentgrpstring"));
+    QVERIFY(subcg3.entryMap().keys() == (QStringList() << "sub3string"));
+
     KConfigGroup subcg4( &subcg3, "3");
     QVERIFY(!subcg4.exists());
 }
Index: config/kconfig.cpp
===================================================================
--- config/kconfig.cpp	(Revision 738799)
+++ config/kconfig.cpp	(Arbeitskopie)
@@ -167,13 +167,16 @@
 
 QStringList KConfigPrivate::groupList(const QByteArray& group) const
 {
-    QStringList groups;
+    QSet<QString> groups;
 
     foreach (const KEntryKey& key, entryMap.keys())
         if (key.mKey.isNull() && key.mGroup.startsWith(group) && key.mGroup != group)
-            groups << QString::fromUtf8(key.mGroup);
+	{
+            QString groupname = QString::fromUtf8(key.mGroup.mid(group.length()+1));
+            groups << groupname.left(groupname.indexOf("/"));
+	}
 
-    return groups;
+    return groups.toList();
 }
 
 QStringList KConfig::keyList(const QString& aGroup) const