----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/127179/#review92777 -----------------------------------------------------------
The logic is more or less sound. You'll have to solve these issues. I think you should try to get this patch merged first, before the writers. The code looks fine enough - I don't think when you apply the patch it'll depend on the writers RR. Just edit the RR and remove the depends-on RR number. src/externalextractor.h (line 40) <https://git.reviewboard.kde.org/r/127179/#comment63200> This isn't the proper way to do private classes. Private classes are done like this: 1) In the header: ``` private: struct ExternalExtractorPrivate; ExternalExtractorPrivate *d_ptr; Q_DECLARE_PRIVATE(ExternalExtractor); ``` Basically, the private class should be named ParentClassPrivate, the next line should create a ParentClassPrivate pointer called d_ptr (the variable name is important), and then you use the Q_DECLARE_PRIVATE(ParentClass) macro call (not ParentClassPrivate) to set up the accessor. I've used the struct keyword instead of class because everything inside the public class is typically public - and in C++ structs are just classes where everything is public by default. Doing it this way is better because the macros take care of casting the pointer correctly - suppose someone subclasses ParentClass to ChildClass, raw pointers won't work because it's still a pointer to ParentClass::Private, not ChildClass::Private. 2) When you use the d-pointer inside a function, do this: ``` someFunc() { Q_D(ParentClass) // not ParentClassPrivate d->whatever = whatever; } ``` Note that you're using d-> and not d_ptr->. The Q_D() macro automatically static_casts() the d_ptr into the appropriate type and makes a variable d (local to your function) that you can use to access the private class. For more information, refer to: https://wiki.qt.io/D-Pointer src/externalextractor.cpp (line 46) <https://git.reviewboard.kde.org/r/127179/#comment63201> You'll need to create an object for the private class here too. src/externalextractor.cpp (line 61) <https://git.reviewboard.kde.org/r/127179/#comment63207> All text needs to be translatable. 1) ```#include <KLocalizedString>``` 2) Replace all ```string``` by ```i18n(string)``` src/externalextractor.cpp (line 83) <https://git.reviewboard.kde.org/r/127179/#comment63208> You're leaking memory. Have a destructor delete the private class object. src/externalextractor.cpp (line 104) <https://git.reviewboard.kde.org/r/127179/#comment63202> You may want to wait for more than 5 seconds for large files. 30 seconds is a good idea. src/externalextractor.cpp (line 108) <https://git.reviewboard.kde.org/r/127179/#comment63203> Clean this up. Vomit out the error output only if there's an error. There's no need to dump the extractor output in case there's no error. src/extractorcollection.cpp (line 4) <https://git.reviewboard.kde.org/r/127179/#comment63204> Copyright who? src/extractorcollection.cpp (line 81) <https://git.reviewboard.kde.org/r/127179/#comment63205> Cat-on-keyboard? ;-) src/extractors/externalextractors/pdfextractor/main.py (line 40) <https://git.reviewboard.kde.org/r/127179/#comment63206> Remove empty space from this line. - Boudhayan Gupta On Feb. 26, 2016, 12:02 a.m., Varun Joshi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/127179/ > ----------------------------------------------------------- > > (Updated Feb. 26, 2016, 12:02 a.m.) > > > Review request for Baloo, KDE Frameworks, Boudhayan Gupta, and Vishesh Handa. > > > Repository: kfilemetadata > > > Description > ------- > > 1. Add the ExternalExtractor class that wrap the external extractor process > into the standard Extractor interface > 2. Modify ExtractorCollection to enable it to support ExternalExtractors > 3. Added an example PyPDF2 extractor plugin > > > Diffs > ----- > > README.md 19b1a26a241e6a35c636aaf8162afe762018f073 > src/CMakeLists.txt a5490856a51aa2f59389ee963f3430c1ce5c60d5 > src/config-kfilemetadata.h.in PRE-CREATION > src/externalextractor.h PRE-CREATION > src/externalextractor.cpp PRE-CREATION > src/extractorcollection.h 8542aed576102be2b0c86bbdf3d65d756d468c6e > src/extractorcollection.cpp a1bde65bf57e493918cd3e85ccdb23c4cd623401 > src/extractorplugin.h 65abad3b2397628ba42a40d9ef2970e02114e250 > src/extractors/CMakeLists.txt 5dd223e1cf6864a943e848664ad5fae0d0603e77 > src/extractors/externalextractors/CMakeLists.txt PRE-CREATION > src/extractors/externalextractors/pdfextractor/main.py PRE-CREATION > src/extractors/externalextractors/pdfextractor/manifest.json PRE-CREATION > > Diff: https://git.reviewboard.kde.org/r/127179/diff/ > > > Testing > ------- > > > Thanks, > > Varun Joshi > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
