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

[Thomas Braxton]

On 11/22/07, Andreas Pakulat <apaku at gmx.de> wrote:
> On 22.11.07 16:36:06, Thomas Braxton wrote:
> > 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.
>
> No, name() should return only the name of this group, not any parent
> group. But anyway Osswald already has a patch posted that fixes nested
> groups the right way.
mName doesn't contain the parent name, it never has, that's why it broke.

> > 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.
>
> Which is correct as of now, because thats the separator that was chosen
> for nested groups. The reason I changed this was that one couldn't
> create groups by using groupList(), i.e.
and if you have a group that *should* have slashes this will break.
Changing the separator is the only way to fix this.

> KConfigGroup grp(config, "someparent");
> foreach(QString subgrp, grp.groupList())
> {
>   KConfigGroup somegrp(grp, subgrp);
> }
>
> Didn't work as expected, i.e. if the file had [someparent/foo] with
> entries I didn't get that group, but instead got the group
> [someparent/someparent/foo].
>
> Thats IMHO just plain wrong, I mean I'd expect the groupList() of
> KConfig to do this maybe, but certainly not the groupList() on
> KConfigGroup as that one can be nested.

then appending groupname, not groupname.left(...) is probably what you want.

> Anyway, all mood because AFAIK Oswalds patch fixes it up completely the
> right way - letting the implementation choose which separator to use for
> the nesting. And not using / for it ;)
>
> Andreas
>
> --
> You will be divorced within a year.
>