even more on kconfig escapes (Re: KDE/kdelibs/kdeui/icons)

[Thomas Braxton]

On 11/21/07, Andreas Pakulat <apaku at gmx.de> wrote:
> On 21.11.07 19:29:44, Oswald Buddenhagen wrote:
> > [moved to core-devel]
> > [warning: lots of reading ahead :)]
> >
> > On Wed, Nov 21, 2007 at 11:42:10AM +0100, David Faure wrote:
> > > On Wednesday 21 November 2007, Andreas Pakulat wrote:
> > > > I don't understand why creating groups with slashes got broken either. I
> > > > suspect that "somewhere" in kconfigs code KConfigGroup::name() is used
> > > > where instead the fullname is needed.
> > >
> > probably. will you have a look? or thomas?
>
> I've got no idea where to start looking and a quick grep doesn't bring
> up any place where KConfigGroup::name() is used inside kdecore/config.
> The only thing I can imagine is that for some reason the entrymap is not
> filled with the absolute path of the config group, but as I said
> KConfigGroup::name() is not used anywhere in there...

Ok, I've had a chance to look at this. There seems to be 2 places that
make an assumption about the separator that might cause problems. The
first is I believe what caused the breakage:
--- trunk/KDE/kdelibs/kdecore/config/kconfiggroup.cpp	2007/11/12 12:25:17	735683
+++ trunk/KDE/kdelibs/kdecore/config/kconfiggroup.cpp	2007/11/20 17:20:16	739168
@@ -83,13 +83,18 @@
     QByteArray fullName() const
     {
         if (!mParent) {
-            if (mName.isEmpty())
-                return "<default>";
-            return mName;
+            return name();
         }
         return mParent->fullName(mName);
     }

+    QByteArray name() const
+    {
+        if (mName.isEmpty())
+            return "<default>";
+        return mName.mid(mName.lastIndexOf("/")+1);
+    }
I think the last line of name() is wrong, mName only contains a slash
if the group was created with a name that contained a slash (i.e. the
directories of an icon theme). So this should probably be reverted and
change KConfigGroup::name() to just return mName.

The other problem I saw was:
--- trunk/KDE/kdelibs/kdecore/config/kconfig.cpp	2007/11/20 09:02:30	739029
+++ trunk/KDE/kdelibs/kdecore/config/kconfig.cpp	2007/11/20 17:20:16	739168
@@ -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("/"));
this will probably cause problems somewhere, because it is  assuming
that if there's a slash in the name it is a group separator.

> > i haven't seen complaints about the broken compatibility yet, but here
> > are some thoughts nonetheless:
> > - i don't think this is relevant for shared kde3/kde4 configs, as they
> >   don't have weird group names, afaik
>
> well, knotifyrc in kde4 has slashes, though thats probably as well
> subgroups.
>
> > concluding questions (yeah, finally ... ;):
> > - should we restore backwards compat?
>
> I think so.
>
> > - which separator encoding to chose?
> >   - i tend to favor my last-minute idea even if i spent only five
> >     minutes developing it, as opposed to five hours on the rest. ;)
>
> I can't really comment on those various ways, as I don't fully
> understand what they'd do exactly. Yes I know you gave an example, but
> still I feel this is a bit over the top of my head.
>
> >   - second option would be the "regular" lowest-layer encoding (without
> >     the = hack in the non-backward-compatible variant). i guess i'd
> >     favor / over |, but i'm undecided.
>
> Well, I actually find ^ quite a good separator, as its probably even
> more seldom used than |. Yes I know that doesn't instantly jump at you
> and scream: I'm separating hierarchies, but so what, whoever looks at
> config files better have a slight idea about their encoding anyway,
> especially nowadays with all those various escapings and special [$x]
> markers.
>
> Andreas
>
> --
> You will be run over by a beer truck.
>