AW: KPluginFactory/-Loader as KLibFactory replacement and KDEenhanced QPluginLoader

[Nhuh Put]

> Von: Matthias Kretz
> Gesendet: Freitag, 24. August 2007 12:22
> An: kde-core-devel at kde.org
> Betreff: Re: KPluginFactory/-Loader as KLibFactory replacement and 
> KDEenhanced QPluginLoader
> 
> First: I'm very much in favour of getting this ready for next monday. 
> At least the KPluginFactory part has to be done before 4.0. The 
> KPluginLoader could be an addition after 4.0 but I'd rather put it in 
> know and see how much of the KLibLoader API we can remove once we have 
> KPluginLoader. And we could also remove kgenericfactory.h, 
> kgenericfactory.tcc and ktypelist.h if we find a good way to 
> automatically port away from KGenericFactory (though I don't see us 
> having the man power to do this).

I could also change KLibrary into a wrapper for QLibrary. This would need
only a few lines of code and would make KLibLoader obsolete.

> 
> > Index: util/klibloader.h
> > @@ -424,7 +463,7 @@
> >                  *error = ErrNoFactory;
> >              return 0;
> >          }
> > -        QObject *object = factory->create( parent,
> T::staticMetaObject.className(), args );
> > +        QObject *object = factory->create<T>(parent, QVariantList() 
> > + <<
> args);
> 
> careful with the QStringList -> QVariantList conversion. In 
> KPluginFactory you have a different method for conversion. OTOH you 
> could probably just call
> factory->create<T>(parent, args) here, it'll just result in two
> deprecation
> warnings.

I didn't check your changes to KLibLoader. Should have done it before
sending the mail. :/ I changed it as you suggested.

> 
> > Index: util/klibloader_p.h
> 
> If KLibFactory is removed this file is not necessary.
> 

I removed it.

> > --- util/kpluginfactory.cpp     (revision 0)
> > +++ util/kpluginfactory.cpp     (revision 0)
> [...]
> > +KPluginFactory::KPluginFactory(const char *componentName, const 
> > +char
> > *catalogName, QObject *parent)
> > +    : QObject(parent), d_ptr(new KPluginFactoryPrivate) {
> > +    Q_D(KPluginFactory);
> > +    d->componentData = *createComponentData();
> 
> This one is for compatiblity? For one it's a mem leak. Do it like this:
>   KComponentData *kcd = createComponentData();
>   if (kcd) {
>       d->componentData = *kcd;
>       delete kcd;
>   }
> OTOH I don't like to have two createComponentData methods. Can you 
> tell me why we need the createComponentData(KAboutData *) function?
> 

I removed both functions. If a derived class want's to manipulate the
componentdata, it can do so in its constructor.

> [...]
> > +KPluginFactory::KPluginFactory(QObject *parent)
> Where is this ctor needed? Not enough to have the "KPluginFactory(const
> char
> *componentName = 0, const char *catalogName = 0, QObject *parent = 0)"
> ctor?
> 

This is for backward compatibility. There a some derived LibFactories. It's
deprecated.

> [...]
> > +KComponentData *KPluginFactory::createComponentData()
> > +{
> > +    return new KComponentData();
> > +}
> 
> This one should return 0 now.

removed

> 
> > Index: util/kpluginfactory.h
> [...]
> > +    template<class impl>
> > +    static QObject *createInstance(QObject *parent, const QVariantList
> > &args)
> > +    {
> > +        return new impl(parent, args);
> > +    }
> 
> Perhaps we'll need another one to ease porting:
>   template<class impl>
>   static QObject *createOldInstance(QObject *parent, const QVariantList
> &args)
>   {
>       return new impl(parent, variantListToStringList(args));
>   }
> 
> This could ship with the following macro:
> 
>   #define K_REGISTER_OLD_PLUGIN(impl, iface) \
> registerPlugin(iface::staticMetaObject.className(),
> &createOldInstance<impl>,
> QString());
> 
> Then the current code looking like this:
> typedef KGenericFactory<MyPluginImpl> MyFactory;
> K_EXPORT_COMPONENT_FACTORY(libname, MyFactory("componentName"))
> 
> could automatically be ported to:
> K_PLUGIN_FACTORY(MyFactory, K_REGISTER_OLD_PLUGIN(MyPluginImpl, QObject))
> K_EXPORT_PLUGIN(MyFactory)
> the interface part is a bit of a problem though, often what we want is
> MyPluginImpl::staticMetaObject().superClass()->className() instead of
> QObject

I could do this, but I'm not sure, if it's worth the effort. It would still
depend on deprecated stuff.

> > +    virtual void setupTranslations();
> 
> What are the use cases for having this virtual? I know KGenericFactory had
> it
> virtual, but was it ever reimplemented?

It's used for KOffice filters. They use the "kofficefilters" catalog, not
the component name. There are probably some other solutions.

> 
> > +    void initializeMessageCatalog();
> 
> Do we need that in the public class. I'd like to move this function to the
> private if possible.

I made it private.

> 
> > +    virtual KDE_DEPRECATED QObject *createObject(QObject *parent, const
> > char *className, const QStringList &args);
> 
> AFAIK marking this function deprecated only gives a warning when compiling
> KPluginFactory, not when reimplementing the function.
> 

Well, it tells the human reader, that it's deprecated (if he locks at the
base class). I don't know a better solution.

> [...]
> > Index: util/kpluginloader.h
> [...]
> > +#include <QtCore/qplugin.h>
> That's <QtCore/QtPlugin>
> 

changed

> [...]
> > +#define K_EXPORT_PLUGIN2(library, factory) \
> > +Q_EXPORT_PLUGIN2(library, factory) \
> > +K_PLUGIN_VERIFICATION_DATA
> > +
> > +#define K_EXPORT_PLUGIN(factory) K_EXPORT_PLUGIN2(factory, factory)
> 
> Do we need this?
> First question: Will we ever support statically compiled in plugins? If
> not
> then
> #define K_EXPORT_PLUGIN(factory) \
> Q_EXPORT_PLUGIN(factory) \
> K_PLUGIN_VERIFICATION_DATA

removed

> 
> is enough. Otherwise we should define
> #define K_EXPORT_PLUGIN(library, factory) \
> Q_EXPORT_PLUGIN2(library, factory) \
> K_PLUGIN_VERIFICATION_DATA
> 
> For max convenience (and too many macros probably) we could also provide
> K_EXPORT_PLUGIN_FACTORY(name, pluginRegistrations) \
> K_PLUGIN_FACTORY(name, pluginRegistrations) \
> K_EXPORT_PLUGIN(name)
> 
> > +class KDECORE_EXPORT KPluginLoader : public QPluginLoader
> > +{
> > +    Q_OBJECT
> > +    Q_PROPERTY(QString fileName READ fileName)
> > +    Q_PROPERTY(QString pluginName READ fileName)
> > +public:
> > +    explicit KPluginLoader(const QString &plugin, const KComponentData
> > &componentdata = KGlobal::mainComponent(), QObject *parent = 0); +
> > explicit KPluginLoader(const KService &service, const KComponentData
> > &componentdata = KGlobal::mainComponent(), QObject *parent = 0); +
> > ~KPluginLoader();
> > +
> > +    KPluginFactory *factory();
> > +
> > +    QString pluginName() const;
> > +
> > +    bool isLoaded() const;
> > +
> > +    bool load();
> 
> What is this method needed for if it's already called automatically from
> the
> ctor?

I made it protected.

	PutHuhn
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kpluginfactoryloader.zip
Type: application/octet-stream
Size: 7290 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20070825/6e130493/attachment.obj>