From kde-core-devel  Mon Oct 29 16:24:39 2007
From: Oswald Buddenhagen <ossi () kde ! org>
Date: Mon, 29 Oct 2007 16:24:39 +0000
To: kde-core-devel
Subject: Re: KConfig borkage
Message-Id: <20071029162439.GB4160 () ugly ! local>
X-MARC-Message: https://marc.info/?l=kde-core-devel&m=119367509221683

On Sun, Oct 28, 2007 at 09:57:30PM -0500, Thomas Braxton wrote:
> This patch fixes the wierdness and
>
good

> removes entries from other locales from the entryMap,
>
good, but ...

> since I was already in the area ;)
>
cumulative patches suck. don't do them unless necessary (yes, svn makes
them necessary more often than one'd like it to, but not in this case).

> I also added a test, since the test for "stringEntry1[fr]" was wrong
> since KConfigGroup was saving the key as "stringEntry1\x5cfr\x5d", I
> think those are the right codes.
> 
yes. you should have tracked the kconfig_new_take2 branch. ;)

pasting the patch wasn't particularly clever ... the line breaks don't
exactly help applying or reading it. :}
btw, gmail's mime type detection works for me - maybe the windows
lineendings were at fault or maybe gmail uses a mime type delivered by
the browser - dunno.

>  Index: tests/kconfigtest.cpp
> ===================================================================
> --- tests/kconfigtest.cpp (revision 730047)
> +++ tests/kconfigtest.cpp (working copy)
> @@ -500,6 +527,23 @@
> +  // test for entries that are marked as deleted when there is no default
> +  KConfig config("kconfigtest", KConfig::SimpleConfig); // make sure there
> are no defaults
> +  cg = config.group("Portable Devices");
> +  cg.writeEntry("devices|manual|(null)", "whatever");
> +  cg.writeEntry("devices|manual|/mnt/ipod", "/mnt/ipod");
> +  cg.sync();
> +
> +  int count=0;
> +  foreach(const QByteArray& item, readLines())
> +      if (item.startsWith("devices|"))
> +          count++;
> +  QVERIFY(count == 2);
> +  cg.deleteEntry("devices|manual|/mnt/ipod");
> +  cg.sync();
> +  foreach(const QByteArray& item, readLines())
> +      QVERIFY(item != "devices|manual|/mnt/ipod[$d]");
>
that doesn't test that the entry is gone, though.
i guess copying the startsWith("devices|") counter and comparing with
one would make more sense.

> Index: config/kconfigini.cpp
> ===================================================================
> --- config/kconfigini.cpp (revision 730047)
> +++ config/kconfigini.cpp (working copy)
> @@ -202,10 +202,8 @@
>              if (!locale.isEmpty()) {
>                  if (locale != currentLocale) {
>                      // backward compatibility. C == en_US
> -                    if (locale.at(0) != 'C' || currentLocale != "en_US") {
> -                        aKey += '[' + locale + ']';
> -                        locale = QByteArray();
> -                    }
>
wow, this was impressively broken - locale appended after the key was
decoded.

> +    // FIXME is this needed anymore?
>
why not?
>      if ( !file.size() && ((fileMode == -1) || (fileMode == 0600)) ) {
>          // File is empty and doesn't have special permissions: delete it.
>          file.abort();
>

while trying to understand the rest of the patch i noticed that
EntryDefault isn't set anywhere. consequently all the logic building on
top of it is dysfuct. so there is little point in reviewing the patch,
i figure.

procedure:
- revert everything
- do the localized entry purge, commit
- add the failing tests (possibly come up with more?), commit
- fix cascaded parsing. revisit the problems. if they persist, post a
  new patch.

-- 
Hi! I'm a .signature virus! Copy me into your ~/.signature, please!
--
Chaos, panic, and disorder - my work here is done.