[Marble-devel] Review Request: Marble Clock Patch

[Bastian Holst]

> On 2010-08-05 09:59:07, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp, line 1094
> > <http://reviewboard.kde.org/r/4819/diff/3/?file=32476#file32476line1094>
> >
> >     see above. Bad naming imho. 
> >     
> >     setClockSpeed. 
> >     
> >     Do we really need this convenience method? Or does it just clutter up the model class?
> 
> hjain wrote:
>     see above.

Why not setClockSpeed()?


> On 2010-08-05 09:59:07, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp, line 1089
> > <http://reviewboard.kde.org/r/4819/diff/3/?file=32476#file32476line1089>
> >
> >     Hm, this is a bad name for the method imho. What is the "speed" of a model? That's something nobody will understand. Instead it should be named something like
> >     
> >     clockSpeed()
> >     
> >     Also: Do we really need a convenience method for this instead of just exposing the clock object?
> 
> hjain wrote:
>     We need a convenience method for MarbleClock class because MarbleClock class is not included in libmarble, hence, cannot be used directly in marble_part.cpp

Why not clockSpeed()?


> On 2010-08-05 09:59:07, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/QtMarbleConfigDialog.h, line 72
> > <http://reviewboard.kde.org/r/4819/diff/3/?file=32478#file32478line72>
> >
> >     what does "UTC()" mean? I don't get what this method does. Please use better naming.
> 
> hjain wrote:
>     UTC() is function which return the boolean value of UTC radiobutton in 'Date & Time' tab in Marble Configure Dialog Box. Kindly suggest some suitable name for this function. It is similar to MarbleSetting::uTC().

Still, a better name would be good. If you can't find one, please explain the function at least in a doxygen comment.


- Bastian


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


On 2010-08-08 22:09:26, hjain wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4819/
> -----------------------------------------------------------
> 
> (Updated 2010-08-08 22:09:26)
> 
> 
> Review request for marble and Bastian Holst.
> 
> 
> Summary
> -------
> 
> This patch has the following features:-
> 1) The older 'Sun Control' dialog box is broken into new 'Sun Control' dialog box and 'Time Control' dialog box.
> 2) The 'Configure Marble Desktop Globe' has 'Date and Time' tab which has configuration features for time.
> 3) The current time is shown in status bar.
> 4) The icon of the Sun is shown on the Earth for zenith feature in 'Sun Control' dialog box.
> 5) Toolbar action button are added for shadow, night map and zenith features(although shadow and night map toolbar buttons are disabled in this patch because of lack of appropriate icons for them).
> 6) The name of ExtDateTime class is changed to MarbleClock. The license of David Roberts(author of ExtDateTime class) has also been updated for this class with this permissions.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/QtMainWindow.h 1157379 
>   /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp 1157379 
>   /trunk/KDE/kdeedu/marble/src/bindings/python/sip/MarbleModel.sip 1157379 
>   /trunk/KDE/kdeedu/marble/src/bindings/python/sip/SunLocator.sip 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/ExtDateTime.h 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/ExtDateTime.cpp 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/LayerManager.cpp 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleClock.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleDataFacade.cpp 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleTimeSettingsWidget.ui PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/QtMarbleConfigDialog.h 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/QtMarbleConfigDialog.cpp 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/SunControlWidget.h 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/SunControlWidget.cpp 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/SunControlWidget.ui 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/SunLocator.h 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/SunLocator.cpp 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/TimeControlWidget.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/TimeControlWidget.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/TimeControlWidget.ui PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/blendings/SunLightBlending.cpp 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp 1157379 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingManager.cpp 1157379 
>   /trunk/KDE/kdeedu/marble/src/marble.kcfg 1157379 
>   /trunk/KDE/kdeedu/marble/src/marble_part.h 1157379 
>   /trunk/KDE/kdeedu/marble/src/marble_part.cpp 1157379 
>   /trunk/KDE/kdeedu/marble/src/marble_part.rc 1157379 
>   /trunk/KDE/kdeedu/marble/src/marbleui.rc 1157379 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/stars/StarsPlugin.h 1157379 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/stars/StarsPlugin.cpp 1157379 
> 
> Diff: http://reviewboard.kde.org/r/4819/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> hjain
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/marble-devel/attachments/20100809/29df73b1/attachment.htm