Review Request: New shape for 1-D and 2-D barcodes

[Thorsten Zachmann]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104235/#review11304
-----------------------------------------------------------


Looks very good. I have added some style issues that should be fixed. After that I think it is good for inclusion.

The filename of the classes should use camelcase. So the classes should be renamed to match the case of the class e.g. barcodeshape.cpp -> BarcodeShape.cpp .


plugins/barcodeshape/barcodeshape.cpp
<http://git.reviewboard.kde.org/r/104235/#comment9057>

    Please move the type to be in the same line as the function.



plugins/barcodeshape/barcodeshape.cpp
<http://git.reviewboard.kde.org/r/104235/#comment9056>

    The indention is wrong here.



plugins/barcodeshape/barcodeshape.cpp
<http://git.reviewboard.kde.org/r/104235/#comment9059>

    There should be a blank after ,



plugins/barcodeshape/barcodeshape.cpp
<http://git.reviewboard.kde.org/r/104235/#comment9055>

    I think this should be written better inside a switch statement. Is the static_cast really needed?



plugins/barcodeshape/barcodeshape.h
<http://git.reviewboard.kde.org/r/104235/#comment9062>

    Is there a reason to make this a QObject? I did not see it it. If not please remove.



plugins/barcodeshape/barcodeshape.h
<http://git.reviewboard.kde.org/r/104235/#comment9054>

    One public, protected is enough. Please remove the additional ones.



plugins/barcodeshape/barcodeshape.h
<http://git.reviewboard.kde.org/r/104235/#comment9058>

    In calligra we use m_ prefix for member variables.



plugins/barcodeshape/barcodeshapeconfigcommand.h
<http://git.reviewboard.kde.org/r/104235/#comment9060>

    Indention wrong, please remove one blank.



plugins/barcodeshape/barcodeshapeconfigcommand.h
<http://git.reviewboard.kde.org/r/104235/#comment9061>

    Members should use m_ prefix



plugins/barcodeshape/barcodeshapefactory.cpp
<http://git.reviewboard.kde.org/r/104235/#comment9063>

    This text might be to long for showing in the Add Shape docker. How about changing the order to Barcode 1/2 D then at least barcode should be fully visible in the docker.


- Thorsten Zachmann


On March 12, 2012, 1:57 a.m., Friedrich W. H. Kossebau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104235/
> -----------------------------------------------------------
> 
> (Updated March 12, 2012, 1:57 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> Adds a shape for barcodes, based on the lib prison.
> Adding, editing, saving and loading seems to work.
> For non-Calligra consumers of the OpenDocument format a fallback image is stored.
> 
> 
> Diffs
> -----
> 
>   plugins/CMakeLists.txt 69e5d96 
>   plugins/barcodeshape/CMakeLists.txt PRE-CREATION 
>   plugins/barcodeshape/Messages.sh PRE-CREATION 
>   plugins/barcodeshape/barcodeshape.cpp PRE-CREATION 
>   plugins/barcodeshape/barcodeshape.desktop PRE-CREATION 
>   plugins/barcodeshape/barcodeshape.h PRE-CREATION 
>   plugins/barcodeshape/barcodeshapeconfigcommand.h PRE-CREATION 
>   plugins/barcodeshape/barcodeshapeconfigcommand.cpp PRE-CREATION 
>   plugins/barcodeshape/barcodeshapeconfigwidget.h PRE-CREATION 
>   plugins/barcodeshape/barcodeshapeconfigwidget.cpp PRE-CREATION 
>   plugins/barcodeshape/barcodeshapefactory.h PRE-CREATION 
>   plugins/barcodeshape/barcodeshapefactory.cpp PRE-CREATION 
>   plugins/barcodeshape/barcodeshapeplugin.h PRE-CREATION 
>   plugins/barcodeshape/barcodeshapeplugin.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/104235/diff/
> 
> 
> Testing
> -------
> 
> Created, edited and deleted barcode shapes. Saved files with barcode shapes and loaded them again.
> 
> 
> Thanks,
> 
> Friedrich W. H. Kossebau
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20120312/6a4dc5b4/attachment.htm>