Making KFileMetaData a framework

[Vishesh Handa]

On Tuesday, August 05, 2014 11:19:16 PM Kevin Ottens wrote:
> Hello,
> 
> 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).

Fixed

> 
> The "test" folder should be named "tests" (see directory structure policy).
>

Fixed
 
> 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.
> <kfilemetadata/foo.h>).
> 
> 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 
variable.

> 
> 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.

Fixed 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
> contrary).

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!

> 
> Regards.

-- 
Vishesh Handa