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

[Aurélien Gâteau]

Luka Renko wrote:
> 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's what I thought. Your post just made me realize the API could be 
improved.

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

> 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 ;-))

Yes this can be a bit tricky. I haven't looked at the code but I guess 
at least for allowRaw and autoLoad it should not be a problem as I 
suppose these properties are not directly gui related. For the gui 
related properties, there are a few ways to do it:

- Add a relayout() slot which deletes the existing layout if any and 
does all the layouting. Call this method through a QTimer in ctor and in 
each setter (using a QTimer means it will be only called once, even if 
multiple setters are called)

- Defer layouting to showEvent(). This is probably simpler, but it means 
all setters altering the layout need to be called before the widget is 
first shown. Which is usually not a problem.

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

No it's not an issue yet, but it could be in the future when the widget 
is in a stable release of libkipi.

Aurélien