[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-devel
Subject:    Re: Konqueror KConfig XT conversion
From:       David Faure <faure () kde ! org>
Date:       2004-12-02 21:49:23
Message-ID: 200412022249.24052.faure () kde ! org
[Download RAW message or body]

On Monday 29 November 2004 11:45, Stefan Nikolaus wrote:
> Hi,
> 
> I've partly converted the configuration related code in konqueror to XML.
Cool!

> TODO:
> - convert code that affects settings defined in kdelibs
> - add label and whatsthis contents
> 
> Could anyone review it? Feedback is welcome.

I'm afraid the conversion introduces some bugs. It's pretty hard to detect them by testing,
so it has to be done with careful coding and careful code review.
Here's one:
-    if( cfg->hasKey( "SafeParts" )
-        && cfg->readEntry( "SafeParts" ) != QString::fromLatin1( "SAFE" ))
-        allowed_parts = cfg->readListEntry( "SafeParts" );
is now (as the inverted test)
+    if( allowed_parts.count() == 1 && allowed_parts.first() == QString::fromLatin1( "SAVE" ))

Notice the typo? SAFE became SAVE.
AFAICS the rest of the rewrite of the logic is OK though.

Also in konq_main.cc:
                  KConfigGroupSaver group( app.config(), "Reusing" );
-                 if( app.config()->readNumEntry( "MaxPreloadCount", 1 ) > 0 )
+                 if( KonquerorConfig::maxPreloadCount() > 0 )
You can remove the KConfigGroupSaver above, it's not used anymore.

I hope there are no bugs in group names or default values... - it would take pretty long to check :)

Hmm, should the HTML Settings group really be defined in konqueror.kcfg?
Those are khtml entries, also used by other apps that use khtml. Can they be
moved to a khtml.kcfg, put in kdelibs/khtml, and somehow be made to apply
to KonquerorConfig/konquerorrc as well? (I'm not a KConfigXT expert)

On the whole, I'm in favour of this conversion. After posting an updated patch
I think this can go in.

-- 
David Faure, faure@kde.org, sponsored by Trolltech to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).
 
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
[prev in list] [next in list] [prev in thread] [next in thread]