SVN commit 1204914 by dfaure:

Fix deletion of groups so that a group doesn't exist() after being deleted :-)
A group now exists if it has at least one non-deleted entry (including in subgroups).

+ added unittest for deleting a nested group, and check what gets written to disk.
BUG: 192266
FIXED-IN: 4.6


 M  +54 -32    config/kconfig.cpp  
 M  +3 -0      config/kconfig_p.h  
 M  +10 -6     config/kconfigdata.h  
 M  +8 -3      config/kconfigini.cpp  
 M  +32 -2     tests/kconfigtest.cpp  


--- trunk/KDE/kdelibs/kdecore/config/kconfig.cpp #1204913:1204914
@@ -207,9 +207,10 @@
     QSet<QString> groups;
 
     for (KEntryMap::ConstIterator entryMapIt( d->entryMap.constBegin() ); entryMapIt != d->entryMap.constEnd(); ++entryMapIt) {
-        const QByteArray group = entryMapIt.key().mGroup;
-        if (entryMapIt.key().mKey.isNull() && !group.isEmpty() && group != "<default>" && group != "$Version") {
-            QString groupname = QString::fromUtf8(group);
+        const KEntryKey& key = entryMapIt.key();	
+        const QByteArray group = key.mGroup;
+        if (key.mKey.isNull() && !group.isEmpty() && group != "<default>" && group != "$Version") {
+            const QString groupname = QString::fromUtf8(group);
             groups << groupname.left(groupname.indexOf(QLatin1Char('\x1d')));
         }
     }
@@ -222,23 +223,53 @@
     QByteArray theGroup = group + '\x1d';
     QSet<QString> groups;
 
-    for (KEntryMap::ConstIterator entryMapIt( entryMap.constBegin() ); entryMapIt != entryMap.constEnd(); ++entryMapIt)
-        if (entryMapIt.key().mKey.isNull() && entryMapIt.key().mGroup.startsWith(theGroup)) {
-            const QString groupname = QString::fromUtf8(entryMapIt.key().mGroup.mid(theGroup.length()));
+    for (KEntryMap::ConstIterator entryMapIt( entryMap.constBegin() ); entryMapIt != entryMap.constEnd(); ++entryMapIt) {
+        const KEntryKey& key = entryMapIt.key();	
+        if (key.mKey.isNull() && key.mGroup.startsWith(theGroup)) {
+            const QString groupname = QString::fromUtf8(key.mGroup.mid(theGroup.length()));
             groups << groupname.left(groupname.indexOf(QLatin1Char('\x1d')));
         }
+    }
 
     return groups.toList();
 }
 
-QStringList KConfig::keyList(const QString& aGroup) const
+// List all sub groups, including subsubgroups
+QSet<QByteArray> KConfigPrivate::allSubGroups(const QByteArray& parentGroup) const
 {
-    Q_D(const KConfig);
+    QSet<QByteArray> groups;
+    QByteArray theGroup = parentGroup + '\x1d';
+    groups << parentGroup;
+
+    for (KEntryMap::const_iterator entryMapIt = entryMap.begin(); entryMapIt != entryMap.end(); ++entryMapIt) {
+        const KEntryKey& key = entryMapIt.key();
+        if (key.mKey.isNull() && key.mGroup.startsWith(theGroup)) {
+            groups << key.mGroup;
+        }
+    }
+    return groups;
+}
+
+bool KConfigPrivate::hasNonDeletedEntries(const QByteArray& group) const
+{
+    const QSet<QByteArray> allGroups = allSubGroups(group);
+
+    Q_FOREACH(const QByteArray& aGroup, allGroups) {
+        // Could be optimized, let's use the slow way for now
+        // Check for any non-deleted entry
+        if (!keyListImpl(aGroup).isEmpty())
+            return true;
+    }
+    return false;
+}
+
+
+QStringList KConfigPrivate::keyListImpl(const QByteArray& theGroup) const
+{
     QStringList keys;
-    const QByteArray theGroup(aGroup.isEmpty() ? "<default>" : aGroup.toUtf8());
 
-    const KEntryMapConstIterator theEnd = d->entryMap.constEnd();
-    KEntryMapConstIterator it = d->entryMap.findEntry(theGroup);
+    const KEntryMapConstIterator theEnd = entryMap.constEnd();
+    KEntryMapConstIterator it = entryMap.findEntry(theGroup);
     if (it != theEnd) {
         ++it; // advance past the special group entry marker
 
@@ -254,6 +285,13 @@
     return keys;
 }
 
+QStringList KConfig::keyList(const QString& aGroup) const
+{
+    Q_D(const KConfig);
+    const QByteArray theGroup(aGroup.isEmpty() ? "<default>" : aGroup.toUtf8());
+    return d->keyListImpl(theGroup);
+}
+
 QMap<QString,QString> KConfig::entryMap(const QString& aGroup) const
 {
     Q_D(const KConfig);
@@ -657,18 +695,9 @@
     Q_D(KConfig);
     KEntryMap::EntryOptions options = convertToOptions(flags)|KEntryMap::EntryDeleted;
 
-    QByteArray theGroup = aGroup + '\x1d';
-    QSet<QByteArray> groups;
-    groups << aGroup;
-
-    for (KEntryMap::ConstIterator entryMapIt( d->entryMap.constBegin() ); entryMapIt != d->entryMap.constEnd(); ++entryMapIt) {
-        if (entryMapIt.key().mKey.isNull() && entryMapIt.key().mGroup.startsWith(theGroup)) {
-            groups << entryMapIt.key().mGroup;
-        }
-    }
-
+    const QSet<QByteArray> groups = d->allSubGroups(aGroup);
     foreach (const QByteArray& group, groups) {
-        const QStringList keys = keyList(QString::fromUtf8(group));
+        const QStringList keys = d->keyListImpl(group);
         foreach (const QString& _key, keys) {
             const QByteArray &key = _key.toUtf8();
             if (d->canWriteEntry(group, key.constData())) {
@@ -709,18 +738,11 @@
 {
     Q_D(const KConfig);
 
-    if (d->entryMap.hasEntry(aGroup)) return true;
+    // No need to look for the actual group entry anymore, or for subgroups:
+    // a group exists if it contains any non-deleted entry.
 
-    QByteArray subGroupMarker = aGroup + '\x1d';
-    // Because this group could have only subgroups but no entries we have to
-    // search for group markers
-    for (KEntryMap::ConstIterator entryMapIt( d->entryMap.constBegin() ); entryMapIt != d->entryMap.constEnd(); ++entryMapIt) {
-        if (entryMapIt.key().mGroup.startsWith(subGroupMarker)) {
-            return true;
+    return d->hasNonDeletedEntries(aGroup);
         }
-    }
-    return false;
-}
 
 bool KConfigPrivate::canWriteEntry(const QByteArray& group, const char* key, bool isDefault) const
 {
--- trunk/KDE/kdelibs/kdecore/config/kconfig_p.h #1204913:1204914
@@ -61,6 +61,9 @@
     // of @p source with @p destination
     void copyGroup(const QByteArray& source, const QByteArray& destination,
                    KConfigGroup *otherGroup, KConfigBase::WriteConfigFlags flags) const;
+    QStringList keyListImpl(const QByteArray& theGroup) const;
+    QSet<QByteArray> allSubGroups(const QByteArray& parentGroup) const;
+    bool hasNonDeletedEntries(const QByteArray& group) const;
 
 protected:
     KSharedPtr<KConfigBackend> mBackend;
--- trunk/KDE/kdelibs/kdecore/config/kconfigdata.h #1204913:1204914
@@ -226,12 +226,13 @@
             if (key.isEmpty()) { // inserting a group marker
                 k.mGroup = group;
                 e.bImmutable = (options&EntryImmutable);
-                if(it == end())
-                {
+                if (options&EntryDeleted) { qWarning("Internal KConfig error: cannot mark groups as deleted"); }
+                if(it == end()) {
                     insert(k, e);
                     return true;
-                } else if(it.value() == e)
+                } else if(it.value() == e) {
                     return false;
+                }
                 
                 it.value() = e;
                 return true;
@@ -274,7 +275,7 @@
                 e.bDeleted = false; // setting a value to a previously deleted entry
             e.bExpand = (options&EntryExpansion);
 
-             //qDebug() << "to [" << group << "," << key << "] =" << e.mValue;
+            //qDebug() << "to [" << group << "," << key << "] =" << e.mValue << "newKey=" << newKey;
             if(newKey)
             {
                 insert(k, e);
@@ -365,10 +366,13 @@
             const ConstIterator it = findEntry(group, key, flags);
             if (it == constEnd())
                 return false;
-            if (key.isNull()) // looking for group marker
+            if (it->bDeleted)
+                return false;
+            if (key.isNull()) { // looking for group marker
                 return it->mValue.isNull();
-            return !it->bDeleted;
         }
+            return true;
+        }
 
         bool getEntryOption(const ConstIterator& it, EntryOption option) const
         {
--- trunk/KDE/kdelibs/kdecore/config/kconfigini.cpp #1204913:1204914
@@ -368,8 +368,9 @@
     Q_ASSERT(!filePath().isEmpty());
 
     KEntryMap writeMap;
-    bool bGlobal = options & WriteGlobal;
+    const bool bGlobal = options & WriteGlobal;
 
+    // First, reparse the file on disk, to merge our changes with the ones done by other apps
     {
         ParseOptions opts = ParseExpansions;
         if (bGlobal)
@@ -378,6 +379,7 @@
         if (info != ParseOk) // either there was an error or the file became immutable
             return false;
     }
+
     const KEntryMapIterator end = entryMap.end();
     for (KEntryMapIterator it=entryMap.begin(); it != end; ++it) {
         if (!it.key().mKey.isEmpty() && !it->bDirty) // not dirty, doesn't overwrite entry in writeMap. skips default entries, too.
@@ -392,11 +394,14 @@
             } else {
                 KEntryKey defaultKey = key;
                 defaultKey.bDefault = true;
-                if (!entryMap.contains(defaultKey))
+                if (!entryMap.contains(defaultKey)) {
                     writeMap.remove(key); // remove the deleted entry if there is no default
-                else
+                    //qDebug() << "Detected as deleted=>removed:" << key.mGroup << key.mKey << "global=" << bGlobal;
+                } else {
                     writeMap[key] = *it; // otherwise write an explicitly deleted entry
+                    //qDebug() << "Detected as deleted=>[$d]:" << key.mGroup << key.mKey << "global=" << bGlobal;
             }
+            }
             it->bDirty = false;
         }
     }
--- trunk/KDE/kdelibs/kdecore/tests/kconfigtest.cpp #1204913:1204914
@@ -133,6 +133,9 @@
   cg = KConfigGroup(&cg, "Nested Group 2.1");
   cg.writeEntry( "stringEntry3", STRINGENTRY3 );
 
+  cg = KConfigGroup(&ct, "Nested Group 3");
+  cg.writeEntry( "stringEntry3", STRINGENTRY3 );
+
   cg = KConfigGroup(&sc, "List Types" );
   cg.writeEntry( "listOfIntsEntry1", INTLISTENTRY1 );
   cg.writeEntry( "listOfByteArraysEntry1", BYTEARRAYLISTENTRY1 );
@@ -667,10 +670,25 @@
   KConfig sc( "kconfigtest" );
 
   KConfigGroup ct(&sc, "Complex Types");
+
+  // First delete a nested group
+  KConfigGroup delgr(&ct, "Nested Group 3");
+  QVERIFY(delgr.exists());
+  QVERIFY(ct.hasGroup("Nested Group 3"));
+  delgr.deleteGroup();
+  QVERIFY(!delgr.exists());
+  QVERIFY(!ct.hasGroup("Nested Group 3"));
+  QVERIFY(ct.groupList().contains("Nested Group 3"));
+
   KConfigGroup ng(&ct, "Nested Group 2");
+  QVERIFY(sc.hasGroup("Complex Types"));
+  QVERIFY(!sc.hasGroup("Does not exist"));
   sc.deleteGroup("Complex Types");
   QCOMPARE(sc.group("Complex Types").keyList().count(), 0);
-  QVERIFY(sc.group("Complex Types").exists()); // yep, we deleted it, but it still "exists"...
+  QVERIFY(!sc.hasGroup("Complex Types")); // #192266
+  QVERIFY(!sc.group("Complex Types").exists());
+  QVERIFY(!ct.hasGroup("Nested Group 1"));
+
   QCOMPARE(ct.group("Nested Group 1").keyList().count(), 0);
   QCOMPARE(ct.group("Nested Group 2").keyList().count(), 0);
   QCOMPARE(ng.group("Nested Group 2.1").keyList().count(), 0);
@@ -682,6 +700,17 @@
   QVERIFY( !sc.entryMap("Hello").isEmpty() ); //not deleted group
   QVERIFY( sc.entryMap("FooBar").isEmpty() ); //inexistant group
 
+  cg.sync();
+  // Check what happens on disk
+  const QList<QByteArray> lines = readLines();
+  //qDebug() << lines;
+  QVERIFY(!lines.contains("[Complex Types]\n"));
+  QVERIFY(!lines.contains("[Complex Types][Nested Group 1]\n"));
+  QVERIFY(!lines.contains("[Complex Types][Nested Group 2]\n"));
+  QVERIFY(!lines.contains("[Complex Types][Nested Group 2.1]\n"));
+  QVERIFY(!lines.contains("[AAA]\n"));
+  QVERIFY(lines.contains("[Hello]\n")); // a group that was not deleted
+
   // test for entries that are marked as deleted when there is no default
   KConfig cf("kconfigtest", KConfig::SimpleConfig); // make sure there are no defaults
   cg = cf.group("Portable Devices");
@@ -1394,10 +1423,11 @@
     // Current state: [ca] and [de] entries left... oops.
     //qDebug() << readLinesFrom(file);
 
-    // The group still exists but the values are all gone.
+    // Bug: The group still exists [because of the localized entries]...
     QVERIFY(cg.exists());
     QVERIFY(!cg.hasKey("foo"));
     QVERIFY(!cg.hasKey("foostring"));
+    QEXPECT_FAIL("", "Currently localized values are not deleted correctly", Continue);
     QVERIFY(!cg.hasKey("foobool"));
 
     // Now switch the locale to "de" and repeat the checks. All values