[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kfm-devel
Subject:    Re: PATCH adds PluginInterface to KonqPopupMenu
From:       Simon Hausmann <hausmann () kde ! org>
Date:       2001-10-29 17:01:40
[Download RAW message or body]

On Mon, Oct 29, 2001 at 09:44:16AM +0100, Holger Freyther wrote:
> > I don't like that we need to make KonqPopupMenu 'public' for that.
> > Yes, it is already public in the sense that it gets installed, but
> > now that I see it I think the API is much to open for a library
> > class IMHO.
> Isn't a libary that for. Exporting some of its functionality

Right. They key is 'some' ;-)

> > Maybe we should close it more in terms of functional methods
> > like the add* ones. However having accessor methods for some of the
> > attributes sounds like a good idea to me (but please no getFoo()
> > style :-} It's so horribly inconsistent to Qt/kdelibs, and
> > inconsistency is what third party developers hate) .
> Ok, so I should just rip of the get in fornt of some methods.

That'd be cool :)

> > Also I don't
> > think we need a KonqPopupMenuPlugin base class. Given that it
> > provides no functionality (only empty stubs) I think the same
> > functionality can be implemented without it. Just ask the factory to
> > create a QObject and pass the KonqPopupMenu as parent (like it's
> > done now) .
> I think it's cleaner with one. I have kindof a double check first I've the 
> results of KTrader and the I check if it's inheritng KonqPopupMenuPlugin. 
> >From the newbies point of view it's much better if there's something like 
> class to inherent and extend it's functuionality then just doing it with 
> QObject and mentioning there should be a function called foo() and one called 
> bar()

Hmmm. You have a point. Just asking the library's factory to create
a QObject is bad, as the library could hold different components,
but all of them inheriting from QObject. A dedicated base class
(almost a bit of an interface) sounds right here. Ok, agreed on
the existance of KonqPopupMenuPlugin ;-)

(but I guess it would be sufficient to let it just be an empty base
class?)

> > The popupmenu implementation does not appear to be interested querying
> > any information off the plugin, so I don't think we need an interface for
> > that.
> KonqPopupMenu is interested in it. I don't know if it's a good choice of 
> style but I thought of something like bool KonqPopupMenuPlugin::isInterested( 
> ); KonqPopupMenuPlugin::isInterested(QString mimetype)
> I need to do this because I wasn't able to do this with KTrader or I was too 
> limited to do this.
> But KonqPopupMenu would do if ( !plugin->isInterested() ){  delete plugin; 
> continue; }

I disagree with that. Imagine you have a huge pile of popupmenu
plugins installed and each time the user wants to bring up the menu
the konqueror starts loading all of those plugins just to figure out
that most of them aren't needed because say for example the context
menu is on a web site ;-) . I think the better approach is to put
the mime-type in the .desktop file of the plugin and perform a query
to KTrader like: query( mimeType, "'KonqPopupMenuPlugin' in
ServiceTypes'" );

> > And the plugin implementation doesn't really need any other
> > information
> Yes it should know the KFileItemList and others. 

Ah, good point. That one we can't encode in a QString ;-) . Ok,
let's dump the args idea then.

> > As for the add* methods. I think we should make them private and let
> > the plugin be a KXMLGUIClient component that the popupmenu simply
> > adds to the GUI. That leaves complete freedom to the plugin and in
> > fact it should make it simpler (no need to call any strange add*
> > methods, use the standard xmlgui 'API' (xml format) instead) .
> >
> this sounds good.
> Ok I'll try improving it. 
> OffTopic: I'm thinking of adding plugin interfaces to more PopupMenu do you 
> think it's welcome if I would mail Dirk about it?

I guess that depends on _which_ context menu ;-)


Simon

[prev in list] [next in list] [prev in thread] [next in thread]