Review Request: GUI to edit pictureshapes

[Silvio Heinrich]

> On Dec. 21, 2011, 8:20 a.m., Thorsten Zachmann wrote:
> > plugins/pictureshape/ClippingRect.cpp, lines 49-52
> > <http://git.reviewboard.kde.org/r/103402/diff/2/?file=43943#file43943line49>
> >
> >     there should be only 1 space between the end of the variable and the *. Please change that on all occurrences.

Don't I have just a little freedom to bring a bit of visual structure in the code?
Anyway, I changed it.


> On Dec. 21, 2011, 8:20 a.m., Thorsten Zachmann wrote:
> > plugins/pictureshape/PictureShape.h, lines 44-56
> > <http://git.reviewboard.kde.org/r/103402/diff/2/?file=43950#file43950line44>
> >
> >     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.

It has nothing to do with a mutex it's just not possible to add slots to the pictureShape class because MOC complains.


> On Dec. 21, 2011, 8:20 a.m., Thorsten Zachmann wrote:
> > plugins/pictureshape/PictureShape.cpp, line 239
> > <http://git.reviewboard.kde.org/r/103402/diff/2/?file=43951#file43951line239>
> >
> >     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?

There are no cropped images in the cache. The images are only scaled down to an optimal viewing size.
The images are clipped when painted (the QPainter::drawPixmap(...) method does it).


> On Dec. 21, 2011, 8:20 a.m., Thorsten Zachmann wrote:
> > plugins/pictureshape/ChangeImageCommand.cpp, lines 52-81
> > <http://git.reviewboard.kde.org/r/103402/diff/2/?file=43941#file43941line52>
> >
> >     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.

I wouldn't like to do this.
It will just add more code without any real benefit in usability.
it doesn't matter if the user knows if the color-mode, the cropping area or the url changed.
This is not an image processing program. Maybe if the picture tool would have more options but not just for three options.


> On Dec. 21, 2011, 8:20 a.m., Thorsten Zachmann wrote:
> > plugins/pictureshape/ClippingRect.h, line 31
> > <http://git.reviewboard.kde.org/r/103402/diff/2/?file=43942#file43942line31>
> >
> >     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.

It's just a structure with a few helping methods. It would be overhead of code. I would rather like to put everything back in the header because everything was clearer back then.


- Silvio


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


On Jan. 2, 2012, 4:08 p.m., Silvio Heinrich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103402/
> -----------------------------------------------------------
> 
> (Updated Jan. 2, 2012, 4:08 p.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/20120102/177507eb/attachment.htm>