KConfigXT Flaws

[Frerich Raabe]

Moin,

I've recently been working a lot with the new KConfig XT framework and noticed
a few flaws, some of which are rather grave:

- KConfigDialog crashes if 0 is passed for the "const char *name" parameter;
  this is because the KConfigDialog constructor attempts to insert the null
  pointer into a dictionary.
  I suggest to generate a generic name if no name is specified - something
  like this:

  if(name) {
    openDialogs.insert(name, this);
  } else {
    QCString genericName("SettingsDialog");
    genericName += QCString().setNum(openDialogs.count());
    openDialogs.insert(genericName, this);

    // So that one can use QObject::name() to get the generated name (which
    // could then used for KConfigDialog::exists(), for instance)
    setName(genericName);
  }

- setMainWidget calls do not make any sense for KConfigDialog and will
  cause broken dialogs at runtime; this is partly a flaw in KDialogBase because
  setMainWidget is not virtual, so KConfigDialog cannot reimplement it.
  I do not know how to fix this without breaking binary compatibility, but a
  note should be added to the KConfigDialog documentation, something along the
  lines of "Please note that using the setMainWidget method inherited from
  KDialogBase yields broken behaviour as runtime, use addPage instead".

- It is not possible to dictate KConfigDialog not to use any default button at
  all. This is annoying when you have e.g. a KLineEdit in the dialog: entering
  a text there and then pressing return immediately triggers the default button
  (which is often "Ok", which in turn closes the dialog). A workaround is to
  use cfgDlg.actionButton( Foo )->setDefault( false ), but that looks clumsy.
  This flaw is also inherited from KDialogBase.
  I suggest adding a new value to the KDialogBase::ButtonCode enumeration called
  "None". This value would get ignored in situations other than specifying a
  default value. I attached a patch which demonstrates this.

- The "aboutToShowPage" signal which KConfigDialog inherits from KDialogBase
  is misleading; the widgets inserted into the dialog via addPage() are never
  the widgets which will get passed to the aboutToShowPage signal. This is
  because KConfigDialog's reimplementation of addPage silently reparents the
  given widget, and then inserts a layout widget (a QVBox, for instance). This
  code demonstrates the problem:

  KConfigDialog *dlg = new KConfigDialog( .. );
  connect( dlg, SIGNAL( aboutToShowPage( QWidget * ) ),
           SLOT( slotAboutToShowPage( QWidget * ) ) );

  m_myPage = new MyPage( dlg, "MyPage" );
  dlg->addPage( m_myPage, .. );

  [..]

  void Foo::slotAboutToShowPage( QWidget *w )
  {
      // w is *never* m_myPage because the addPage() call above didn't
      // actually insert m_myPage, but a proxy layout widget which embeds
      // m_myPage. Hence, we have to use a hack like this to get to our
      // widget:
      QObject *page = w->child( "MyPage" );

      // Now 'page' *might* be the same as m_myPage. Note that we have to
      // make sure to use the same object name ("MyPage") as the one used
      // when instantiating the page!
  }

  IMHO a good (read: transparent to clients) fix would be to add a protected
  non-virtual method
  "const KJanusWidget *janusWidget() const { return mJanus; }"
  to KDialogBase. Then, one can use that method in the KConfigDialog
  constructor to disconnect the aboutToShowPage signals (right now the signal
  is directly forwarded from KJanusWidget to KDialogBase), and instead forward
  it to a private slot. That slot then maps the widget emitted by the janus
  widget back to the widget which was passed to addPage() and finally emits
  the aboutToShowPage signal again.

- kconfig_compiler assumes "Mutators=false" as the default; this does not seem
  to make much sense, I'd say the point of a configuration facility is to make
  it possible to *change* values. IMHO mutators should be enabled by default.

- kconfig_compiler expects the name of the XML file listing the configuration
  items on the commandline. In addition, the .kcfgc file containing the code
  generation options has a "File=" key which is supposed to enlist the XML file
  as well - this seems redundant and confusing to me (in fact, it seems that
  the File= entry in the  .kcfgc file is currently ignored). IMHO only the
  File= entry in the .kcfgc file should be honoured by kconfig_compiler, which
  then would take only one argument.

I'd like to know what you people (in particular the KConfig XT people) think
about this, and what solutions you have in mind. Looking forward to your
comments.

- Frerich