[Kde-imaging] Proposed enhancement to KIPIPlugins::ImagesList

[Luka Renko]

On Sunday 07 December 2008 19:48:57 Aurélien Gâteau wrote:
> I think the idea is good, but adding another bool to the constructor is
> not a good idea, IMO. It leads to unreadable code like this:
>
> list = new ImagesList(iface, parent, true, false, false, true);

I agree, I just used the way it was already done for other options (not to 
invent new way for only single option).
>
> That kind of API is considerd bad practice in KDE4. It's better to
> define setters and getters. This way one can write code like this:
>
> list = new ImagesList(iface, parent);
> list->setListViewOnly(true);
> list->setAllowRaw(false);
> list->setAutoLoad(false);
> list->setButtonsBelow(true);

Yep, I like this "Qt-way" of doing it. I will wait for Andi Clemens (original 
author of the widget) to comment and see in what direction we should move.
I think if we do this, we should change it for already existing options.

I am also not sure how hard it is to reorganize the code in that way, as 
currently all GUI widgets/layouts are created already in constructor, 
therefore these additional methods would need to change already created layout 
(btw, I am no GUI/Qt expert - just pure low-level C++ developer trying to 
write a simple GUI ;-))

> Additionally, adding new default parameters to a constructor is not
> binary compatible, so one has to write and maintain separate
> constructors. In contrast, adding new getters and setters (as long as
> they are not virtual) is binary compatible.

True, but I think this is not really the issue, as I think this widget was not 
officially released yet (still in beta) and does not have that many uses (at 
least not outside of KDE svn, which are trivial to fix).

> PS: Another suggestion: instead of a boolean "buttonsBelow" property,
> define an enum:
>
> class ImagesList {
> public:
>    enum ButtonsPosition { ButtonsBelow, ButtonsOnRight };
>    void setButtonsPosition(ButtonsPosition);
> }
>
> The advantage of this solution is that it is possible to add a new
> position if need be.

Another good point! 

Thank you very much for your feedback.

Regards,
Luka