[Marble-devel] Review Request 124622: Basic routing in Marble Maps

Wed Aug 5 14:57:30 UTC 2015

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124622/#review83454
-----------------------------------------------------------


src/apps/marble-maps/CircularButton.qml (line 11)
<https://git.reviewboard.kde.org/r/124622/#comment57690>

    is this really used somewhere? If not I wouldn't add it.


src/apps/marble-maps/MainScreen.qml (line 22)
<https://git.reviewboard.kde.org/r/124622/#comment57691>

    I think we should follow KDE HIG also in QML as much as possible. For menu items it suggests using title capitalization, see https://techbase.kde.org/Projects/Usability/HIG/Capitalization
    
    Therefore
    "Delete route" => "Delete Route"
    "Navigation instructions" => "Navigation Instructions"
    "Show the map" => "Show the Map"


src/apps/marble-maps/MainScreen.qml (line 107)
<https://git.reviewboard.kde.org/r/124622/#comment57688>

    This is equivalent to setting
    
    RoutePlanViewer {
      id: ...
      model: routing.routingModel
    }
    below.
    
    If you tried this before and got a crash there, this happens when Routing::routingModel() does not check whether it has the map set already (see comment there).


src/apps/marble-maps/MainScreen.qml (line 127)
<https://git.reviewboard.kde.org/r/124622/#comment57675>

    This ends up quite much towards the screen middle e.g. on my Nexus 4. For button sizes using millimeter is the way to go, but for margins I'd use absolute pixel values or something relative to the width of the parent to achieve a layout that works fine across all devices.


src/apps/marble-maps/MainScreen.qml (line 181)
<https://git.reviewboard.kde.org/r/124622/#comment57692>

    departureSetted => departureEntered or hasDeparture,
    destinationSetted => destinationEntered or hasDestination


src/lib/marble/declarative/Coordinate.h (line 35)
<https://git.reviewboard.kde.org/r/124622/#comment57676>

    Please remove


src/lib/marble/declarative/Coordinate.h (line 36)
<https://git.reviewboard.kde.org/r/124622/#comment57677>

    QObject and classes derived from it have private copy constructors and private assignment operators. This shouldn't be changed, please remove this one.


src/lib/marble/declarative/Coordinate.h (line 37)
<https://git.reviewboard.kde.org/r/124622/#comment57678>

    not needed, please remove


src/lib/marble/declarative/Coordinate.h (line 39)
<https://git.reviewboard.kde.org/r/124622/#comment57679>

    please remove


src/lib/marble/declarative/MarbleQuickItem.h (line 54)
<https://git.reviewboard.kde.org/r/124622/#comment57680>

    please remove, see http://quickgit.kde.org/?p=marble.git&a=commit&h=8f201d0


src/lib/marble/declarative/Routing.cpp (line 37)
<https://git.reviewboard.kde.org/r/124622/#comment57681>

    seems unused? please remove


src/lib/marble/declarative/Routing.cpp (line 38)
<https://git.reviewboard.kde.org/r/124622/#comment57682>

    seems unused? please remove


src/lib/marble/declarative/Routing.cpp (line 104)
<https://git.reviewboard.kde.org/r/124622/#comment57683>

    The routing model only changes once, which is here. So this can be replaced with
    `emit routingModelChanged();`
    
    Upon state changes the routing model stays the same and only clears itself. So all further changes to the model are handled internally by Qt without further work on our side.


src/lib/marble/declarative/Routing.cpp (line 150)
<https://git.reviewboard.kde.org/r/124622/#comment57689>

    Must return 0 if d->m_marbleMap is 0 (see waypointModel implementation).


src/lib/marble/routing/Maneuver.cpp (line 23)
<https://git.reviewboard.kde.org/r/124622/#comment57687>

    According to Qt docs, ':/' and 'qrc://' can both be used, so this change should be fine. Unfortunately QPixmap only understands ':/' and fails to load 'qrc://' however. Therefore with this change the Desktop applications do not display turn type icons anymore.
    
    To work around it, I'd suggest leaving Maneuver.cpp unchanged and using
    `source: turnTypeIcon.replace(':/', 'qrc:///')`
    in RoutePlanViewer.qml to change the prefix on the fly. This way it works in both QWidget and QML based apps.


- Dennis Nienhüser


On Aug. 5, 2015, 1:49 p.m., Gábor Péterffy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124622/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2015, 1:49 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> This patch contains a minimalistic ui for routing.
> 
> - Planning route to destination from current position
> - Planning route from setted departure
> - Planning route with waypoints
> - Instructions in the menu
> 
> What is missing:
> - Waypoint markers for the map, these will come in a new patch.
> - Maybe a loading screen while route planning in progress
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/declarative/Routing.h 182fe64 
>   src/lib/marble/declarative/MarbleQuickItem.cpp f8c396f 
>   src/lib/marble/declarative/MarbleQuickItem.h cefb0c0 
>   src/lib/marble/declarative/MarbleDeclarativePlugin.cpp 3bf2198 
>   src/lib/marble/declarative/Coordinate.cpp bbdc829 
>   src/lib/marble/declarative/Coordinate.h db2ab9f 
>   src/apps/marble-maps/Search.qml 9b712c8 
>   src/apps/marble-maps/RoutePlanViewer.qml PRE-CREATION 
>   src/apps/marble-maps/NavigationSetupButton.qml PRE-CREATION 
>   src/apps/marble-maps/NavigationSetup.qml PRE-CREATION 
>   src/apps/marble-maps/MarbleMaps.qrc 0168893 
>   src/apps/marble-maps/MainScreen.qml b8dc7b0 
>   src/apps/marble-maps/CircularButton.qml 93a09c0 
>   data/android/drawable-xxxhdpi/waypoint.png PRE-CREATION 
>   data/android/drawable-xxxhdpi/walk.png PRE-CREATION 
>   data/android/drawable-xxxhdpi/place.png PRE-CREATION 
>   data/android/drawable-xxxhdpi/navigation.png PRE-CREATION 
>   data/android/drawable-xxxhdpi/map.png PRE-CREATION 
>   data/android/drawable-xxxhdpi/car.png PRE-CREATION 
>   data/android/drawable-xxxhdpi/bike.png PRE-CREATION 
>   src/lib/marble/declarative/Routing.cpp dbbcb80 
>   src/lib/marble/declarative/SearchBackend.h 4981c31 
>   src/lib/marble/declarative/SearchBackend.cpp fdef702 
>   src/lib/marble/routing/Maneuver.cpp c0ab724 
> 
> Diff: https://git.reviewboard.kde.org/r/124622/diff/
> 
> 
> Testing
> -------
> 
> It works well on my device, everything fits and has right positioning.
> 
> 
> File Attachments
> ----------------
> 
> bike.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/04/f39163fa-12ab-4aa2-a787-9c43e5d54177__bike.png
> walk.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/04/21576ef7-f5c8-4cb2-b731-63e14bdb51db__walk.png
> place.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/04/373c2a86-625c-4380-b699-0acbdd269a9d__place.png
> navigation.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/04/7adf250e-4a25-4ddd-a1a6-5e9e6d905ee1__navigation.png
> map.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/04/efe967e2-0ae1-4ac0-8678-8b2cea4e32b1__map.png
> car.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/04/9e5f5f87-352c-4e70-b7ae-cdf8be296863__car.png
> waypoint.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/04/2fcdc0b5-6552-4a89-bf55-ecf9b50e1605__waypoint.png
> Screenshot I.
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/04/08bed68d-5ec3-43a1-98ac-467fc8aa94da__Screenshot_2015-08-04-22-39-57.png
> Screenshot II.
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/04/f2cc5de7-f966-496a-bbe1-2b24859511c1__Screenshot_2015-08-04-22-39-52.png
> Screenshot III
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/04/76795bf1-590f-4e61-bb67-1cb10ed6870b__Screenshot_2015-08-04-22-39-24.png
> Screenshot IV
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/04/8f8d1795-d751-4ee9-bec0-eee3404b15b4__Screenshot_2015-08-04-22-39-11.png
> Screenshot V
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/08/04/ba50137d-21ec-4759-b6f4-38cbe882f47e__Screenshot_2015-08-04-22-38-03.png
> 
> 
> Thanks,
> 
> Gábor Péterffy
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20150805/1fe843fd/attachment-0001.html>