Review Request: Add Blob Tool to Calligra

[Thorsten Zachmann]

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


Nice work. I'm impressed with how little code is needed for the stuff you have created.

There seems to be a problem generating the outline when the shape generated contains holes. See http://www.zagge.de/files/blobtool.png for a screenshot. The painting started in the bottom right corner.

When trying to paint a O there is also a problem generating the outline.


plugins/vectortools/KoBlobTool.h
<http://git.reviewboard.kde.org/r/106283/#comment14468>

    If it is not needed please remove.



plugins/vectortools/KoBlobTool.cpp
<http://git.reviewboard.kde.org/r/106283/#comment14469>

    This is a very very hacky thing to do. The common functionality should be moved to e.g. flake so it can be reused in both parts without such a hack.



plugins/vectortools/KoBlobTool.cpp
<http://git.reviewboard.kde.org/r/106283/#comment14470>

    Please move to the initialization list of the constructor. Also the m_bg should be moved there.



plugins/vectortools/KoBlobTool.cpp
<http://git.reviewboard.kde.org/r/106283/#comment14471>

    It seems like m_stroke and m_bg are not deleted when the tool is destructed and therefore leeking memory.



plugins/vectortools/KoBlobTool.cpp
<http://git.reviewboard.kde.org/r/106283/#comment14472>

    If the include is needed please move it to the beginning of the file.



plugins/vectortools/KoBlobTool.cpp
<http://git.reviewboard.kde.org/r/106283/#comment14473>

    qDebug should only be used for your own debugging and not be committed. Either removed or use kDebug here.



plugins/vectortools/KoBlobTool.cpp
<http://git.reviewboard.kde.org/r/106283/#comment14475>

    The indention is wrong here.



plugins/vectortools/KoBlobTool.cpp
<http://git.reviewboard.kde.org/r/106283/#comment14476>

    these 2 member need to be deleted to not leak memory before asigning 0.



plugins/vectortools/KoBlobTool.cpp
<http://git.reviewboard.kde.org/r/106283/#comment14474>

    This leaks memory as you always generate a new KoPathShape without deleting the old one.



plugins/vectortools/KoBlobTool.cpp
<http://git.reviewboard.kde.org/r/106283/#comment14478>

    how about selecting a shape and then when painting it will add to that shape and create one shape when they overlap?



plugins/vectortools/KoBlobTool.cpp
<http://git.reviewboard.kde.org/r/106283/#comment14477>

    Please use only one space.


- Thorsten Zachmann


On Aug. 30, 2012, 10:59 p.m., José Luis Vergara wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106283/
> -----------------------------------------------------------
> 
> (Updated Aug. 30, 2012, 10:59 p.m.)
> 
> 
> Review request for Calligra, Boudewijn Rempt, Inge Wallin, C. Boemann, and Sven Langkamp.
> 
> 
> Description
> -------
> 
> Blob tool is the name for a vector brush that creates a broad stroke that fuses with itself and with other shapes that have the same styling, thus helping to create more organic-looking vector images.
> This patch implements a very basic version of this tool, missing the ability to fuse shapes with other shapes, which I am postponing until we fix bug 297623 (see thread http://mail.kde.org/pipermail/calligra-devel/2012-August/006633.html ).
> Further details and a link to a video in blog  http://pentalis.org/kritablog/?p=315
> 
> Note: some dependencies (like KarbonCurveFit) need to be moved, I will post a separate review for that, because I need to discuss what directory it belongs to.
> 
> I need an easy way to make clear I commit my code in either license, LGPL, GPL, or anything we need for it to fit well in our project; it's not easy to convey since files accumulate more contributors the more time passes by. Suggestions appreciated.
> 
> 
> Diffs
> -----
> 
>   plugins/CMakeLists.txt 526499f 
>   plugins/vectortools/CMakeLists.txt PRE-CREATION 
>   plugins/vectortools/KoBlobTool.h PRE-CREATION 
>   plugins/vectortools/KoBlobTool.cpp PRE-CREATION 
>   plugins/vectortools/KoBlobToolFactory.h PRE-CREATION 
>   plugins/vectortools/KoBlobToolFactory.cpp PRE-CREATION 
>   plugins/vectortools/KoVectorToolsPlugin.h PRE-CREATION 
>   plugins/vectortools/KoVectorToolsPlugin.cpp PRE-CREATION 
>   plugins/vectortools/calligravectortools.desktop PRE-CREATION 
>   plugins/vectortools/hi22-action-blob-tool.png PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/106283/diff/
> 
> 
> Testing
> -------
> 
> Tested compilation, loading in applications Karbon and Krita, and basic functioning of the tool. It works (incompletely) as intended for a small group of use cases. Also tested speed, it needs to be faster, but is usable.
> 
> 
> Thanks,
> 
> José Luis Vergara
> 
>

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