Re: Making KFileMetaData a framework
On Wednesday 06 August 2014 16:09:19 Milian Wolff wrote: On Wednesday 06 August 2014 16:05:27 Vishesh Handa wrote: 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). For the record: not fixed yet (just checked). 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. Nah, imo having the manager load the plugins is just fine. Otherwise you'd leak the plugins etc. pp. Imo, the manager is OK. Right, good point. Harder to get right in a multithreaded context too. My comment there can be safely ignored. :-) I don't like the Manager name in public API though, it's generally a sign of fuzzy responsibility (or opens the door to greatly dilute the responsibility of the class). What about ExtractorCollection? 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 ... ? No, it's either Extractor or ExtractorInterface afaics. Well, probably badly worded but I proposed two options which are kind of orthogonal. Basically: a) Rename ExtractorPlugin to ExtractorInterface; b) Separate between the plugin side API and the client side API. In which case you'd have Extractor used by client code, wrapping subclasses of ExtractorPlugin (or ExtractorInterface, the name matters less in that case) inherited by plugin implementors. You question was which does ... ?, in first instance Extractor would probably relay 1:1 methods from ExtractorPlugin. The value of such a thing would be more over time. Such a class would probably have no virtual method so you can extend its interface at will, while the one carrying pure virtual is basically frozen (can't add virtuals). If in the future you'd need more API it could be added right away on Extractor, and you could provide a ExtractorPluginV2 inheriting from ExtractorPlugin. That'd mean plugin implementors would need to know which interface to target while client code wouldn't need to know, it is more future proof that way. 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. See below, making them QObjects with proper signals would make it easier to run them in a background thread. 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. That opens the door of allowing people to make mistakes. 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. This would be good since it makes it trivial to use it then in a background Thread - essentially you'd put a worker object into some thread/task which then emits a signal with the data when its ready. This could be the Manager (which
Re: Making KFileMetaData a framework
On Wednesday 06 August 2014 17:47:49 Milian Wolff wrote: Though note that the above does not mean that a call to extract would be async. It would still be sync. But you could use QFile and its readyRead signals internally to process stuff asynchronously and then emit the data once the end of the file is reached. If the signals are there (see above) this could be done without changing the API and thus could be done later. With Qt we have QFile and readReady signals so this can be done. But a lot of the extractors use third party libraries which are quite often synchronous. Right, then imo just leave it as-is but still add some signals which make it simpler to use the code as a worker object in a thread. At least, that's my opinion. Not sure what Kevin has to say in that regard. My opinion is that the threading shouldn't be forced on the client code. So I would say: signals for the API. If you reuse something third party which doesn't even allow for polling then the thread should probably be in the extractor. In other words: hide the constraints from the third party libraries you are forced to use, don't leak them to your users. 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
Re: Making KFileMetaData a framework
On Tuesday, August 05, 2014 07:58:03 PM David Edmundson wrote: In general that's some of the tidied code I've seen in a long time. Thanks! Comments below. One major, most not. TypeInfo/PropertyInfo/SimpleExtractionResult need a working operator= otherwise we shallow copy d. I can cause a crash in dump.cpp Fixed Extracting info from a file can take a long time, right now you spawn a separate execuatble for Baloo; but for other uses (i.e dolphin getting info on a non-indexed file) you might want to add a helper API; either a new thread or a service like baloo-file-extractor and a wrapper round calling it. ExtractorPluginManager::Private::allExtractors what's going on with the variable plugins? You're always checking if an empty list contains things. Looks leftover from something. Fixed In properties.h I would space out the enum so that in the future when you insert extra ones you can put it alongside the relevant category without breaking API i.e //Audio BitRate = 100, Channels, Duration, // Documents Author =200, Title, ... Otherwise it'll be an ungrouped mess in future releases if you ever have to add another audio property. Might make sense. Currently adding gaps in the properties would break my tests, but it might be a good idea. I'll look more into it. - ExtractionPluginManager possibly shouldn't be a QObject? Fixed It /might/ be a good idea to make ExtractionPluginManager static so you don't load the plugins every time (which can be a bit expensive) Hmm, Possibly. dump.cpp should check there's at least one arg. (I know it's a test, but you explicitly check for =2 but not 1) Fixed Thanks for reviewing the code. -- Vishesh Handa ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Making KFileMetaData a framework
On Tuesday 05 August 2014 19:58:03 David Edmundson wrote: In general that's some of the tidied code I've seen in a long time. Comments below. One major, most not. TypeInfo/PropertyInfo/SimpleExtractionResult need a working operator= otherwise we shallow copy d. I can cause a crash in dump.cpp Extracting info from a file can take a long time, right now you spawn a separate execuatble for Baloo; but for other uses (i.e dolphin getting info on a non-indexed file) you might want to add a helper API; either a new thread or a service like baloo-file-extractor and a wrapper round calling it. I don't think its a libraries' task to provide such an API. It cannot know whether I want to use ThreadWeaver, QThreadPool or something else. As long as it's possible to use the library from different threads (e.g. make the extractors stateless and thereby threadsafe), I don't think anything else must be done. - ExtractionPluginManager possibly shouldn't be a QObject? It /might/ be a good idea to make ExtractionPluginManager static so you don't load the plugins every time (which can be a bit expensive) I would not enforce this just because it might accidentally be misused. Singletons are an anti-pattern. Bye -- Milian Wolff m...@milianw.de http://milianw.de ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Making KFileMetaData a framework
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
Re: Making KFileMetaData a framework
On Wednesday 06 August 2014 16:05:27 Vishesh Handa wrote: 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. Nah, imo having the manager load the plugins is just fine. Otherwise you'd leak the plugins etc. pp. Imo, the manager is OK. 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 ... ? No, it's either Extractor or ExtractorInterface afaics. 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. See below, making them QObjects with proper signals would make it easier to run them in a background thread. 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. This would be good since it makes it trivial to use it then in a background Thread - essentially you'd put a worker object into some thread/task which then emits a signal with the data when its ready. This could be the Manager (which then automatically finds the correct extractor for the given mimetype), or a certain Extractor, if you know which one should be used. Though note that the above does not mean that a call to extract would be async. It would still be sync. But you could use QFile and its readyRead signals internally to process stuff asynchronously and then emit the data once the end of the file is reached. If the signals are there (see above) this could be done without changing the API and thus could be done later. What about the thread safety? At a glance I would say ExtractorPluginManager is not This should be investigated. Note that it would be fine to load the plugins in the ctor (which by definition is never thread safe). Just the usage later to match a given data set/file against the extractors and to get some result out of that should be made thread safe. Bye -- Milian Wolff m...@milianw.de http://milianw.de ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Making KFileMetaData a framework
On Wednesday 06 August 2014 17:29:40 Vishesh Handa wrote: On Wednesday, August 06, 2014 04:09:19 PM Milian Wolff wrote: 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. This would be good since it makes it trivial to use it then in a background Thread - essentially you'd put a worker object into some thread/task which then emits a signal with the data when its ready. This could be the Manager (which then automatically finds the correct extractor for the given mimetype), or a certain Extractor, if you know which one should be used. It might just be easier to make the manager return a QRunnable with all the extractors for that mimetype ready to run. And then you run them in a thread loop. No please don't. QtConcurrent just plain sucks. Though note that the above does not mean that a call to extract would be async. It would still be sync. But you could use QFile and its readyRead signals internally to process stuff asynchronously and then emit the data once the end of the file is reached. If the signals are there (see above) this could be done without changing the API and thus could be done later. With Qt we have QFile and readReady signals so this can be done. But a lot of the extractors use third party libraries which are quite often synchronous. Right, then imo just leave it as-is but still add some signals which make it simpler to use the code as a worker object in a thread. At least, that's my opinion. Not sure what Kevin has to say in that regard. Bye -- Milian Wolff m...@milianw.de http://milianw.de ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Making KFileMetaData a framework
In general that's some of the tidied code I've seen in a long time. Comments below. One major, most not. TypeInfo/PropertyInfo/SimpleExtractionResult need a working operator= otherwise we shallow copy d. I can cause a crash in dump.cpp Extracting info from a file can take a long time, right now you spawn a separate execuatble for Baloo; but for other uses (i.e dolphin getting info on a non-indexed file) you might want to add a helper API; either a new thread or a service like baloo-file-extractor and a wrapper round calling it. ExtractorPluginManager::Private::allExtractors what's going on with the variable plugins? You're always checking if an empty list contains things. Looks leftover from something. In properties.h I would space out the enum so that in the future when you insert extra ones you can put it alongside the relevant category without breaking API i.e //Audio BitRate = 100, Channels, Duration, // Documents Author =200, Title, ... Otherwise it'll be an ungrouped mess in future releases if you ever have to add another audio property. - ExtractionPluginManager possibly shouldn't be a QObject? It /might/ be a good idea to make ExtractionPluginManager static so you don't load the plugins every time (which can be a bit expensive) dump.cpp should check there's at least one arg. (I know it's a test, but you explicitly check for =2 but not 1) ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
