From kde-graphics-devel  Wed Jul 02 01:21:05 2008
From: Fredrik =?iso-8859-1?q?H=F6glund?= <fredrik () kde ! org>
Date: Wed, 02 Jul 2008 01:21:05 +0000
To: kde-graphics-devel
Subject: Re: [Kde-graphics-devel] Review Request: shadowBlur and shadowText
Message-Id: <200807020321.06770.fredrik () kde ! org>
X-MARC-Message: https://marc.info/?l=kde-graphics-devel&m=121496166915266

On Tuesday 01 July 2008 08:56, Jamboarder wrote:
> http://reviewboard.vidsolbach.de/r/64/
> 
> This patch exports a shadowBlur function and add a shadowText convenience function for libplasma.
> Folderview will be patched so that the title uses the shadowText function.  Other areas of plasma might find this function useful as well (krunner and perhaps Plasma::Icon comes to mind).

Hi Andrew,

I've looked at your patch, and I have a few comments:

Starting with the API, I'm not sure if it's a good idea to return a pixmap
with the original text and the shadow, because it then becomes the
caller's responsibility to figure out where it needs to draw the pixmap
in order to position the text. The position of the text within the pixmap
depends both on the blur radius and the offset, and if the caller uses the
default parameters there's simply no way for it to know where it should
draw the pixmap. The default parameters or the implementation could
never be changed without breaking existing code. With this API there's
also no way to specify the font that should be used for the text, it will
always be drawn with the applications's default font.

I would suggest that you change the API to something like this instead:

void drawShadowedText(QPainter *painter, const QPoint &pos,
	const QString &text, int radius, const QColor &shadowColor,
	const QPoint &offset)

This function would use the font and the pen color in the painter, and
figure out where it needs to render the shadow image relative to the text.

Not tying the shadow image to a specific paint device also seems
more future proof, in case we replace the implementation at some point
with one that uses Quasar.

I also see some issues in the implementation. Creating a temporary pixmap
just to start a QPainter on it to get to the font metrics for the default font
is wasteful, since you can access those metrics from the QApplication object.

You add padding to the shadow image to compensate for the blur radius,
but you then draw the text in the upper right corner, and this has the
effect that you end up with no padding on the left and top sides of the
image, and twice the needed padding on the right and bottom sides.

The code that creates the composite pixmap also assumes that the shadow
offset is positive, which it might not be. It will also size the composite
pixmap larger than it needs to be if the shadow offset is smaller than the
padding added to the image earlier.

Regards,
Fredrik

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