Review Request: Support for svg like clipping for shapes

[Thorsten Zachmann]

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


The following things I noted when testing the patch.

When I first clip a shape and then resize it somehow does not work correctly. Looks like the clipped area is not correct and therefore I can't see anything anymore. If I then undo the clipping and create a new command then the resizing works as expected.

If I clip an object then resize it and then unclip the object I would expect the clipping object to not restore the size it had before it was used for clipping.

When you clip an object with a rotated object then move it and unclip the rotated object jumps its position.

I also noticed that the code style is not always followed. I marked some of the issues I have seen. It would be nice if you can look over the stuff that goes to libs once more to fix these issues.


filters/karbon/svg/SvgParser.h
<http://git.reviewboard.kde.org/r/100809/#comment1536>

    There is additional space not needed.



filters/karbon/svg/SvgParser.cpp
<http://git.reviewboard.kde.org/r/100809/#comment1537>

    Looks like the comment is wrong.



filters/karbon/svg/SvgParser.cpp
<http://git.reviewboard.kde.org/r/100809/#comment1538>

    I think 0 should be enough. The L is not needed and might not be a good option on different platforms



filters/karbon/svg/SvgParser.cpp
<http://git.reviewboard.kde.org/r/100809/#comment1545>

    The comment is wrong.



filters/karbon/svg/SvgParser.cpp
<http://git.reviewboard.kde.org/r/100809/#comment1546>

    Comment is wrong



filters/karbon/svg/SvgParser.cpp
<http://git.reviewboard.kde.org/r/100809/#comment1547>

    How about initializing n with href then the else part can be removed.



filters/karbon/svg/SvgParser.cpp
<http://git.reviewboard.kde.org/r/100809/#comment1548>

    There should be no space after the *



filters/karbon/svg/svgexport.cc
<http://git.reviewboard.kde.org/r/100809/#comment1549>

    These includes shoud use <> instead of "



karbon/ui/KarbonView.cpp
<http://git.reviewboard.kde.org/r/100809/#comment1550>

    One question about that one. Is it possible as a used to make sure the right shape gets a clip path?



karbon/ui/KarbonView.cpp
<http://git.reviewboard.kde.org/r/100809/#comment1578>

    should the command not be named Clip Object as always one objects is clipped



libs/flake/KoClipPath.h
<http://git.reviewboard.kde.org/r/100809/#comment1551>

    There should be no space between *,& and the variable



libs/flake/KoClipPath.cpp
<http://git.reviewboard.kde.org/r/100809/#comment1552>

    There should be a space after the if



libs/flake/KoClipPath.cpp
<http://git.reviewboard.kde.org/r/100809/#comment1553>

    There should be a blank after the if.
    
    Can it happen that clipShapes contains a 0?



libs/flake/KoClipPath.cpp
<http://git.reviewboard.kde.org/r/100809/#comment1554>

    There should be a blank after the while



libs/flake/KoShape.h
<http://git.reviewboard.kde.org/r/100809/#comment1555>

    There should be no blank after (,* and before )



libs/flake/KoShape.cpp
<http://git.reviewboard.kde.org/r/100809/#comment1579>

    There is no need to check clipPath before deleting it.



libs/flake/KoShape.cpp
<http://git.reviewboard.kde.org/r/100809/#comment1580>

    There should be no blank after ( and *  and before )


- Thorsten


On March 6, 2011, 6:01 p.m., Jan Hambrecht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100809/
> -----------------------------------------------------------
> 
> (Updated March 6, 2011, 6:01 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Summary
> -------
> 
> This patch implements clipping for shapes using arbitrary clipping paths. This is needed to properly support svg clipping.
> There is a new class KoClipPath which can be set on a shape. The clipping path is then used when painting the shape to set the clipping path on the painter.
> There are also two commands to set and to unset a clipping path on a shape.
> 
> 
> Diffs
> -----
> 
>   filters/karbon/svg/CMakeLists.txt 2a89825 
>   filters/karbon/svg/SvgClipPathHelper.h PRE-CREATION 
>   filters/karbon/svg/SvgClipPathHelper.cpp PRE-CREATION 
>   filters/karbon/svg/SvgGraphicContext.h dbb28ae 
>   filters/karbon/svg/SvgParser.h c9a09b1 
>   filters/karbon/svg/SvgParser.cpp 0372fde 
>   filters/karbon/svg/svgexport.h 6689299 
>   filters/karbon/svg/svgexport.cc 7627006 
>   karbon/data/karbon.rc cf0a238 
>   karbon/ui/KarbonView.h 235720f 
>   karbon/ui/KarbonView.cpp c4e3603 
>   libs/flake/CMakeLists.txt e153e14 
>   libs/flake/KoClipPath.h PRE-CREATION 
>   libs/flake/KoClipPath.cpp PRE-CREATION 
>   libs/flake/KoShape.h b4b3033 
>   libs/flake/KoShape.cpp c5f0fd9 
>   libs/flake/KoShapeManager.cpp 7f225f9 
>   libs/flake/KoShapeManagerPaintingStrategy.cpp 4a267c5 
>   libs/flake/KoShape_p.h 5874cbd 
>   libs/flake/commands/KoShapeClipCommand.h PRE-CREATION 
>   libs/flake/commands/KoShapeClipCommand.cpp PRE-CREATION 
>   libs/flake/commands/KoShapeUnclipCommand.h PRE-CREATION 
>   libs/flake/commands/KoShapeUnclipCommand.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/100809/diff
> 
> 
> Testing
> -------
> 
> Tested with svg files from the svg test suite as well as some kde icons like akregator.svgz (see bug 264411)
> 
> 
> Thanks,
> 
> Jan
> 
>

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