Review Request: GUI to edit pictureshapes

[Thorsten Zachmann]

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



plugins/pictureshape/ChangeImageCommand.cpp
<http://git.reviewboard.kde.org/r/103402/#comment7553>

    For changing the cropping or changing the color mode new commands should be added and not all pushed in one command. The it is also much easier possible to specify the text of the command that is done.



plugins/pictureshape/ClippingRect.h
<http://git.reviewboard.kde.org/r/103402/#comment7557>

    Maybe it would be good to change it to a proper class, and add methods for the members. It is not used in a place where overhead this introduces would be problematic. And it will result in better understandable code e.g. as it is clearer what is a member variable and what not.



plugins/pictureshape/ClippingRect.cpp
<http://git.reviewboard.kde.org/r/103402/#comment7554>

    there should be only 1 space between the end of the variable and the *. Please change that on all occurrences.



plugins/pictureshape/CropWidget.cpp
<http://git.reviewboard.kde.org/r/103402/#comment7556>

    Is it sure that the height is never 0?



plugins/pictureshape/CropWidget.cpp
<http://git.reviewboard.kde.org/r/103402/#comment7555>

    Please follow the coding style. There should be a blank between if and (. Please move the code to be in the next line then the if e.g.
    if (viewAspect > imgAspect) {
        return ....
    }
    else {
    ....



plugins/pictureshape/CropWidget.cpp
<http://git.reviewboard.kde.org/r/103402/#comment7559>

    I'm not sure but is the 0.0 correct as it will not return the rect in the center but at the top



plugins/pictureshape/CropWidget.cpp
<http://git.reviewboard.kde.org/r/103402/#comment7558>

    please add a blank between if and (



plugins/pictureshape/CropWidget.cpp
<http://git.reviewboard.kde.org/r/103402/#comment7560>

    How about adding a definition for TopLeft that does the or already. That can improve readability.



plugins/pictureshape/PictureShape.h
<http://git.reviewboard.kde.org/r/103402/#comment7561>

    Please add some documentation to make clear for what the proxy is used. As I understand this is used to make sure you can update the image without using a mutex.



plugins/pictureshape/PictureShape.cpp
<http://git.reviewboard.kde.org/r/103402/#comment7562>

    might it be that a cropped and a uncropped image have the same key and therefore a cropped image will be shown as it is found in the cache?



plugins/pictureshape/PictureShape.cpp
<http://git.reviewboard.kde.org/r/103402/#comment7563>

    The fo:clip should only be added when there is clipping and not always


- Thorsten Zachmann


On Dec. 20, 2011, 9:51 a.m., Silvio Heinrich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103402/
> -----------------------------------------------------------
> 
> (Updated Dec. 20, 2011, 9:51 a.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> It's a feature meant to be applied after the release is out.
> It adds a GUI to the pictureshape tool for cropping and setting the color mode.
> The scaling of the loaded picture is done in a background thread.
> 
> 
> Diffs
> -----
> 
>   plugins/pictureshape/CMakeLists.txt 5e6c922 
>   plugins/pictureshape/ChangeImageCommand.h 44244d4 
>   plugins/pictureshape/ChangeImageCommand.cpp 66decc4 
>   plugins/pictureshape/ClippingRect.h PRE-CREATION 
>   plugins/pictureshape/ClippingRect.cpp PRE-CREATION 
>   plugins/pictureshape/CropWidget.h PRE-CREATION 
>   plugins/pictureshape/CropWidget.cpp PRE-CREATION 
>   plugins/pictureshape/GreyscaleFilterEffect.h e858599 
>   plugins/pictureshape/GreyscaleFilterEffect.cpp ed8032f 
>   plugins/pictureshape/MonoFilterEffect.h 2696378 
>   plugins/pictureshape/MonoFilterEffect.cpp 115b068 
>   plugins/pictureshape/PictureShape.h fc3c221 
>   plugins/pictureshape/PictureShape.cpp 68eeb6a 
>   plugins/pictureshape/PictureShapeConfigWidget.h 7249e7b 
>   plugins/pictureshape/PictureShapeConfigWidget.cpp a85e764 
>   plugins/pictureshape/PictureTool.h 5c1dd5f 
>   plugins/pictureshape/PictureTool.cpp aef9304 
>   plugins/pictureshape/SelectionRect.h PRE-CREATION 
>   plugins/pictureshape/SelectionRect.cpp PRE-CREATION 
>   plugins/pictureshape/WatermarkFilterEffect.h f8a5b05 
>   plugins/pictureshape/WatermarkFilterEffect.cpp 513d44f 
>   plugins/pictureshape/filters/GreyscaleFilterEffect.h PRE-CREATION 
>   plugins/pictureshape/filters/GreyscaleFilterEffect.cpp PRE-CREATION 
>   plugins/pictureshape/filters/MonoFilterEffect.h PRE-CREATION 
>   plugins/pictureshape/filters/MonoFilterEffect.cpp PRE-CREATION 
>   plugins/pictureshape/filters/WatermarkFilterEffect.h PRE-CREATION 
>   plugins/pictureshape/filters/WatermarkFilterEffect.cpp PRE-CREATION 
>   plugins/pictureshape/forms/wdgPictureTool.ui PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/103402/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Silvio Heinrich
> 
>

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