[Marble-devel] Review Request: Marble Clock Patch

Wed Aug 4 12:51:56 CEST 2010

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


/trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp
<http://reviewboard.kde.org/r/4819/#comment6618>

    also: m_controlTimeAct( 0 ), m_clockLabel( 0 )


/trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp
<http://reviewboard.kde.org/r/4819/#comment6619>

    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


/trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp
<http://reviewboard.kde.org/r/4819/#comment6620>

    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.


/trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h
<http://reviewboard.kde.org/r/4819/#comment6630>

    toJulianDayNumber()? Avoid abbreviations whenever possible.


/trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h
<http://reviewboard.kde.org/r/4819/#comment6631>

    doxygen missing


/trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h
<http://reviewboard.kde.org/r/4819/#comment6632>

    doxygen please - when does it get emitted? every second? when i call setDataTime?


/trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h
<http://reviewboard.kde.org/r/4819/#comment6633>

    what effect does the speed have? Is it an acceleration factor (see above)?


/trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h
<http://reviewboard.kde.org/r/4819/#comment6634>

    As a difference, i guess? To which timezone? UTC? Maybe setTimezoneDifference() and timezoneDifference() would be better method names
    

/trunk/KDE/kdeedu/marble/src/lib/MarbleClock.cpp
<http://reviewboard.kde.org/r/4819/#comment6635>

    if ( year < 0 ) {
      ++year;
    }
    
    I'm not sure why this is done, maybe add a comment?
    

/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp
<http://reviewboard.kde.org/r/4819/#comment6636>

    dateTime()


/trunk/KDE/kdeedu/marble/src/lib/QtMarbleConfigDialog.cpp
<http://reviewboard.kde.org/r/4819/#comment6637>

    Maybe customTimezone() instead of chooseTimezone()?


/trunk/KDE/kdeedu/marble/src/marble.kcfg
<http://reviewboard.kde.org/r/4819/#comment6622>

    what about naming it startupDateTime?


/trunk/KDE/kdeedu/marble/src/marble_part.cpp
<http://reviewboard.kde.org/r/4819/#comment6624>

    Shows or hides the shadow of the sun.


/trunk/KDE/kdeedu/marble/src/marble_part.cpp
<http://reviewboard.kde.org/r/4819/#comment6626>

    Shows or hides the night view of the earth.


/trunk/KDE/kdeedu/marble/src/marble_part.cpp
<http://reviewboard.kde.org/r/4819/#comment6625>

    Better copy the icon and rename it


/trunk/KDE/kdeedu/marble/src/marble_part.cpp
<http://reviewboard.kde.org/r/4819/#comment6627>

    Shows or hides the zenith location of the sun.


/trunk/KDE/kdeedu/marble/src/marble_part.cpp
<http://reviewboard.kde.org/r/4819/#comment6628>

    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 ;-)
    

/trunk/KDE/kdeedu/marble/src/marble_part.cpp
<http://reviewboard.kde.org/r/4819/#comment6629>

    The capitalization of uTC looks odd.


/trunk/KDE/kdeedu/marble/src/plugins/render/stars/StarsPlugin.cpp
<http://reviewboard.kde.org/r/4819/#comment6638>

    m_marbleWidget( 0 )


/trunk/KDE/kdeedu/marble/src/plugins/render/stars/StarsPlugin.cpp
<http://reviewboard.kde.org/r/4819/#comment6639>

    whitespace in inner parenthesis at the end


/trunk/KDE/kdeedu/marble/src/plugins/render/stars/StarsPlugin.cpp
<http://reviewboard.kde.org/r/4819/#comment6640>

    Please copy weather-clear.png to your own icon


- Dennis


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