<br><br><div class="gmail_quote">2008/12/7 Aurélien Gâteau <span dir="ltr"><<a href="mailto:aurelien.gateau@free.fr">aurelien.gateau@free.fr</a>></span><br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="Ih2E3d">Luka Renko wrote:<br>
> On Sunday 07 December 2008 19:48:57 Aurélien Gâteau wrote:<br>
>> I think the idea is good, but adding another bool to the constructor is<br>
>> not a good idea, IMO. It leads to unreadable code like this:<br>
>><br>
>> list = new ImagesList(iface, parent, true, false, false, true);<br>
><br>
> I agree, I just used the way it was already done for other options (not to<br>
> invent new way for only single option).<br>
<br>
</div>That's what I thought. Your post just made me realize the API could be<br>
improved.<br>
<div class="Ih2E3d"><br>
>> That kind of API is considerd bad practice in KDE4. It's better to<br>
>> define setters and getters. This way one can write code like this:<br>
>><br>
>> list = new ImagesList(iface, parent);<br>
>> list->setListViewOnly(true);<br>
>> list->setAllowRaw(false);<br>
>> list->setAutoLoad(false);<br>
>> list->setButtonsBelow(true);<br>
><br>
> Yep, I like this "Qt-way" of doing it. I will wait for Andi Clemens (original<br>
> author of the widget) to comment and see in what direction we should move.<br>
> I think if we do this, we should change it for already existing options.<br>
<br>
</div>I agree.<br>
<div class="Ih2E3d"><br>
> I am also not sure how hard it is to reorganize the code in that way, as<br>
> currently all GUI widgets/layouts are created already in constructor,<br>
> therefore these additional methods would need to change already created layout<br>
> (btw, I am no GUI/Qt expert - just pure low-level C++ developer trying to<br>
> write a simple GUI ;-))<br>
<br>
</div>Yes this can be a bit tricky. I haven't looked at the code but I guess<br>
at least for allowRaw and autoLoad it should not be a problem as I<br>
suppose these properties are not directly gui related. For the gui<br>
related properties, there are a few ways to do it:<br>
<br>
- Add a relayout() slot which deletes the existing layout if any and<br>
does all the layouting. Call this method through a QTimer in ctor and in<br>
each setter (using a QTimer means it will be only called once, even if<br>
multiple setters are called)<br>
<br>
- Defer layouting to showEvent(). This is probably simpler, but it means<br>
all setters altering the layout need to be called before the widget is<br>
first shown. Which is usually not a problem.<br>
<div class="Ih2E3d"><br>
>> Additionally, adding new default parameters to a constructor is not<br>
>> binary compatible, so one has to write and maintain separate<br>
>> constructors. In contrast, adding new getters and setters (as long as<br>
>> they are not virtual) is binary compatible.<br>
><br>
> True, but I think this is not really the issue, as I think this widget was not<br>
> officially released yet (still in beta) and does not have that many uses (at<br>
> least not outside of KDE svn, which are trivial to fix).<br>
<br>
</div>No it's not an issue yet, but it could be in the future when the widget<br>
is in a stable release of libkipi.</blockquote><div><br>Aurélien,<br><br>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.<br>
<br>In fact libikipi is just a low level library with KDE4. There is no i18n relevant.<br><br>Gilles</div></div><br>