[Marble-devel] Re: Review Request: Add planet option to map creation wizard

[Torsten Rahn]

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


There are still some minor implementation issues. Otherwise this looks pretty good already.


/trunk/KDE/kdeedu/marble/src/lib/Planet.h
<http://svn.reviewboard.kde.org/r/6223/#comment10472>

    This method should be const ... or much better: static. In case this would be static you wouldn't need to artificially create an object in order to retrieve all possible values.



/trunk/KDE/kdeedu/marble/src/lib/Planet.cpp
<http://svn.reviewboard.kde.org/r/6223/#comment10473>

    The implementation should be closer in sync with the one of the name method. Currently it would easily be possible to accidently have different string ids in both implementations, possibly contradicting each other. This makes maintenance harder and the implementation more prone to errors.


- Torsten


On 2010-12-29 13:37:30, den wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/6223/
> -----------------------------------------------------------
> 
> (Updated 2010-12-29 13:37:30)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> Google Code In Task - http://socghop.appspot.com/gci/task/show/google/gci2010/kde/t129277012002
> 
> Adds a planet option combobox to the map creation wizard page.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/lib/MapWizard.h 1210090 
>   /trunk/KDE/kdeedu/marble/src/lib/MapWizard.cpp 1210090 
>   /trunk/KDE/kdeedu/marble/src/lib/MapWizard.ui 1210090 
>   /trunk/KDE/kdeedu/marble/src/lib/Planet.h 1210090 
>   /trunk/KDE/kdeedu/marble/src/lib/Planet.cpp 1210090 
> 
> Diff: http://svn.reviewboard.kde.org/r/6223/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> 
>   http://svn.reviewboard.kde.org/r/6223/s/591/
> 
> 
> Thanks,
> 
> den
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/marble-devel/attachments/20101229/15291b92/attachment-0001.htm