[Marble-devel] Review Request: Interface for Small Screen Devices

Dennis Nienhüser earthwings at gentoo.org
Fri Aug 13 22:41:16 CEST 2010 

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



/trunk/KDE/kdeedu/marble/src/lib/gps/PositionTracking.h
<http://reviewboard.kde.org/r/5011/#comment7086>

    I'd prefer not to have this function, it introduces redundancy.



/trunk/KDE/kdeedu/marble/src/lib/gps/PositionTracking.h
<http://reviewboard.kde.org/r/5011/#comment7087>

    I'd rather have 
    
    void positionProviderPluginChanged( PositionProviderPlugin* activePlugin )
    
    which would be emitted by setPositionProviderPlugin(). "Disabled" would result in
     emit positionProviderPluginChanged( 0 );
    
    This avoids the redundant method.
    



/trunk/KDE/kdeedu/marble/src/lib/gps/PositionTracking.cpp
<http://reviewboard.kde.org/r/5011/#comment7088>

    Not needed, see above



/trunk/KDE/kdeedu/marble/src/lib/graphicsview/WidgetGraphicsItem.h
<http://reviewboard.kde.org/r/5011/#comment7089>

    Can you explain this change?



/trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.h
<http://reviewboard.kde.org/r/5011/#comment7091>

    void recenterModeChanged( int mode )



/trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.h
<http://reviewboard.kde.org/r/5011/#comment7090>

    void autoZoomToggled( bool enabled )?



/trunk/KDE/kdeedu/marble/src/lib/routing/RoutingModel.h
<http://reviewboard.kde.org/r/5011/#comment7092>

    Is the doxygen comment still valid wrt the unit (minutes)? Even if it were seconds, a qint32 would be enough to store routes of 68 years, so I don't quite see why a qint64 is needed here.
    
    It may make sense to refactor this for the future to pass a class for the remaining times and similar information. Instead of returning a set of integer etc. you'd just get one class where you can easily access the remaining time to the next instruction, the total remaining time, same for distance, the instruction text and maybe other information.
    



/trunk/KDE/kdeedu/marble/src/lib/routing/RoutingModel.cpp
<http://reviewboard.kde.org/r/5011/#comment7093>

    Don't use c-style casts, please.
    
    qint64 value = (qint64) something;
    
    can be replaced with ctor-like casts:
    
    qint64 value = qint64( something );
    
    Then there's also qRound.



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.h
<http://reviewboard.kde.org/r/5011/#comment7094>

    Doesn't this enum duplicate another one?



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.h
<http://reviewboard.kde.org/r/5011/#comment7097>

    setRecenterMode or updateRecenterMenu



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.h
<http://reviewboard.kde.org/r/5011/#comment7096>

    setAutoZoomEnabled or updateAutoZoomMenu



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.h
<http://reviewboard.kde.org/r/5011/#comment7095>

    disableRecentering



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.h
<http://reviewboard.kde.org/r/5011/#comment7098>

    int zoomValue?



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.h
<http://reviewboard.kde.org/r/5011/#comment7099>

    setXY without a parameter sounds odd. Maybe rename the method.



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp
<http://reviewboard.kde.org/r/5011/#comment7101>

    Why is there whitespace at the start and end of the text? Same for the lines below.



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp
<http://reviewboard.kde.org/r/5011/#comment7102>

    m_whenRequired => m_recenterWhenRequired
    Even better get rid of both booleans and use the enum.



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp
<http://reviewboard.kde.org/r/5011/#comment7103>

    initialize to 0, please. Same for the button below.



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp
<http://reviewboard.kde.org/r/5011/#comment7104>

    no c-style cast, please



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp
<http://reviewboard.kde.org/r/5011/#comment7105>

    no c-style cast, please



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp
<http://reviewboard.kde.org/r/5011/#comment7106>

    'f' would be better. I've already seen some "e^2 Hr..." text appearing...
    
    Thinking on it, you're using an int. The method you're using converts it to a double and then applies the formatting - that's not needed. QString::number( remainingTimeHours ) should be enough.
    
    Same for the calls below



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp
<http://reviewboard.kde.org/r/5011/#comment7108>

    Please see http://doc.trolltech.com/4.6/i18n-source-translation.html#handling-plurals how to handle plural properly



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp
<http://reviewboard.kde.org/r/5011/#comment7110>

    No 'g', 2 parameters here as well.



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp
<http://reviewboard.kde.org/r/5011/#comment7109>

    "Less than a minute."



/trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp
<http://reviewboard.kde.org/r/5011/#comment7111>

    isEmpty()?


- Dennis


On 2010-08-13 16:36:09, Siddharth Srivastava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/5011/
> -----------------------------------------------------------
> 
> (Updated 2010-08-13 16:36:09)
> 
> 
> Review request for marble and Dennis Nienhüser.
> 
> 
> Summary
> -------
> 
> An interface of Routing Plugin for Small Screen Devices is added. The Routing Plugin for Small Screen Device contains options for navigation( recenter, auto zoom), routing information widget, zoom in and zoom out functionalities.
> The routing information widget for desktop version of marble, now shows the next instruction as well as the distance remaining to reach it.
> 
> The instruction point now grows in two levels when the gps location gets closer to it instead of displaying the instruction text at it. 1) The instruction point going to be visited grows in size 2) When the gps device is at certain time away from the instruction point, it grows a bit further.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/lib/CurrentLocationWidget.h 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/CurrentLocationWidget.cpp 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/CurrentLocationWidget.ui 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleControlBox.cpp 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/gps/PositionTracking.h 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/gps/PositionTracking.cpp 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/graphicsview/ScreenGraphicsItem_p.h 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/graphicsview/WidgetGraphicsItem.h 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.h 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingLayer.cpp 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingManager.h 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingManager.cpp 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingModel.h 1162824 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingModel.cpp 1162824 
>   /trunk/KDE/kdeedu/marble/src/marble.qrc 1162824 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/navigation/NavigationFloatItem.cpp 1162824 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/routing/CMakeLists.txt 1162824 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingInformationWidget.ui PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingInformationWidgetSmall.ui PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingItemWidget.ui 1162824 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.h 1162824 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp 1162824 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingWidgetSmall.ui PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/5011/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Routing Information Widget for Desktop version of marble
>   http://reviewboard.kde.org/r/5011/s/478/
> Routing Widget for small screen devices. This screen shot shows the routing information widget which appears on clicking "R" on the routing widget
>   http://reviewboard.kde.org/r/5011/s/479/
> navigation menu for routing widget for small screen devices
>   http://reviewboard.kde.org/r/5011/s/480/
> 
> 
> Thanks,
> 
> Siddharth
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/marble-devel/attachments/20100813/bdf39fae/attachment-0001.htm

More information about the Marble-devel mailing list