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

[Andreas Pakulat]

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.

> 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. 

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.

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.