[Marble-devel] Review Request 124640: Colored icons in WaypointOrderManager in MarbleMaps

[Dennis Nienhüser]

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

Ship it!


The QML changes look fine, but I don't agree with the changes to `RouteRequestModel`: The new property would need a proper role id, not `Qt::UserRole`. Also it shouldn't be a `QString`, but an enum due to the restricted values it can take. And signalling a data change to update the icons feels wrong because it suggests a data change of uninvolved rows. This is easy to fix however by not changing RouteRequestModel and instead mimicking the property directly in the delegate, see comment below.


src/apps/marble-maps/WaypointOrderManager.qml (line 58)
<https://git.reviewboard.kde.org/r/124640/#comment58091>

    Let's implement the property directly in QML:
    
    ```
    diff --git a/src/apps/marble-maps/WaypointOrderManager.qml b/src/apps/marble-maps/WaypointOrderManager.qml
    index 89ef77f..c77a460 100644
    --- a/src/apps/marble-maps/WaypointOrderManager.qml
    +++ b/src/apps/marble-maps/WaypointOrderManager.qml
    @@ -55,7 +55,7 @@ Item {
                             verticalCenter: parent.verticalCenter
                             left: parent.left
                         }
    -                    type: waypointType
    +                    type: index === 0 ? "departure" : (index === waypointList.count-1 ? "destination" : "waypoint")
     
                         Rectangle {
                             color: palette.base
    ```


- Dennis Nienhüser


On Aug. 15, 2015, 1:30 p.m., Gábor Péterffy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124640/
> -----------------------------------------------------------
> 
> (Updated Aug. 15, 2015, 1:30 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> WaypointOrderManager uses the same icons as the markers on the map and they also show the index. It supports the indexes up to 100.
> 
> 
> Diffs
> -----
> 
>   src/apps/marble-maps/WaypointOrderManager.qml f8c136c 
>   src/lib/marble/declarative/RouteRequestModel.cpp f253259 
> 
> Diff: https://git.reviewboard.kde.org/r/124640/diff/
> 
> 
> Testing
> -------
> 
> Works fine on my device.
> 
> 
> Thanks,
> 
> Gábor Péterffy
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20150815/c91c568b/attachment.html>