Review Request: fix rendering of klineedit clear button
Michael Pyne
mpyne at kde.org
Sun Apr 25 04:00:52 BST 2010
-----------------------------------------------------------
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
>
>
More information about the kde-core-devel
mailing list