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
