Making KFileMetaData a framework

[Kevin Ottens]

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

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

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

AFAICT there's no reason for ExtractorPlugin to inherit from QObject at that 
point. Same thing for ExtractorPluginManager.

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

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.

Regards.
-- 
Kévin Ottens, http://ervin.ipsquad.net

KDAB - proud supporter of KDE, http://www.kdab.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140805/ea48d8ca/attachment.sig>