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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
