[Marble-devel] Review Request: Marble Clock Patch
hjain.itbhu at gmail.com
hjain.itbhu at gmail.com
Thu Aug 5 00:07:04 CEST 2010
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/plugins/render/stars/StarsPlugin.cpp, line 132
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32207#file32207line132>
> >
> > whitespace in inner parenthesis at the end
fixed it.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/plugins/render/stars/StarsPlugin.cpp, line 30
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32207#file32207line30>
> >
> > m_marbleWidget( 0 )
fixed it.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.cpp, line 834
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32203#file32203line834>
> >
> > Shows or hides the night view of the earth.
fixed it.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.cpp, line 842
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32203#file32203line842>
> >
> > Shows or hides the zenith location of the sun.
fixed it.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.cpp, line 826
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32203#file32203line826>
> >
> > Shows or hides the shadow of the sun.
fixed it.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp, line 69
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32176#file32176line69>
> >
> > also: m_controlTimeAct( 0 ), m_clockLabel( 0 )
m_controlTimeAct is getting initialized in MainWindow::createActions() before it is used any where else.
m_clockLabel is getting initialized in MainWindow::setupStatusBar() before it is used any where else.
Thus, we don't need to initialize them in constructor initializer list.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp, line 826
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32176#file32176line826>
> >
> > Possibly old code that can go away? Not defined in the kcfg or used in the code. Does the same as the previous one as well.
Its was old code. Removed it.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h, line 39
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32183#file32183line39>
> >
> > toJulianDayNumber()? Avoid abbreviations whenever possible.
fixed it.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h, line 60
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32183#file32183line60>
> >
> > what effect does the speed have? Is it an acceleration factor (see above)?
The speed of timer is how fast the marble clock can run relative to actual speed of time. Value of speed may varies form -10x to 10x ( 10x means)
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.cpp, line 1363
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32203#file32203line1363>
> >
> > The capitalization of uTC looks odd.
uTC() is a KConfig generated function.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.cpp, line 839
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32203#file32203line839>
> >
> > Better copy the icon and rename it
The oxygen theme contains icons of different sizes ranging from 16x16 to 64x64 pixels. How will be renamed icon will be added to oxygen theme?
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/QtMarbleConfigDialog.cpp, line 315
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32190#file32190line315>
> >
> > Maybe customTimezone() instead of chooseTimezone()?
renamed function to customTimezone()
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/marble.kcfg, line 54
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32201#file32201line54>
> >
> > what about naming it startupDateTime?
dateTime suites better because it is the dateTime of last session and startup prefix is not added to any other entries.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp, line 1084
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32187#file32187line1084>
> >
> > dateTime()
fixed it.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h, line 72
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32183#file32183line72>
> >
> > As a difference, i guess? To which timezone? UTC? Maybe setTimezoneDifference() and timezoneDifference() would be better method names
> >
Yes, it is difference to UTC. I agree that setTimezoneDifference() and timezoneDifference() are more specific names but generally we talk the timezone w.r.t. UTC so setTimezone() and timezone() are more reable.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h, line 43
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32183#file32183line43>
> >
> > doxygen please - when does it get emitted? every second? when i call setDataTime?
timeChanged() signal is called every minute( according to marble clock ) and when setDateTime() is called.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp, line 815
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32176#file32176line815>
> >
> > looks a bit odd: why use three booleans here? I'd rather expect one key/value like mode/{system,lastSession,lastSetting}
> > Or one boolean since only system and lastSession seem to be the possible values
Kconfig is used to save value for system time and lastSession time options, thus I chose to used separate booleans. It is possible to save this with one boolean but I find the two booleans way more reable.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleClock.cpp, line 41
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32184#file32184line41>
> >
> > if ( year < 0 ) {
> > ++year;
> > }
> >
> > I'm not sure why this is done, maybe add a comment?
> >
David Robert is author of year0(), dayFraction() functions, so I have no idea about these.
> On 2010-08-04 10:52:09, Dennis Nienhüser wrote:
> > /trunk/KDE/kdeedu/marble/src/marble_part.cpp, line 880
> > <http://reviewboard.kde.org/r/4819/diff/2/?file=32203#file32203line880>
> >
> > I still doubt reverting this fixes the crash. Can you revert this reversion (just the two lines) and check whether that triggers the crash again? Then I'm convinced ;-)
> >
This time after reverting this two lines, my marble didn't crashed :)
But I still don't know why it was crashing last time because of these two lines :(
- hjain
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4819/#review6789
-----------------------------------------------------------
On 2010-08-03 11:13:32, hjain wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4819/
> -----------------------------------------------------------
>
> (Updated 2010-08-03 11:13:32)
>
>
> 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/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/20100804/9c2b3660/attachment-0001.htm
More information about the Marble-devel
mailing list