Moving code for "Open With" and ServiceMenu actions to kdelibs

[David Faure]

[taking plasma-devel off the CC since I'm not subscribed there, was just for info]

On Thursday 19 March 2009, Aaron J. Seigo wrote:
> On Tuesday 17 March 2009, David Faure wrote:
> > Thanks for reviewing the patch (especially the API, which I already cleaned
> > up compared to what was in libkonq).
> 
> should KFileItemActions be a QObject to ease in memory management and/or make 
> it possible to introduce signals/slots in future if needed/desired?

That's a good point, added.

The question becomes whether the ctor should take a QObject* or a QWidget*
as a parameter (to replace the setParentWidget call). But I think I prefer to keep
the two separate; the typical usage in konq is to create the KFileItemActions
as a member of the popupmenu, but to give the mainwindow as parentwidget
(so that setStatusTip can find a statusbar where to display the help text,
and so that dialogs have a proper window as parent). OK so I think this answers
my own question, I made it   KFileItemActions(QObject* parent = 0);

> +    KAction* preferredOpenWithAction(const QString& traderConstraint);
> 
> should be const? or does this alter the object in some way that is 
> visible/relevant to the consumer? looking at the code it doesn't seem so?

Well this method is no different from the add...To methods: technically
you could all make them const since the data is in the d pointer anyway,
but semantically this is cheating; they all modify one of the QActionGroups.
Even preferredOpenWithAction calls createAppAction which adds
the action to both m_ownActions (for deletion) and to
m_runApplicationActionGroup (so that the slot is called).
I don't see any benefit/sense in making these methods const.

> would it make sense to have a version of setItemListProperties that takes a 
> const KFileItemList& so that code using this wouldn't necessarily need to use 
> KFileItemListProperties? 

Maybe... I just fear that people end up creating two ListProperties instances
for the same set of items (once internally in KFileItemActions and once in their
popup menu code in order to handle copy/move/cut/paste/etc.), which leads
to duplicate effort (the whole point of ListProperties is to cache the result
of that checking). Ok, OTOH, that's not going to bring a machine on its knees
either ;-)

Hmm, actually, the KFileItemListProperties(KFileItemList) ctor isn't explicit,
so you should be able to go ahead and do just that, passing a KFileItemList
to that method. We can discuss whether implicit ctor is a good idea though
(but in this case it just might; the ListProperties class is a list with more data)
(in fact I'm not 100% happy about the ListProperties name, technically it's not
just the properties but also the itemlist, so it's really a KFileItemListAndProperties ;-)

> for that matter, does KFileItemListProperties even need to be a public class 
> at this time

Yes. The libkonq patch (which I didn't post but can if you want) says:
konq_popupmenu.cpp:    KFileItemListProperties m_popupItemProperties;
and that member is used all over the popupmenu code, since that's where
we get the list of items from, as well as their capabilities and properties like
konq_popupmenu.cpp:    const bool isDirectory = m_popupItemProperties.isDirectory();
konq_popupmenu.cpp:    const bool sMoving = sDeleting && m_popupItemProperties.supportsMoving();
konq_popupmenu.cpp:    const QString commonMimeType = m_popupItemProperties.mimeType();
etc.

dolphin, OTOH, just passes a kfileitemlist to KFileItemActions, and indeed the
implicit ctor works, I tested it now.

> btw, is this code supposed to conform to the kdelibs coding style?

Sure.

> if so,  i suppose that clean up needs to be done ...

Can you be more specific about which coding style rules it violates?
Ah, ok, there were still spaces inside parenthesis. Fixed. Anything else?

-- 
David Faure, faure at kde.org, sponsored by Qt Software @ Nokia to work on KDE,
Konqueror (http://www.konqueror.org), and KOffice (http://www.koffice.org).