sugestions for KCharSelect API (was: Fwd: KDE/kdelibs/kdeui)

[Simon Hausmann]

>Subject: KDE/kdelibs/kdeui
>Date: Sunday 01 April 2007
>From: Daniel Laidig <d.laidig at gmx.de>
>To: kde-commits at kde.org
>
>SVN commit 648943 by laidig:
>
>Extend KCharSelect with Unicode information (character names and details).
>This is quite a huge change, KCharSelect has a completely redesigned user 
>interface and a 
>different API.
>Note that the way the data is loaded is not final yet. It will be made sure 
>the data is 
>just loaded on access.
>GUI

Wohoo, this is now indeed a cool looking widget :)

I have a few suggestions on the API. They're meant as constructive criticism, 
I don't want to discourage anyone from kdelibs development (just in 
contrary!).

* I suggest to make currentChar() non-virtual. I don't see why it should be 
virtual, nobody calls it.

* I suggest to rename setChar to setCurrentChar, for naming consistency with 
the propery name and the getter function.

* I suggest to rename the "font" property to "currentFont" (with all its 
getters/setters/signals). This removes the current conflict with 
QWidget's "font" property and it is consistent with for example QFontComboBox 
which is also a font related widget and has a QFont based property.

* The constructor

	KCharSelect(QWidget *parent, const QChar &chr = 0x0,
	const QFont &font = QFont(),
	const GuiElements guiElements = GuiElements(65535))

is not very consistent with other constructors used in Qt and kdelibs. The 
most visible difference is that the parent widget/object is usually passed as 
very last argument. But in overall constructors in KDE4/Qt4 tend to take only 
a very minimal amount of arguments in favor of regular properties. The reason 
behind that is increased readability of the caller code. If you pass 4 
arguments to a constructor it is much harder to understand later from reading 
the code what the individual arguments mean, whereas the following is much 
more readable:

KCharSelect *charSelect = new KCharSelect(parent);
charSelect->setCurrentFont(someFont);
charSelect->setCurrentChar(aspecialChar);

So I suggest to have only one simple constructor that takes just one single 
QWidget *parent = 0 argument.

* ... which leaves us with the GuiElement enum :)

First of all I suggest to rename MaxGuiElements to "AllGuiElements" (or 
generally AllFoo). If there were a property then

charSelect->setVisibileGuiElements(KCharSelect::AllGuiElements);

would be more readable than

charSelect->setVisibleGuiElements(KCharSelect::MaxGuiElements);

I would even go a step further and use the term "Control" instead of the 
longer "GuiElement". For the user the visible widgets are mostly "controls".

I see that the old KCharSelect had individual properties for hiding the 
spinbox and the font combo. But I do wonder though: Is it really worth 
exposing these implementation details of the widgets? Does any application 
actually make use of that? Whatever is exposed there cannot be removed 
anymore, limiting the possibilities of a re-design in the future.

Our color dialog for example doesn't have the ability to show/hide individual 
controls either. It doesn't sound essential to me and dangerous from the 
point of encapsulation. I suggest to remove the option alltogether from 
KCharSelect's API.


Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20070401/53e95757/attachment.sig>