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

[Torsten Rahn]

> On 2010-12-29 15:42:52, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/Planet.cpp, line 351
> > <http://svn.reviewboard.kde.org/r/6223/diff/3/?file=43268#file43268line351>
> >
> >     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.
> 
> den wrote:
>     Would this not occur with the method 
>     
>     Planet::Planet( const QString& id )
>         : d( new PlanetPrivate )
>     
>     as well. Also could you suggest how I could achieve this because a list of variables of viable planet IDs seems a bad way to do it.

Yes, the danger is already in the current code. So ideally we'd need some kind of registration mechanism in the future for the Planet class. 

So if you come up with some nice idea how to solve this (which might result in a new task maybe ;) don't hesitate to let us know :)

I have marked the task as complete. 


- Torsten


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


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/864cb042/attachment-0001.htm