Review Request: fix rendering of klineedit clear button

[Michael Pyne]

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


I like the idea, and have actually used it elsewhere recently since fredrikh pointed it out on TechBase. I have a couple of specific quibbles and also want to point out that the coding style is kind of hit-or-miss in your changes. You should strive to maintain the style of the current source. I haven't read it all but it looks like it is trying to maintain kdelibs coding style policy, so that means things like: if (conditional) {, or function(param1, param2);

I'm not sure if the coding style did that on purpose (have a space between keywords and ( but not between functions/methods and ( ) but that seems to be consistently used in the original code.

Other than that I think it's good to go, I'll do the compile/run test after revision. ;)


/trunk/KDE/kdelibs/kdeui/widgets/klineedit_p.h
<http://reviewboard.kde.org/r/3804/#comment4706>

    Using QIcon is probably the right thing to do but I think it's still important to cache the pixmap instead of always going through QIcon::pixmap(), which has to do a lot of processing everytime it's called, even if the pixmap will typically be cached (see src/gui/image/qicon.cpp). Perhaps try just adding an event handler for when the enabled state changes to re-cache the right pixmap.



/trunk/KDE/kdelibs/kdeui/widgets/klineedit_p.h
<http://reviewboard.kde.org/r/3804/#comment4707>

    You should wait to declare this until you know it will be needed. (i.e. after your timeline and null size checks)


- Michael


On 2010-04-25 02:35:12, Hugo Pereira Da Costa wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3804/
> -----------------------------------------------------------
> 
> (Updated 2010-04-25 02:35:12)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> this patch
> - fixes the rendering of animated klineedit clear button, to use QPainter compositing mode rather than QPainter::setOpacity to fake semi-transparent clearButton during animation. This should be more efficient as QPainter::setOpacity (last time I checked) was using the rasterEngine. 
> 
> - uses a QIcon rather than QPixmap to store the clearButton pixmap. This allows to have the proper icon effect when rendering enabled/disabled klineedit. 
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kdeui/widgets/klineedit_p.h 1118236 
> 
> Diff: http://reviewboard.kde.org/r/3804/diff
> 
> 
> Testing
> -------
> 
> in konqueror and dolphin, as well as custom made test widgets.
> Test done in klineedit, as well as editable kcombobox
> Using either oxygen, cleanlooks and plastik style. 
> 
> Finally, tested with/without the animation being enabled
> 
> 
> Thanks,
> 
> Hugo
> 
>