[Kde-games-devel] Re: Review Request: Port KLines to KGameRenderer

[Lindsay Mathieson]

Thanks Kevin, I'll get onto those.

When updating the patch, is it best to start a new one or just add the 
changes as a diff to the existing reviewboard entry?

On 06/02/11 22:17, Kevin Krammer wrote:
> This is an automatically generated e-mail. To reply, visit: 
> http://svn.reviewboard.kde.org/r/5399/
>
>
> Not a KLines developer so just a few general hints.
>
> /trunk/KDE/kdegames/klines/ballitem.cpp 
> <http://svn.reviewboard.kde.org/r/5399/diff/3/?file=38349#file38349line31> 
> (Diff revision 3)
>
> 29 	
>      :  QGraphicsPixmapItem(  0,  parent  )
> 	31 	
>    :  KGameRenderedItem(KLinesRenderer::renderer()  ,  "",  NULL)
>
> Use QString() instead of "".
> QString() is a very cheap operation, it only creates the handle object, without any data.
>
> /trunk/KDE/kdegames/klines/ballitem.cpp 
> <http://svn.reviewboard.kde.org/r/5399/diff/3/?file=38349#file38349line48> 
> (Diff revision 3)
>
> 45 	
>          setPixmap(  KLinesRenderer::self()->ballPixmap(  m_color  )  );
> 	48 	
>        setSpriteKey(KLinesRenderer::ballPixmapId(m_color));
>
> Seems to change the coding style regarding whitespace within parentheses.
>
> /trunk/KDE/kdegames/klines/renderer.h 
> <http://svn.reviewboard.kde.org/r/5399/diff/3/?file=38352#file38352line38> 
> (Diff revision 3)
>
> 38 	
>   * Only one instance of this class exists during a program run.
> 	35 	
>   * Only one instance of this class exists during a program run.
>
> Is this part of the comment still applicable after the change to "all members static"?
>
> /trunk/KDE/kdegames/klines/renderer.cpp 
> <http://svn.reviewboard.kde.org/r/5399/diff/3/?file=38353#file38353line71> 
> (Diff revision 3)
>
> 	
> KLinesRenderer	*g_KLinesRenderer = NULL;
>
> 	
> 	70 	
> KLinesRenderer	*g_KLinesRenderer  =  NULL;
>
> Is this needed?
>
> /trunk/KDE/kdegames/klines/renderer.cpp 
> <http://svn.reviewboard.kde.org/r/5399/diff/3/?file=38353#file38353line106> 
> (Diff revision 3)
>
> 	
> KLinesRenderer	*g_KLinesRenderer = NULL;
> 87 	
>      QString  id  =  color2char(  color  )+QString(  "_rest"  );
> 	101 	
>    return  color2char(  color  )+QString(  "_rest"  );
>
> When converting an ASCII string literal into a QString, using QLatin1String(literal) is preferable.
> I don't remember why exactly, I think it makes it faster to convert the 8bit literal into Qt's internal 16bit representation due to not having to consider UTF-8 multi byte sequences
>
> /trunk/KDE/kdegames/klines/renderer.cpp 
> <http://svn.reviewboard.kde.org/r/5399/diff/3/?file=38353#file38353line131> 
> (Diff revision 3)
>
> 	
> KLinesRenderer	*g_KLinesRenderer = NULL;
> 107 	
>          return  QPixmap();
> 	121 	
>          return  "";
>
> QString()
>
> - Kevin
>
>
> On September 25th, 2010, 12:41 p.m., Lindsay Mathieson wrote:
>
> Review request for KDE Games.
> By Lindsay Mathieson.
>
> /Updated Sept. 25, 2010, 12:41 p.m./
>
>
>   Description
>
> Updates klines to use KGameRenderer and KGameRenderedItem in place of QSvgRenderer and QGraphicsPixmapItem
> Removes KPixmapCache usage
>
>
>   Testing
>
> Playing game
> Changing themes
> Resizing game
> Comparison with original
>
>
>   Diffs
>
>     * /trunk/KDE/kdegames/klines/scene.cpp (1177731)
>     * /trunk/KDE/kdegames/klines/renderer.cpp (1177731)
>     * /trunk/KDE/kdegames/klines/renderer.h (1177731)
>     * /trunk/KDE/kdegames/klines/previewitem.cpp (1177731)
>     * /trunk/KDE/kdegames/klines/klines.cpp (1177731)
>     * /trunk/KDE/kdegames/klines/ballitem.cpp (1177731)
>     * /trunk/KDE/kdegames/klines/ballitem.h (1177731)
>     * /trunk/KDE/kdegames/klines/animator.cpp (1177731)
>
> View Diff <http://svn.reviewboard.kde.org/r/5399/diff/>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-games-devel/attachments/20110208/ecbedff3/attachment.htm