[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/#review2970
-----------------------------------------------------------


Looks good, but...


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

    This copyright notices need dates to be valid. Could you at least add the year(s) to your own line?



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

    Is it really worth it abbreviating "bckgrnd"? This is really just a matter personal preference, the name is already quite long. Is saving three vowels really worth the extra effort of trying to remember the correct abbreviation?



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

    Why not move this into its own function? The exact same set of steps was just down for "planet_name_bckgrnd". Having to repeat the SVG ID several times is unpleasant.
    
    Also, the width of the text for each planet is presumably going to be different, yet you don't include the pixmap dimensions in the key used for storing and retrieving the pixmap from cache. Is this actually working as you expect it to?


- Parker


On 2009-11-06 16:23:37, Dmitry Suzdalev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2086/
> -----------------------------------------------------------
> 
> (Updated 2009-11-06 16:23:37)
> 
> 
> 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.cc 1045668 
>   trunk/KDE/kdegames/konquest/pics/default_theme.svgz UNKNOWN 
> 
> Diff: http://reviewboard.kde.org/r/2086/diff
> 
> 
> Testing
> -------
> 
> Tested rendering. Looks ok.
> 
> 
> Thanks,
> 
> Dmitry
> 
>