[Kde-games-devel] Review Request: Porting Kapman from Phonon to KgSound

[Frederik Schwarzer]

> On March 22, 2012, 11:48 a.m., Frederik Schwarzer wrote:
> > /trunk/KDE/kdegames/kapman/game.cpp, lines 37-44
> > <http://svn.reviewboard.kde.org/r/6914/diff/1/?file=47721#file47721line37>
> >
> >     Would it not better to put these into the initializer list? This way they are intialised twice, iirc.
> 
> Roney Gomes wrote:
>     Can you point another place in the code where the sounds are also loaded? I couldn't see it.

I thought the right thing and said the wrong thing.
If you have elements of a class type (here KgSound), they are all initialised in stage 1 of the constructor. Stage 2 is everything between { and }. So in stage 1, the standard constructor of the element is called and the element is initialised and assigned some standard value. In stage 2 you then assign the real value. So two values are assigned.
By using an initialiser list, you can avoid the double assignment.


> On March 22, 2012, 11:48 a.m., Frederik Schwarzer wrote:
> > /trunk/KDE/kdegames/kapman/game.cpp, lines 72-73
> > <http://svn.reviewboard.kde.org/r/6914/diff/1/?file=47721#file47721line72>
> >
> >     Please avoid unrelated white space changes. There are several.
> 
> Roney Gomes wrote:
>     I'm really sorry about that. My editor is totally out of the standards. But please, notice that are a lot of other white spaces in places I haven't changed.

Yes, I know there are many white spaces in those files. The point of not changing them when changing functionality is to keep the patch small and simple. White space fixes should go in a separate commit. It's not a deal breaker, but a good idea in general.


- Frederik


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


On March 22, 2012, 3:43 p.m., Roney Gomes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6914/
> -----------------------------------------------------------
> 
> (Updated March 22, 2012, 3:43 p.m.)
> 
> 
> Review request for KDE Games.
> 
> 
> Description
> -------
> 
> I've ported kapman to KgSound as requested in the mailing list. The files changed were game.cpp and game.h.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdegames/kapman/game.h 1286463 
>   /trunk/KDE/kdegames/kapman/game.cpp 1286463 
> 
> Diff: http://svn.reviewboard.kde.org/r/6914/diff/
> 
> 
> Testing
> -------
> 
> I've played and observed whether the sounds were been played in the proper situations.
> 
> 
> Thanks,
> 
> Roney Gomes
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-games-devel/attachments/20120322/09a14929/attachment.html>