[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: sugestions for KCharSelect API (was: Fwd: KDE/kdelibs/kdeui)
From:       Daniel Laidig <d.laidig () gmx ! de>
Date:       2007-04-01 21:23:49
Message-ID: 200704012323.49185.d.laidig () gmx ! de
[Download RAW message or body]

On Sunday 01 April 2007 21:26, Simon Hausmann wrote:
> Wohoo, this is now indeed a cool looking widget :)

Thanks. :)

> * 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.

OK.

> * 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.

I agree, but I would include the enum (unless it's completely removed), since 
it is much easier to do this in the constructor. And who wants to change the 
visible elements at runtime?

> First of all I suggest to rename MaxGuiElements to "AllGuiElements" (or
> generally AllFoo).

/me is stupid.

> 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'll rename it.

> 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.

This is an important point to be discussed. The reason for me to include this 
was that for example KBabel includes a very minimalistic version of 
KCharSelect in a dock widget. I don't know if this option is really needed 
and I would like to hear more comments on it.

Thanks for your ideas.

Daniel
[prev in list] [next in list] [prev in thread] [next in thread]