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 _______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel