Making KFileMetaData a framework
me at vhanda.in
Wed Aug 6 14:05:27 UTC 2014
On Tuesday, August 05, 2014 11:19:16 PM Kevin Ottens wrote:
> On Tuesday 05 August 2014 18:36:24 Vishesh Handa wrote:
> > I would appreciate it if everyone could review the code once before it
> > gets
> > into frameworks.
> metainfo.yaml should have "release: false" until it's part of a KF release.
> Also it should be integration and not functional (relies on plugins).
> The "test" folder should be named "tests" (see directory structure policy).
> Public headers should use <> style include. Also it seems you're not
> following the k convention but the namespace convention for your classes,
> then the includes in public headers should be namespaced as well (e.g.
> I'm not sure why ExtractorPluginManager is exported. A function in a
> namespace would be enough, there's no point in instantiating a manager by
> hand from the client code perspective, at best looks like leaking an
> implementation detail.
We could just expose a function which loads all the Extractors, but then the
client side would need to iterate over all of them and figure out which ones
match the mimetype they are targeting. I was hoping to avoid that.
Currently, ExtractorPluginManager just provides a fetchExtractors(mimetype)
function. This cannot just be made a function as one doesn't want to load all
the plugins each time. Hmm, or maybe we just save all the plugins in a static
> Similarly the ExtractorPlugin naming is odd in the public API as it states
> it is necessarily a plugin (implementation detail). I'd rename it
> ExtractorInterface, I'd drop the suffix altogether or I'd keep
> ExtractorPlugin for plugin implementors while they'd be wrapped in
> Extractor instances on the client code side (I think that's actually my
> favorite solution).
Just to clarify -
* ExtractorPlugin - All plugins would derive from this. We could also call it
ExtractorInterface (I do like this name better).
* We have an Extractor class which is just a wrapper on top of the
ExtractorInterface which does ... ?
> AFAICT there's no reason for ExtractorPlugin to inherit from QObject at that
> point. Same thing for ExtractorPluginManager.
ExtractorPlugin - This now requires every plugin to derive from both
ExtractorPlugin and QObject. This might be less friendly.
> Creating by hand the ExtractionResult, then passing it by pointer to the
> extract method looks odd. I'd expect calling extract() with a bunch of
> parameters and getting a result back (another reason for wrapping plugins on
> the client side).
The idea was that every client should implement their own ExtractionResult. By
default it is a pure virtual class. Cause you really don't want to be storing
all the text in memory.
> The whole API is synchronous which we probably don't want. It'd be better to
> have an async API (much better to build up sync on top of async than the
Hmm, how would we do async? I thought people could just run it in another
thread / process if they want. The only thing I can think of changing is that
the plugins return the result later via a signal instead of doing all the work
in one function.
> What about the thread safety? At a glance I would say ExtractorPluginManager
> is not
> That's about it after a quick glance while tired. Keep us posted for a
> second round.
Thanks for the review!
More information about the Kde-frameworks-devel