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

[Aurélien Gâteau]

Luka Renko wrote:
> Andi, Caulier and other developers,
> 
> I am currently writing SmugMug exporter KIPI plugin for KDE4 and have seen 
> that we have nice ImagesList widget that can be shared between plugins.
> I am using it in SmugMug widget, but since I would like to fit all options 
> required for upload on one page/dialog (no tabs), I would prefer if ImagesList 
> would have an option to put control buttons (Add/Remove) below the list 
> (currently on the right side). That way we get some real estate that can be 
> used to put other plugin options on the right side.
> I have prepared a patch that adds new option (buttonsBelow) to ImagesList 
> constructor. It just moves buttons to different location and removes 
> addRowStretch() call that caused strange resizing (not sure if this is OK, but 
> I did not see any side effect to other plugins using). Please review and let 
> me know if I am fine to commit this change to SVN.

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

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


This is more code to write, but the logic behind this is that code is 
more often read than written. So one should optimize for the more 
frequent case.

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.

Aurélien

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.