Review Request 127179: Add external extractor plugin support

[Boudhayan Gupta]

-----------------------------------------------------------
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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160225/8780eeda/attachment-0001.html>