|
View:
New views
11 Messages
—
Rating Filter:
Alert me
|
|
|
Review Request: Konquest: add support for svg-themeable highlights for planet names and ships count----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/2086/ ----------------------------------------------------------- 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 _______________________________________________ kde-games-devel mailing list kde-games-devel@... https://mail.kde.org/mailman/listinfo/kde-games-devel |
|
|
Re: Review Request: Konquest: add support for svg-themeable highlights for planet names and ships count----------------------------------------------------------- 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 > > _______________________________________________ kde-games-devel mailing list kde-games-devel@... https://mail.kde.org/mailman/listinfo/kde-games-devel |
|
|
Re: Review Request: Konquest: add support for svg-themeable highlights for planet names and ships count> On 2009-11-07 15:10:49, Parker Coates wrote: > > trunk/KDE/kdegames/konquest/mapitems.cc, line 4 > > <http://reviewboard.kde.org/r/2086/diff/1/?file=13830#file13830line4> > > > > This copyright notices need dates to be valid. Could you at least add the year(s) to your own line? Will do > On 2009-11-07 15:10:49, Parker Coates wrote: > > trunk/KDE/kdegames/konquest/mapitems.cc, line 140 > > <http://reviewboard.kde.org/r/2086/diff/1/?file=13830#file13830line140> > > > > 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? Hehe. "bkgnd", not "bckgrnd" - two chars less. but anyway, can change this - no probs. This is just abbreviation i get used to... > On 2009-11-07 15:10:49, Parker Coates wrote: > > trunk/KDE/kdegames/konquest/mapitems.cc, lines 162-168 > > <http://reviewboard.kde.org/r/2086/diff/1/?file=13830#file13830line162> > > > > 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? Yeah, you absolutely right on both points. I must've overlooked this while being in a kind of "quick patch"-mode. Well, "quick"-mode rarely leads to good results - this prooves it once more for me ;) Will fix, thank you. - Dmitry ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/2086/#review2970 ----------------------------------------------------------- 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 > > _______________________________________________ kde-games-devel mailing list kde-games-devel@... https://mail.kde.org/mailman/listinfo/kde-games-devel |
|
|
Re: Review Request: Konquest: add support for svg-themeable highlights for planet names and ships count> On 2009-11-07 15:10:49, Parker Coates wrote: > > trunk/KDE/kdegames/konquest/mapitems.cc, line 140 > > <http://reviewboard.kde.org/r/2086/diff/1/?file=13830#file13830line140> > > > > 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? > > Dmitry Suzdalev wrote: > Hehe. "bkgnd", not "bckgrnd" - two chars less. but anyway, can change this - no probs. This is just abbreviation i get used to... Well, if anything I guess that proves that arbitrary abbreviations are hard to remember. :P I realise now that I commented on the wrong line. I meant to comment on the name of the SVG ID, because that's semi-public API (artist program interface). I couldn't care less about the name of the internal variable. :) - Parker ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/2086/#review2970 ----------------------------------------------------------- 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 > > _______________________________________________ kde-games-devel mailing list kde-games-devel@... https://mail.kde.org/mailman/listinfo/kde-games-devel |
|
|
Re: Review Request: Konquest: add support for svg-themeable highlights for planet names and ships count> On 2009-11-07 15:10:49, Parker Coates wrote: > > trunk/KDE/kdegames/konquest/mapitems.cc, line 140 > > <http://reviewboard.kde.org/r/2086/diff/1/?file=13830#file13830line140> > > > > 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? > > Dmitry Suzdalev wrote: > Hehe. "bkgnd", not "bckgrnd" - two chars less. but anyway, can change this - no probs. This is just abbreviation i get used to... > > Parker Coates wrote: > Well, if anything I guess that proves that arbitrary abbreviations are hard to remember. :P > > I realise now that I commented on the wrong line. I meant to comment on the name of the SVG ID, because that's semi-public API (artist program interface). I couldn't care less about the name of the internal variable. :) Ah. Ok, then I completely agree. will fix both places :) - Dmitry ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/2086/#review2970 ----------------------------------------------------------- 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 > > _______________________________________________ kde-games-devel mailing list kde-games-devel@... https://mail.kde.org/mailman/listinfo/kde-games-devel |
|
|
Re: Review Request: Konquest: add support for svg-themeable highlights for planet names and ships count----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/2086/ ----------------------------------------------------------- (Updated 2009-11-09 12:53:48.725540) Review request for KDE Games. Changes ------- Updated diff according to Parker's notes. 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 (updated) ----- 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 _______________________________________________ kde-games-devel mailing list kde-games-devel@... https://mail.kde.org/mailman/listinfo/kde-games-devel |
|
|
Re: Review Request: Konquest: add support for svg-themeable highlights for planet names and ships count----------------------------------------------------------- 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 > > _______________________________________________ kde-games-devel mailing list kde-games-devel@... https://mail.kde.org/mailman/listinfo/kde-games-devel |
|
|
Re: Review Request: Konquest: add support for svg-themeable highlights for planet names and ships count> On 2009-11-10 00:36:12, Parker Coates wrote: > > trunk/KDE/kdegames/konquest/mapitems.cc, line 156 > > <http://reviewboard.kde.org/r/2086/diff/2/?file=14223#file14223line156> > > > > 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. Nicely spotted, you're right :) On 2009-11-10 00:36:12, Dmitry Suzdalev wrote: > > 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. :) Well here i'm almost in the same situation, I was just implementing Eugenes request :) But at least I don't see that it's something bad. This is simply an extension that enables artists to do themes with certain color palette which otherwise would make playing impossible because names/numbers would not be visible. - Dmitry ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/2086/#review2995 ----------------------------------------------------------- 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 > > _______________________________________________ kde-games-devel mailing list kde-games-devel@... https://mail.kde.org/mailman/listinfo/kde-games-devel |
|
|
Re: Review Request: Konquest: add support for svg-themeable highlights for planet names and ships count----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/2086/ ----------------------------------------------------------- (Updated 2009-11-10 11:04:09.733163) Review request for KDE Games. Changes ------- Create pixmap without dimensions before searching it in cache 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 (updated) ----- trunk/KDE/kdegames/konquest/mapitems.h 1045562 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 _______________________________________________ kde-games-devel mailing list kde-games-devel@... https://mail.kde.org/mailman/listinfo/kde-games-devel |
|
|
Re: Review Request: Konquest: add support for svg-themeable highlights for planet names and ships count----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/2086/ ----------------------------------------------------------- (Updated 2009-11-10 11:08:14.390783) Review request for KDE Games. Changes ------- Oops, changes in default_theme.svgz shouldn't be committed, removed them from review. 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 (updated) ----- 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 _______________________________________________ kde-games-devel mailing list kde-games-devel@... https://mail.kde.org/mailman/listinfo/kde-games-devel |
|
|
Re: Review Request: Konquest: add support for svg-themeable highlights for planet names and ships countThis all look ok to me, you can commit it imho...
_______________________________________________ kde-games-devel mailing list kde-games-devel@... https://mail.kde.org/mailman/listinfo/kde-games-devel |
| Free embeddable forum powered by Nabble | Forum Help |