Review Request 127178: Added writer plugin support

[Boudhayan Gupta]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127178/#review92822
-----------------------------------------------------------



I don't see any issues with logic. Just do Private classes the proper way and check for any i18n issues I may have missed.

I have one concern. The class ```WriterPrivate``` inside ```writer_p.h``` shares a name with ```WriterPrivate``` inside the class ```Writer``` in ```writer.h```. Are they the same class or different classes? If they're the same class, it's not declared correctly inside ```writer_p.h``` - it should be declared as ```class Writer::WriterPrivate { ... };```. If they're different classes, there could be a name conflict down the line - please rename this class then.


src/writedata.h (line 46)
<https://git.reviewboard.kde.org/r/127178/#comment63246>

    Remove whitespace from empty line.



src/writedata.h (line 56)
<https://git.reviewboard.kde.org/r/127178/#comment63247>

    Do private classes the proper way



src/writer.h (line 51)
<https://git.reviewboard.kde.org/r/127178/#comment63248>

    Again, use Q_DECLARE_PRIVATE here.



src/writer_p.h (line 1)
<https://git.reviewboard.kde.org/r/127178/#comment63249>

    License header missing



src/writercollection.h (line 44)
<https://git.reviewboard.kde.org/r/127178/#comment63250>

    Again, do private classes the proper way.



src/writercollection.cpp (line 113)
<https://git.reviewboard.kde.org/r/127178/#comment63251>

    i18n, here and in other places in the file



src/writers/taglibwriter.cpp (line 22)
<https://git.reviewboard.kde.org/r/127178/#comment63255>

    You can do this as:
    
    ```
    QStringList types = {
        QStringLiteral("audio/mpeg"),
        QStringLiteral("audio/mpeg3"),
        QStringLiteral("audio/x-mpeg"),
        ...
    }
    ```
    
    Using initialiser lists generates faster code. Remember every use of ```<<``` is one execution of ```operator<<()```.


- Boudhayan Gupta


On Feb. 25, 2016, 10:45 p.m., Varun Joshi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127178/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2016, 10:45 p.m.)
> 
> 
> Review request for Baloo, KDE Frameworks, Boudhayan Gupta, and Vishesh Handa.
> 
> 
> Repository: kfilemetadata
> 
> 
> Description
> -------
> 
> 1. Added the Writer class similar to the Extractor class
> 2. Added the WriteData class that works like an ExtractionResult
> 3. Added WriterPlugin and WriterCollection that work similar to ExtractorPlugin and ExtractorCollection
> 
> 
> Diffs
> -----
> 
>   README.md 19b1a26a241e6a35c636aaf8162afe762018f073 
>   src/CMakeLists.txt a5490856a51aa2f59389ee963f3430c1ce5c60d5 
>   src/writedata.h PRE-CREATION 
>   src/writedata.cpp PRE-CREATION 
>   src/writer.h PRE-CREATION 
>   src/writer.cpp PRE-CREATION 
>   src/writer_p.h PRE-CREATION 
>   src/writercollection.h PRE-CREATION 
>   src/writercollection.cpp PRE-CREATION 
>   src/writerplugin.h PRE-CREATION 
>   src/writerplugin.cpp PRE-CREATION 
>   src/writers/CMakeLists.txt PRE-CREATION 
>   src/writers/taglibwriter.h PRE-CREATION 
>   src/writers/taglibwriter.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/127178/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Varun Joshi
> 
>

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