[Marble-devel] Review Request: Navigation Mode: Recentering, AutoZooming, showing routing instructions and map download for offline usage

Thu Jul 22 00:49:00 CEST 2010

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

Hello,

The code is quite well thought and the comments are mostly nitpicks.

The AdjustNavigation seems a bit complicated still and sounds a bit experimental with lots of magic numbers.
There are some issues with that many slots defined, whith some could be acting on the map at the same time, maybe this should be the first shot of rework.

I had mixed feelings in the most complicated case of recenter when required coupled with autozoom, which makes me think some simplification should happen so all the actions on the map are well understood.

/trunk/KDE/kdeedu/marble/src/lib/CurrentLocationWidget.ui
<http://reviewboard.kde.org/r/4728/#comment6406>

    This string is too wide, could it be "When required", given the context and labels around?

    I think this would convey the same information.

/trunk/KDE/kdeedu/marble/src/lib/MarbleControlBox.h
<http://reviewboard.kde.org/r/4728/#comment6407>

    The variable could have a more explicit name, say centerMode  or something which reminds of the enum in disguise.

/trunk/KDE/kdeedu/marble/src/lib/MarbleControlBox.h
<http://reviewboard.kde.org/r/4728/#comment6408>

    ditto for variable name.

/trunk/KDE/kdeedu/marble/src/lib/MarbleControlBox.cpp
<http://reviewboard.kde.org/r/4728/#comment6411>

    Is it desirable if other code than the RoutingWidget need to access the RoutingManager that it belongs in the MarbleModel, and that the setRoutingManager is done in the RoutingWidget with the manager in the model instead?

/trunk/KDE/kdeedu/marble/src/lib/MarbleControlBox.cpp
<http://reviewboard.kde.org/r/4728/#comment6409>

    seems a bit unfortunate to create the adjustNavigation in the middle of the routing stuff (see line below).

/trunk/KDE/kdeedu/marble/src/lib/MarbleControlBox.cpp
<http://reviewboard.kde.org/r/4728/#comment6410>

    The connection code is a bit unfortunate.
    The intent should be to provide the recenter mode to the AdjustNavigation class.
    You are basically doing connect/disconnect as a way to pass this configuration value.

/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLineString.h
<http://reviewboard.kde.org/r/4728/#comment6412>

    seems a merge mismatch or?
    latLonAltBox has been virtualised recently so that all GeoDataGeometries could provide their box.

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

    from Provider

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

    Do all those really need to be slots?
    setAutoZoom is called explicitely iirc, and I guess you would want to have only one setPosition slot and act according to the mode which you would have set rather than do connects.

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

    private Q_SLOTS even?

/trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp
<http://reviewboard.kde.org/r/4728/#comment6417>

    I think you want to call m_widget->centerOn, this would spare one function call ;)

/trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp
<http://reviewboard.kde.org/r/4728/#comment6418>

    Would it be possible to do those calculus with a QRectF and moveCenter, setWidth and contains ?
     This could be more readable and maybe shorter code too...

/trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp
<http://reviewboard.kde.org/r/4728/#comment6427>

    There is a concurrency for reacting to position change if both one of the recenter slots and the autozoom feature are activated.
    You cannot determine the order for them to manipulate zoom and recenter, and this is bad imho...

    I think you definitely need one single slot to receive position updates, and depending on settings call such or such and such.

    I observed strange behaviour of zooming and centering where the settings were moving the current position out of the map, which seems contrary to the goal of keeping the position visible...

/trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp
<http://reviewboard.kde.org/r/4728/#comment6420>

    err, wouldn't you mean >= ??

/trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp
<http://reviewboard.kde.org/r/4728/#comment6419>

    You are defining and setting the values of an enum variable for the sole purpose of doing a switch afterwards... This makes the same test twice for not much value...

/trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp
<http://reviewboard.kde.org/r/4728/#comment6421>

    What happens for the specific 90, 180, 270 and 0 or 360 case?
    Your switch is not handling a None case...

    I truely recommend dropping the enum and defining behaviour for said values.

/trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp
<http://reviewboard.kde.org/r/4728/#comment6424>

    any reason against not using QPointF for those 3 points??

/trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp
<http://reviewboard.kde.org/r/4728/#comment6423>

    The code would be a lot more understandable if First and Second were replaced with, say crossVertical and crossHorizontal, which I guess is what you are trying to do?

/trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp
<http://reviewboard.kde.org/r/4728/#comment6422>

    either add a comment that this deals with the non-visible case, or maybe reverse the test, do the move close to the test and a return after. That would free you from this biig if bracket.

/trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp
<http://reviewboard.kde.org/r/4728/#comment6426>

    0 speed leads to a fixed zoom value,
    couldn't the zoom be unchanged if speed gets null?

/trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp
<http://reviewboard.kde.org/r/4728/#comment6425>

    Those values are all constants, like magic numbers...
    Could they first be their actual value?
    How were they decided/What to they achieve?

- Thibaut

On 2010-07-21 04:57:08, Siddharth Srivastava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4728/
> -----------------------------------------------------------
> 
> (Updated 2010-07-21 04:57:08)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> Added "Auto Recentering and Auto Zooming" support for navigation mode. Auto Recentering has two options: 
> (i) The current gps location is always at the center of the map (ii)The gps location is moved to the center of the map when required according to the custom area predefined according to the size of the map.
> 
> "Current Routing Instructions" are shown to the user while navigating through the route calculated. A plugin is added to show the time and distance remaining as well as the total distance covered on the route. When an instruction is selected the portion of the route where the instruction is valid gets highlighted. The instruction also appears a certain time before a user reaches it.
> 
> "Downloading of maps for offline mode" is implemented. For calculating the region to be downloaded an offset(in meters) around the calculated route and the required tile level ranges to be downloaded need to be specified. A default offset is used in case user doesn't specify any offset.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/ControlView.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/QtMainWindow.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/CurrentLocationWidget.ui 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/DownloadRegionDialog.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/DownloadRegionDialog.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/LatLonBoxWidget.ui 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleControlBox.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleControlBox.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleDataFacade.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleDataFacade.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleMath.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/PositionProviderPlugin.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/PositionProviderPlugin.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/TileCoordsPyramid.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/TileCoordsPyramid.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLineString.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataLineString.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/global.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/gps/PositionTracking.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/gps/PositionTracking.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/graphicsview/MarbleGraphicsItem.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/AdjustNavigation.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingLayer.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingManager.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingModel.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingModel.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingWidget.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/lib/routing/RoutingWidget.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/plugins/positionprovider/gpsd/GpsdPositionProviderPlugin.h 1151760 
>   /trunk/KDE/kdeedu/marble/src/plugins/positionprovider/gpsd/GpsdPositionProviderPlugin.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/CMakeLists.txt 1151760 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/navigation/NavigationFloatItem.cpp 1151760 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/routing/CMakeLists.txt PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingItemWidget.ui PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/routing/RoutingPlugin.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/tests/TestGeoData.cpp 1151760 
> 
> Diff: http://reviewboard.kde.org/r/4728/diff
> 
> 
> Testing
> -------
> 
> "Auto Recentering and Auto Zooming"- testing done on various gps tracks available. All possible combinations of Auto Zoom and the two modes of auto recentering were tested.
> "Current Routing Instructions" - testing done on various routes calculated using osmprovider as well as yoursnavigation provider and the gps tracks. Testing also done with and without plugin.
> "Downloading of maps for offline mode"- testing done for various routes around the world using orsprovider and yoursnavigation provider.
> 
> 
> Thanks,
> 
> Siddharth
> 
>

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