[Marble-devel] Review Request: Marble Clock Patch

hjain.itbhu at gmail.com hjain.itbhu at gmail.com
Sat Aug 7 16:04:51 CEST 2010 


> On 2010-08-07 12:12:21, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp, line 104
> > <http://reviewboard.kde.org/r/4819/diff/4/?file=32609#file32609line104>
> >
> >     Please write a space inside both opening and closing parenthesis.

fixed it.


> On 2010-08-07 12:12:21, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.cpp, line 786
> > <http://reviewboard.kde.org/r/4819/diff/4/?file=32637#file32637line786>
> >
> >     Usually one will use three dots at the end of the action string, if there is a dialog being shown. Why do you remove it?

I was unaware of this fact :(
fixed it.


> On 2010-08-07 12:12:21, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.cpp, line 795
> > <http://reviewboard.kde.org/r/4819/diff/4/?file=32637#file32637line795>
> >
> >     It should be "&Time Control..." here, too.

fixed it.


> On 2010-08-07 12:12:21, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.cpp, line 876
> > <http://reviewboard.kde.org/r/4819/diff/4/?file=32637#file32637line876>
> >
> >     Again a missing space character.

fixed it.


> On 2010-08-07 12:12:21, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.cpp, line 1488
> > <http://reviewboard.kde.org/r/4819/diff/4/?file=32637#file32637line1488>
> >
> >     Couldn't we use negative integer values here?

Entries are according to the order of Timezone name in drop down box in 'Date and Time' tab in Configure Marble Box


> On 2010-08-07 12:12:21, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp, line 665
> > <http://reviewboard.kde.org/r/4819/diff/4/?file=32609#file32609line665>
> >
> >     You still don't use QLocale here, but a static time format.

fixed it.


> On 2010-08-07 12:12:21, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/marble.kcfg, line 72
> > <http://reviewboard.kde.org/r/4819/diff/4/?file=32635#file32635line72>
> >
> >     I'd write "utc" instead of "UTC" here, to prevent things like "uTC"

fixed it.


> On 2010-08-07 12:12:21, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.cpp, line 970
> > <http://reviewboard.kde.org/r/4819/diff/4/?file=32637#file32637line970>
> >
> >     I won't use this static string as a template here.

fixed it.


> On 2010-08-07 12:12:21, Bastian Holst wrote:
> > /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp, line 728
> > <http://reviewboard.kde.org/r/4819/diff/4/?file=32609#file32609line728>
> >
> >     I do not think that you can get the maximum length with that. You don't know which format is used and you don't know it this one is the longest. Additionally you don't use tr( DATETIME_STRING ) here.

fixed it.


- hjain


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


On 2010-08-05 20:30:37, hjain wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4819/
> -----------------------------------------------------------
> 
> (Updated 2010-08-05 20:30:37)
> 
> 
> 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/20100807/966c4dcf/attachment-0001.htm

More information about the Marble-devel mailing list