--nextPart2101355.QiGsP5ytGH
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline


>Subject: KDE/kdelibs/kdeui
>Date: Sunday 01 April 2007
>From: Daniel Laidig <d.laidig@gmx.de>
>To: kde-commits@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=
=20
>interface and a=20
>different API.
>Note that the way the data is loaded is not final yet. It will be made sur=
e=20
>the data is=20
>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 criticis=
m,=20
I don't want to discourage anyone from kdelibs development (just in=20
contrary!).

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

* I suggest to rename setChar to setCurrentChar, for naming consistency wit=
h=20
the propery name and the getter function.

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

* The constructor

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

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

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

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

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

=46irst of all I suggest to rename MaxGuiElements to "AllGuiElements" (or=20
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=20
longer "GuiElement". For the user the visible widgets are mostly "controls".

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

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


Simon

--nextPart2101355.QiGsP5ytGH
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQBGEAdZWXvMThJCpvIRAhSgAJ4lTECJZiEK+h5eqhPygIAxCvSd4gCdEUBW
wLM87CPFwe3iaqL+F+UdT9U=
=eNZ7
-----END PGP SIGNATURE-----

--nextPart2101355.QiGsP5ytGH--