[Kde-games-devel] Review Request: Konquest: add support for svg-themeable highlights for planet names and ships count

[Parker Coates]

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



trunk/KDE/kdegames/konquest/mapitems.cc
<http://reviewboard.kde.org/r/2086/#comment2446>

    If I'm not mistaken, creating a QPixmap with a width and height allocates memory for the pixel data even if you don't initialise that pixel data with anything. Since this pixmap is just "thrown away" if the key is already in the cache, it would probably make more sense to create a null QPixmap here and then only set the width and height if it wasn't found in the cache.


Other than that, the patch looks good. Of course, I'm not familiar with the Konquest so I can't really comment on whether it makes sense for Konquest or not. :)

- Parker


On 2009-11-09 12:53:48, Dmitry Suzdalev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2086/
> -----------------------------------------------------------
> 
> (Updated 2009-11-09 12:53:48)
> 
> 
> Review request for KDE Games.
> 
> 
> Summary
> -------
> 
> Implemented an Eugene's (it-s) request:
> 
> For proper themeing he needs the way to "highlight" the planet names and ships count texts that are drawn near each planet.
> This is needed because otherwise they are always drawn in black text and this doesn't correlate well with some backgrounds.
> We communicated that it would be great for our artists to have two additional svg elements which will be drawn under the texts, 
> so they can presizely control highlights.
> 
> So that's what i've done - i've added a support for two svg elements with IDs "planet_name_bkgnd" and "planet_ship_count_bkgnd" and made 
> them to be rendered under the respective texts.
> 
> Along with this change i did some minor things:
> - Stored some often used values in variables - instead of calling functions and evaluating same expressions every time
> - Fixed a typo which might prevented caching to work properly - QString+int won't work :) it has to be QString+QString::number(int).
> - Replaced painting custom pixmap and setting it as alfamask with simpler pix.fill(Qt::transparent)
> - Do not use hardcoded font heights/widths - calculate them with QFontMetrics
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdegames/konquest/mapitems.h 1045562 
>   trunk/KDE/kdegames/konquest/mapitems.cc 1045668 
> 
> Diff: http://reviewboard.kde.org/r/2086/diff
> 
> 
> Testing
> -------
> 
> Tested rendering. Looks ok.
> 
> 
> Thanks,
> 
> Dmitry
> 
>