[Marble-devel] Review Request: Marble Clock Patch
hjain.itbhu at gmail.com
hjain.itbhu at gmail.com
Thu Aug 5 19:35:26 CEST 2010
> 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?
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
> 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?
see above.
> On 2010-08-05 09:59:07, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp, line 1099
> > <http://reviewboard.kde.org/r/4819/diff/3/?file=32476#file32476line1099>
> >
> > Do we really need this convenience method or wouldn't exposing the clock be good enough? Or do we just not want to expose the clock as a public class? I'm not sure.
see above.
> On 2010-08-05 09:59:07, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.h, line 131
> > <http://reviewboard.kde.org/r/4819/diff/3/?file=32492#file32492line131>
> >
> > "Show Zenith" is not a proper name imho.
> >
> > methods should always be concise and easily tell what the method is about.
> >
> > So
> >
> > void showSunInZenith(bool) might be a better method name.
fixed it.
> On 2010-08-05 09:59:07, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/plugins/render/stars/StarsPlugin.cpp, line 227
> > <http://reviewboard.kde.org/r/4819/diff/3/?file=32497#file32497line227>
> >
> > YAY, you got it implemented correctly. Very cool :-)
thanks :)
> On 2010-08-05 09:59:07, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp, line 611
> > <http://reviewboard.kde.org/r/4819/diff/3/?file=32465#file32465line611>
> >
> > I think a comment here would be nice explaining that this dialog is not supposed to be modal and why it's not supposed to be modal.
Added following comment before the statement:-
/* m_timeControlDialog is a modeless dialog so that user may adjust time and interact with main application simultaneously.*/
> On 2010-08-05 09:59:07, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp, line 665
> > <http://reviewboard.kde.org/r/4819/diff/3/?file=32465#file32465line665>
> >
> > Hm, why not adhere to the timeformat provided via QLocale? In Germany and other countries putting the month in front of the day and the year is not used at all.
fixed it.
Now ShortFormat of datetime provided via QLocale is used.
> On 2010-08-05 09:59:07, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp, line 728
> > <http://reviewboard.kde.org/r/4819/diff/3/?file=32465#file32465line728>
> >
> > See comment about QLocale use above.
edited to :-
QString( "%1 00-00-00T00:00 AM" ).arg( DATETIME_STRING );
> On 2010-08-05 09:59:07, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.h, line 221
> > <http://reviewboard.kde.org/r/4819/diff/3/?file=32492#file32492line221>
> >
> > This doesn't look correct to me.
> >
> > The choice is really between 3 different states:
> >
> > 1) Shadow off
> > 2) Normal Shadow on
> > 3) Nightmap Shadow on
> >
> > Two toggle actions buttons don't sufficiently represent this in terms of UI since the nightmap action depends on the shadow action. We need a better solution here.
fixed it.
> On 2010-08-05 09:59:07, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp, line 738
> > <http://reviewboard.kde.org/r/4819/diff/3/?file=32465#file32465line738>
> >
> > I think that this way of using signals and slots is atypical. Usually the changed datetime would get passed as a parameter.
> > Wouldn't this also get around having the clock pointer as a member (which to me looks a bit messy) ? Why do we want to have the clock as a member in here? Just makes the code more complex imho.
> >
MarbleClock class is not accessible from here directly.
> On 2010-08-05 09:59:07, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/QtMarbleConfigDialog.h, line 71
> > <http://reviewboard.kde.org/r/4819/diff/3/?file=32478#file32478line71>
> >
> > systemTimezone vs. customTimezone should be an enum from what I understand.
The Kconfig only saves boolean value for radiobuttons, so I am using boolean values for systemTimezone and lastSessionTimezone for kde version. To keep the similarity between the code for kde and qtonly, I am using boolean values in qt-only too
> On 2010-08-05 09:59:07, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.cpp, line 829
> > <http://reviewboard.kde.org/r/4819/diff/3/?file=32493#file32493line829>
> >
> > See my comment above about the problematic mapping of the three states to two toggle buttons.
There will be only one toggle button to show/hide shadow.
- hjain
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4819/#review6811
-----------------------------------------------------------
On 2010-08-04 22:13:25, hjain wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4819/
> -----------------------------------------------------------
>
> (Updated 2010-08-04 22:13:25)
>
>
> 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/20100805/14155309/attachment-0001.htm
More information about the Marble-devel
mailing list