[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: Review Request: Filter effects for karbon
From:       "Thomas Zander" <zander () kde ! org>
Date:       2009-04-13 12:08:31
Message-ID: 20090413120831.24295.84566 () localhost
[Download RAW message or body]


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


In general this looks like a great approach; though I'm a tad worried about big \
shapes on a high-resolution printer. How much memory that will take and all that.  \
Would be nice to test that.

Still some coding style issues, which I'll ignore.  Below are more substantial \
comments :)


trunk/koffice/libs/flake/KoFilterEffect.h
<http://reviewboard.kde.org/r/580/#comment593>

    Why pass in an id and name if its never used?



trunk/koffice/libs/flake/KoFilterEffect.h
<http://reviewboard.kde.org/r/580/#comment589>

    QImage is a sharedData based document, passing it as a pointer will give crashes \
under certain situations.  I suggest passing it as 'QImage &image'  
    oh, and on that note, don't abbreviate variables in koffice-libs API, please.



trunk/koffice/libs/flake/KoFilterEffect.h
<http://reviewboard.kde.org/r/580/#comment590>

    The API is a bit unclear; I don't know what 'needed' is and I am confused why you \
need to pass something called 'needed' to a method called 'needed' to get back the \
'needed' rect.  This gives me circular dependencies ;)



trunk/koffice/libs/flake/KoFilterEffect.h
<http://reviewboard.kde.org/r/580/#comment591>

    s/need/needs



trunk/koffice/libs/flake/KoFilterEffect.h
<http://reviewboard.kde.org/r/580/#comment594>

    I think we can avoid the d pointer in a all-pure-virtual class.
    If we really need it, then we should use a class, not a struct.



trunk/koffice/libs/flake/KoFilterEffect.cpp
<http://reviewboard.kde.org/r/580/#comment595>

    forgot to delete the d-pointer.  But I think we can remove the whole d-pointer \
anyway, see comment to the header file :)



trunk/koffice/libs/flake/KoShape.h
<http://reviewboard.kde.org/r/580/#comment596>

    You should not return a reference to a QList (or any QSharedData class) as it \
exposes the internals and will cause crashes if the caller keeps a reference to the \
list around when the shape is deleted.



trunk/koffice/libs/flake/KoShape.h
<http://reviewboard.kde.org/r/580/#comment597>

    Maybe we should use the name 'stack' instead of 'list' to make clear the idea of \
how the filters are applied to the rendering.



trunk/koffice/libs/flake/KoShapeManager.cpp
<http://reviewboard.kde.org/r/580/#comment598>

    Once is enough ;)



trunk/koffice/libs/flake/KoShapeManager.cpp
<http://reviewboard.kde.org/r/580/#comment599>

    You probably want to use qCeil for the width/height instead of rounding using \
toRect()


- Thomas


On 2009-04-13 04:34:04, Cyrille Berger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/580/
> -----------------------------------------------------------
> 
> (Updated 2009-04-13 04:34:04)
> 
> 
> Review request for KOffice.
> 
> 
> Summary
> -------
> 
> New version of the patch (couldn't update the previous one, so opening a new \
> request). This time, the rendering is done in flake, KoShapeManager. 
> For now, there is still the karbon plugin to create the filter, obviously it will \
> be replaced by a tool... The real interesting part is the patch to lib/flake. 
> I have two issues:
> * one the rect created to do the filtering is too big when the shape is rotated, I \
> was also unable to use the painter.clip to reduce the rect, not sure why, but if I \
>                 tried to use it, then only part of the shape became visible
> * there seem to be a bug in Qt, when you have an image with a transparent bottom, \
> then when it is rotated, there is a thin line appearing at the bottom of the image, \
> one can easily reproduce it with the picture shape 
> If it's ok, I would like to commit the flake part, and then work on a tool.
> 
> 
> Diffs
> -----
> 
> trunk/koffice/karbon/plugins/CMakeLists.txt 951712 
> trunk/koffice/karbon/plugins/effectsfilters/CMakeLists.txt PRE-CREATION 
> trunk/koffice/karbon/plugins/effectsfilters/FilterEffects.h PRE-CREATION 
> trunk/koffice/karbon/plugins/effectsfilters/FilterEffects.cpp PRE-CREATION 
> trunk/koffice/karbon/plugins/effectsfilters/KoInvertFilterEffect.h PRE-CREATION 
> trunk/koffice/karbon/plugins/effectsfilters/KoInvertFilterEffect.cpp PRE-CREATION 
> trunk/koffice/karbon/plugins/effectsfilters/karbonfiltereffects.rc PRE-CREATION 
> trunk/koffice/libs/flake/CMakeLists.txt 951712 
> trunk/koffice/libs/flake/KoFilterEffect.h PRE-CREATION 
> trunk/koffice/libs/flake/KoFilterEffect.cpp PRE-CREATION 
> trunk/koffice/libs/flake/KoShape.h 951712 
> trunk/koffice/libs/flake/KoShape.cpp 951712 
> trunk/koffice/libs/flake/KoShapeManager.cpp 951712 
> 
> Diff: http://reviewboard.kde.org/r/580/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Cyrille
> 
> 

_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel


[prev in list] [next in list] [prev in thread] [next in thread]