KConfigXT Flaws
Frerich Raabe
raabe at kde.org
Sun Jan 25 17:23:31 GMT 2004
On Sun, Jan 25, 2004 at 03:47:27PM +0100, Waldo Bastian wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Sun January 25 2004 15:44, Frerich Raabe wrote:
> > 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.
>
> Good point, but rather user something like:
> QCString genericName;
> genericName.sprintf("SettingsDialog-%p", this);
Fine with me.
> > - 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".
>
> I gues so.
>
> > - 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.
>
> Don't see a patch
That's because there was none even though I claimed otherwise. I noticed
a weird problem which I did not yet find the cause of: the attached patch
implements this (the "NoDefault" enumeration value) but apparently it does
not work yet. If I pass "NoDefault" the Ok button does indeed not become
the default - but instead, always the leftmost button does (for instance,
if the Help button is in the button mask, then it appears at the very left and
thus gets marked. If the Help button misses but a Default button is in the mask,
then it appears at the very left - and gets marked). Maybe you or somebody
who is more familiar with KDialogBase internals can have a look.
> > 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.
>
> It think it would be easier if KJanusWidget would make it possible to
> insert our own widget as page. Something like
> KJanusWidget::insertPage(QWidget *widget), that does a reparent to
> KJanusWidget::FindParent() and then calls addPageWidget()
>
> That way KConfigDialog doesn't need to reparent, and doesn't need to add a
> layout and the aboutToShowPage signal is the right one.
Yes, maybe that is a better way. A proper implementation of this would probably
require to make stuff like insertPage virtual though. What you suggest
sounds like a rather invasive change to me. Whatever solution you or whoever
feels responsible for this percieve as the most appropriate one, it'd be nice
to have it since right now the slots connected to the aboutToShowPage signal
always get a little dirty. ;-/
> > - 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.
>
> In 99% of the cases you don't need the mutators because mutation happens via
> KConfigDialog which uses setProperty() calls. No need to generate code that
> isn't used.
Maybe in 99% of the simple cases, but every semicomplex application I ported
to KConfig XT had at least one custom widget for presenting the configuration
(for instance, a listview with checkable items, and each item has an ID code,
which is used as the widget for presenting a list of unsigned integers). I'd
like to hear more opinions from other people about this.
> > - 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.
>
> It is passed via the commandline because that way the build system can do its
> magic to point to the correct file.
Yes, and what is the "File=" entry in the .kcfgc file good for then? Zack's
KConfig XT tutorial on developer.kde.org says
"[..] the "File=your_application_name.kcfg" entry which specifies where the
configuration options for your application are stored."
Running "kconfig_compiler --help" mentions that one has to pass
"file.kfcg Input kcfg XML file." as well.". I don't see the
difference between these two. As I said, after looking at the
kconfig_compiler sourcecode, the "File=" entry in the .kcfgc file doesn't seem
to be used at all. Is this just Zack's tutorial being outdated?
Another weird behaviour of KConfig XT I just noticed is that using names
for <entry> elements which have already been used in
KApplication::installKDEPropertyMap() gives very confusing behaviour. For
instance, installKDEPropertyMap() uses the name "font": if I now have an
<entry> in my application's .kcfg file which uses the same name, but a
different default font, then the "Defaults" button of the KConfigDialog dialog
is always enabled, apparently detecting the mismatch between my application's
default, and the global default (in case such a thing exists). I don't know
exactly what's happening there, maybe you can shed some light, but not using
a name which installKDEPropertyMap uses (like, changing "font" into
"defaultFont") fixes this and makes the dialog yield the expected behaviour.
This time I hopefully didn't forget to attach a patch against current CVS
which implements some of these fixes. Please review.
- Frerich
-------------- next part --------------
? kconfigxt.diff
Index: kconfigdialog.cpp
===================================================================
RCS file: /home/kde/kdelibs/kdeui/kconfigdialog.cpp,v
retrieving revision 1.8
diff -u -3 -p -r1.8 kconfigdialog.cpp
--- kconfigdialog.cpp 23 Dec 2003 14:21:39 -0000 1.8
+++ kconfigdialog.cpp 25 Jan 2004 16:32:54 -0000
@@ -55,7 +55,14 @@ KConfigDialog::KConfigDialog( QWidget *p
parent, name, modal, i18n("Configure"), dialogButtons, defaultButton ),
d(new KConfigDialogPrivate(dialogType))
{
- openDialogs.insert(name, this);
+ if ( name ) {
+ openDialogs.insert(name, this);
+ } else {
+ QCString genericName;
+ genericName.sprintf("SettingsDialog-%p", this);
+ openDialogs.insert(genericName, this);
+ setName(genericName);
+ }
d->mgr = new KConfigDialogManager(this, config);
Index: kconfigdialog.h
===================================================================
RCS file: /home/kde/kdelibs/kdeui/kconfigdialog.h,v
retrieving revision 1.9
diff -u -3 -p -r1.9 kconfigdialog.h
--- kconfigdialog.h 7 Jan 2004 23:51:11 -0000 1.9
+++ kconfigdialog.h 25 Jan 2004 16:32:55 -0000
@@ -61,6 +61,9 @@ class KConfigSkeleton;
* have a loadSettings() type slot to read settings and perform any
* necessary changes.
*
+ * Please note that using the setMainWidget method inherited from KDialogBase
+ * currently yields broken behaviour at runtime; use addPage instead.
+ *
* @see KConfigSkeleton
* @since 3.2
*/
Index: kdialogbase.cpp
===================================================================
RCS file: /home/kde/kdelibs/kdeui/kdialogbase.cpp,v
retrieving revision 1.90
diff -u -3 -p -r1.90 kdialogbase.cpp
--- kdialogbase.cpp 30 Nov 2003 21:55:27 -0000 1.90
+++ kdialogbase.cpp 25 Jan 2004 16:32:56 -0000
@@ -646,10 +646,13 @@ void KDialogBase::makeButtonBox( int but
connect( pb, SIGNAL(clicked()), SLOT(slotClose()) );
}
- QPushButton *pb = actionButton( defaultButton );
- if( pb != 0 )
+ if( defaultButton != NoDefault )
{
- setButtonFocus( pb, true, false );
+ QPushButton *pb = actionButton( defaultButton );
+ if( pb != 0 )
+ {
+ setButtonFocus( pb, true, false );
+ }
}
setButtonStyle( ActionStyle0 );
Index: kdialogbase.h
===================================================================
RCS file: /home/kde/kdelibs/kdeui/kdialogbase.h,v
retrieving revision 1.103
diff -u -3 -p -r1.103 kdialogbase.h
--- kdialogbase.h 21 Jan 2004 16:19:21 -0000 1.103
+++ kdialogbase.h 25 Jan 2004 16:32:57 -0000
@@ -212,6 +212,8 @@ class KDialogBase : public KDialog
* @li @p Yes - Show Yes-button.
* @li @p Stretch - Used internally. Ignored when used in a constructor.
* @li @p Filler - Used internally. Ignored when used in a constructor.
+ * @li @p NoDefault - Used when specifying a default button; indicates that
+ * no button should be marked by default.
*/
enum ButtonCode
{
@@ -229,7 +231,8 @@ class KDialogBase : public KDialog
Yes = 0x00000100,
Details = 0x00000400,
Filler = 0x40000000,
- Stretch = 0x80000000
+ Stretch = 0x80000000,
+ NoDefault ///< @since 3.3
};
enum ActionButtonStyle
@@ -276,7 +279,8 @@ class KDialogBase : public KDialog
* @param buttonMask Specifies which buttons will be visible. If zero
* (0) no button box will be made.
* @param defaultButton Specifies which button will be marked as
- * the default.
+ * the default. Use ButtonCode::NoDefault to indicate that no button
+ * should be marked as the default button.
* @param separator If @p true, a separator line is drawn between the
* action buttons and the main widget.
* @param user1 User button1 item.
@@ -305,7 +309,8 @@ class KDialogBase : public KDialog
* @param buttonMask Specifies which buttons will be visible. If zero
* (0) no button box will be made.
* @param defaultButton Specifies which button will be marked as
- * the default.
+ * the default. Use ButtonCode::NoDefault to indicate that no button
+ * should be marked as the default button.
* @param parent Parent of the dialog.
* @param name Dialog name (for internal use only).
* @param modal Controls dialog modality. If @p false, the rest of the
@@ -343,7 +348,8 @@ class KDialogBase : public KDialog
* @param buttonMask Specifies which buttons will be visible. If zero
* (0) no button box will be made.
* @param defaultButton Specifies which button will be marked as
- * the default.
+ * the default. Use ButtonCode::NoDefault to indicate that no button
+ * should be marked as the default button.
* @param separator If @p true, a separator line is drawn between the
* action buttons and the main widget.
* @param user1 User button1 text item.
@@ -377,7 +383,8 @@ class KDialogBase : public KDialog
* @param buttonMask Specifies which buttons will be visible. If zero
* (0) no button box will be made.
* @param defaultButton Specifies which button will be marked as
- * the default.
+ * the default. Use ButtonCode::NoDefault to indicate that no button
+ * should be marked as the default button.
* @param escapeButton Specifies which button will be activated by
* when the dialog receives a @p Key_Escape keypress.
* @param parent Parent of the dialog.
@@ -1561,8 +1568,9 @@ class KDialogBase : public KDialog
* only be ran once from the constructor.
*
* @param buttonMask Specifies what buttons will be made.
- * @param defaultButton Specifies what button we be marked as the
- * default.
+ * @param defaultButton Specifies which button will be marked as
+ * the default. Use ButtonCode::NoDefault to indicate that no button
+ * should be marked as the default button.
* @param user1 User button1 item.
* @param user2 User button2 item.
* @param user2 User button3 item.
More information about the kde-core-devel
mailing list