KDE/kdelibs/kparts

[Stephan Kulow]

Am Mittwoch, 21. Februar 2007 22:34 schrieb David Faure:
> On Wednesday 21 February 2007, Stephan Kulow wrote:
> > SVN commit 636048 by coolo:
> >
> > whatever happened
> >
> >
> >  M  +2 -2      mainwindow.cpp
> >
> >
> > --- trunk/KDE/kdelibs/kparts/mainwindow.cpp #636047:636048
> > @@ -176,8 +176,8 @@
> >  void KParts::MainWindow::saveNewToolbarConfig()
> >  {
> >      createGUI( d->m_activePart );
> > -    KConfigGroup *
> > -    applyMainWindowSettings = new KConfigGroup(KGlobal::config(),
> > QByteArray()); +    KConfigGroup cg(KGlobal::config(), QByteArray());
> > +    applyMainWindowSettings(cg);
>
> Hmm, again an empty group... which wasn't empty in kde3.
> The code applyMainWindowSettings said:
>     KConfigGroupSaver saver( config, configGroup.isEmpty() ?
> config->group() : configGroup ); and configGroup was a parameter with an
> empty default value.
> So it was saving into "the current group". This was quite broken, on
> retrospect. The goal is to always store the settings for a given type of
> mainwindow (e.g. kmail composer window) into the same group (e.g.
> "composer"). Whether we're doing that explicitely, via autosave, or after
> "OK" in the toolbar config dialog like above.
> So I think we should store the group into a member var, a bit like
> d->autoSaveGroup - but even if not enabling autosave (=> I would rename it
> to settingsGroup - like the method that we found was redundant ;)
>
> But that means changing apply/saveMainWindowSettings to take a KConfig*
> again :(

I'm not sure we should go there, but let's analyze KDE3 use cases:

This code is from konsole's "save scheme" (path is the new scheme file):
1428     KSimpleConfig cfg( path );
1429     savePropertiesInternal(&cfg,1);
1430     saveMainWindowSettings(&cfg);

savePropertiesInternal sets the group of cfg to "1" and saveMainWindowSettings 
stores its settings in that group. Would you expect this? I don't (fortunately 
konsole seems to be the only app calling savePropertiesInternal).

Interestingly enough I can only find one applyMainWindowSettings call in 
konsole:
294   KConfig * config = KGlobal::config();
295   config->setDesktopGroup();
296   applyMainWindowSettings(config);

(huh? desktop group for main window?)

ksysguard.cc:
328 void TopLevel::slotNewToolbarConfig()
329 {
330   createGUI();
331   applyMainWindowSettings( kapp->config() );
332 }

With other words: it reads from _whatever_ group happens to be set.

keduca:
098 void Keduca::configRead()
099 {
100         KConfig *config = KGlobal::config();
101         config->setGroup( "keduca" );
102         applyMainWindowSettings( config, "keduca" );
103         _recentFiles->loadEntries(config);
104 }
105 

It sets the group even twice - but it at least sets it to something on its 
own.

kate (one of many examples how it's was meant to be):
393 void KateMainWindow::slotNewToolbarConfig()
394 {
395   applyMainWindowSettings( KateApp::self()->config(), "MainWindow" );
396 }

kfilereplace (possibly the most correct :)
086 void KFileReplace::applyNewToolbarConfig()
087 {
088     applyMainWindowSettings(KGlobal::config(), autoSaveGroup());
089 }

So: whenever applications pass KConfig* without an explicit group, they're 
most likely buggy.

So looking at it: using the object name for the group seems to be good enough.
I don't think requiring an extra setSettingsGroup() when the object name 
property is already there for use is (a kWarning() for unset object names 
will do :)

And using KGlobal::config() for default calls seems to be justified.

Greetings, Stephan