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

[Gilles Caulier]

2008/12/7 Aurélien Gâteau <aurelien.gateau at free.fr>

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

The widget is not planed to be moved in libkipi. It's only used in
kipi-plugins. There is no reason to sahre it with host applications or the
rest of KDE.

In fact libikipi is just a low level library with KDE4. There is no i18n
relevant.

Gilles
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-imaging/attachments/20081208/02686f6b/attachment.htm