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

List:       kmail-devel
Subject:    [PATCH] janitorial: Make KConfig group switching save.
From:       Marc Mutz <Marc.Mutz () uni-bielefeld ! de>
Date:       2001-05-14 12:03:04
[Download RAW message or body]

Hi!

This patch is quite huge, but what it does is just janitor's work:

Whereever a method changes the config group of the app-global KConfig 
kApp->config(), it introduces potential bugs like the one we saw with 
the loading of the "set identity" filter action a few days ago, namely, 
that this method might be called from a method that itself uses 
kApp->config() and assumes that the config group it sets at it's 
beginning stays the same until their end.

The same problem arises e.g. with passing around QPainter's. But the 
latter has a save/restore facility built-in, so that it's commonly used 
like

void myfunc( QPainter* p)
{
  p->save();
  ...
  p->restore();
}

KConfig, otoh, does not have built-in save/restore, but there is a 
helper class, KConfigGroupSaver, defined in kconfigbase.h, which does 
that job much more nicely:

You simply create it on the stack and give it's constructor the group 
you want to set. It's destructor will re-set the group to the one it 
was before. The fine thing is that you don't need to check all possible 
exits of your method to make sure you call the restore function. The 
compiler takes care of that by automatically deleting the 
KConfigGroupSaver object on return.

So, what does this patch do?

It simply replaces
Q> config->setGroup("mygroup");
with
Q> KConfigGroupSaver saver(config, "mygroup");

That's it basically. There arises a problem if the method in question 
uses more than one group, of course. The kdecore docs suggest that one 
forms a block {...} of the code part that uses a particular config 
group and create a KConfigGroupSaver that's local to that block.

IMO, a positive side-effect of this change is that the code blocks, 
which use a common config group stand out quite well in the source.

In addition to this search-replace thingy, I've cleaned up a few places 
of config parsing. E.g. there were sometimes code snippets that uses 
readEntry + sscanf ("%d,%d",...) to read in sizes, where there is 
readSizeEntry, which does this for you.

There was one problem, though: The composer size was not stored with 
"%d,%d", but "%d %d". I changed it nonetheless, because this 
incompatibility just leads to the composer win having default size 
after upgrading. I think this is a minor point and not worth of a 
compatibility-hack.

Also, I've changed code like
KConfig &config = *kApp->config();
to
KConfig *config = kApp->config();
for the sake of equal handling over souce file boundaries (ie. 
cosmetics).

At the end, the wrong fix for the set identity action can be reverted. 
Patch for that attached, too.

Marc

-- 
Marc Mutz <Marc@Mutz.com>
http://marc.mutz.com/
http://www.mathematik.uni-bielefeld.de/~mmutz/
http://EncryptionHOWTO.sourceforge.net/

["kmail-save-config-groups.diff.bz2" (application/x-bzip2)]
["kmail-revert-my-wrong-setidentity-fix.diff;" (text/x-c++)]

Index: kmfilter.cpp
===================================================================
RCS file: /home/kde/kdenetwork/kmail/kmfilter.cpp,v
retrieving revision 1.34
diff -u -3 -p -u -r1.34 kmfilter.cpp
--- kmfilter.cpp	2001/05/09 15:33:33	1.34
+++ kmfilter.cpp	2001/05/14 11:15:16
@@ -15,14 +15,11 @@
 #include <kmessagebox.h>
 #include <kconfig.h>
 #include <kdebug.h>
-//#include <kapp.h>
 
 #include <qregexp.h>
 #include <qstring.h>
-#include <qstringlist.h>
 
 #include <assert.h>
-//#include <string.h>
 
 
 KMFilter::KMFilter( KConfig* aConfig )
@@ -109,12 +116,11 @@ bool KMFilter::folderRemoved( KMFolder* 
 void KMFilter::readConfig(KConfig* config)
 {
   // MKSearchPattern::readConfig ensures
-  // it the pattern is purified.
+  // that the pattern is purified.
   mPattern.readConfig(config);
 
   int i, numActions;
   QString actName, argsName;
-  QStringList actNames, actArgs;
 
   mActions.clear();
 
@@ -124,37 +136,29 @@ void KMFilter::readConfig(KConfig* confi
     KMessageBox::information( 0, i18n("Too many filter actions in filter rule \
`%1'").arg( mPattern.name() ) );  }
 
-  // (mmutz) Some KMFilterAction* constructors want to read
-  // from the config, too. So make sure we have read our stuff
-  // before they can switch the group...
   for ( i=0 ; i < numActions ; i++ ) {
-    actNames.append( config->readEntry( actName.sprintf("action-name-%d", i) ) );
-    actArgs.append( config->readEntry( argsName.sprintf("action-args-%d", i) ) );
-  }
-
-  QStringList::Iterator nIt = actNames.begin(), aIt = actArgs.begin();
-  for ( ; nIt != actNames.end() && aIt != actArgs.end(); ++nIt, ++aIt ) {
-    KMFilterActionDesc *desc = (*kernel->filterActionDict())[ (*nIt) ];
+    actName.sprintf("action-name-%d", i);
+    argsName.sprintf("action-args-%d", i);
+    // get the action description...
+    KMFilterActionDesc *desc = (*kernel->filterActionDict())[ config->readEntry( \
actName ) ];  if ( desc ) {
       //...create an instance...
       KMFilterAction *fa = desc->create();
       if ( fa ) {
 	//...load it with it's parameter...
-	fa->argsFromString( *aIt );
+	fa->argsFromString( config->readEntry( argsName ) );
 	//...check if it's emoty and...
-	if ( !fa->isEmpty() ) {
+	if ( !fa->isEmpty() )
 	  //...append it if it's not and...
 	  mActions.append( fa );
-	} else {
-	  //...delete it else.
+	else
+	  //...delete is else.
 	  delete fa;
-	}
       }
-    } else {
+    } else
       KMessageBox::information( 0 /* app-global modal dialog box */,
 				i18n("Unknown filter action `%1'\n in filter rule `%2'."
-				     "\nIgnoring it.").arg( *nIt ).arg( mPattern.name() ) );
-    }
+				     "\nIgnoring it.").arg( config->readEntry( actName ) ).arg( mPattern.name() \
) );  }
 }
 


_______________________________________________
Kmail Developers mailing list
Kmail@master.kde.org
http://master.kde.org/mailman/listinfo/kmail


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