[Marble-devel] Review Request 118717: Added 'highlight on click' feature.

Dennis Nienhüser earthwings at gentoo.org
Sun Aug 10 18:23:46 UTC 2014 

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



src/lib/marble/GeoGraphicsScene.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44827>

    Please pass a const reference



src/lib/marble/GeoGraphicsScene.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44828>

    foreach?



src/lib/marble/GeoGraphicsScene.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44829>

    const TileId &tileId



src/lib/marble/GeoGraphicsScene.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44830>

    foreach?



src/lib/marble/GeoGraphicsScene.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44831>

    is parent guaranteed to be nonzero?



src/lib/marble/GeoGraphicsScene.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44832>

    Makes no sense here. If parent was 0, parent->nodeType above would crash already. If parent was != 0, static_cast will always be != 0 also.



src/lib/marble/GeoGraphicsScene.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44833>

    this block looks very much like the one above, can you extract a method?



src/lib/marble/MarbleModel.h
<https://git.reviewboard.kde.org/r/118717/#comment44834>

    shouldn't this be called addHighlightStyle?



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44835>

    highlight



src/lib/marble/MarbleModel.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44836>

    what's the intention here?



src/lib/marble/MarbleWidget.h
<https://git.reviewboard.kde.org/r/118717/#comment44837>

    ideally signal names are passive



src/lib/marble/geodata/handlers/dgml/DgmlMapTagHandler.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44838>

    highlightBrushColor



src/lib/marble/graphicsview/GeoGraphicsItem.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44839>

    check is not needed (calling delete 0 is perfectly fine)



src/lib/marble/graphicsview/GeoGraphicsItem_p.h
<https://git.reviewboard.kde.org/r/118717/#comment44840>

    m_highlighted is not initialized



src/lib/marble/layers/GeometryLayer.h
<https://git.reviewboard.kde.org/r/118717/#comment44841>

    ideally a passive name
    parameter should be a const reference



src/lib/marble/layers/GeometryLayer.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44842>

    specify any highlight



src/lib/marble/layers/GeometryLayer.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44843>

    highlight



src/lib/marble/layers/GeometryLayer.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44844>

    specifies



src/lib/marble/layers/GeometryLayer.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44845>

    this is not sufficient. Either check the nodeType (preferably) or use a dynamic_cast



src/lib/marble/layers/GeometryLayer.cpp
<https://git.reviewboard.kde.org/r/118717/#comment44846>

    highlight


- Dennis Nienhüser


On Aug. 10, 2014, 12:39 p.m., Abhinav Gangwar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118717/
> -----------------------------------------------------------
> 
> (Updated Aug. 10, 2014, 12:39 p.m.)
> 
> 
> Review request for Marble, Dennis Nienhüser, Torsten Rahn, and Thibaut Gridel.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> Modifications:
> 
> 1. Added support to specify highlight color in dgml file.
> 2. Highlighting is done in the following way:
> 
> -> Input handler emits mouseClickGeoPostion( qreal lon, qreal lat, GeoDataCoordinates::Unit unit ) whenever a click 
>    is detected on map.
>    
> -> Signal mouseClickGeoPostion(..) is connected to the signal MarbleMap::announceMouseClick(..) which
>    further triggers the slot GeometryLayer::hadnleHighlight(..) which iterates over the tree model to find the        
>    placemarks, from all GeoDataDocuments in which any of its style map has an entry for highlight styleId, which
>    were under mouse when the click event happened.
>      
> -> After searching for all such placemarks GeometryLayer::hadnleHighlight(..) emits signal
>    announceHighlight(QVector<GeoDataPlacemarks*> which is caught by GeoGraphicsScene to execute the slot
>    GeoGraphicsScene::applyHighlight(QVector< GeoDataPlacemark* > selectedPlacemark).
>    
> -> GeoGraphicsScene::applyHighlight(..) we find all GeoGraphicsItems for a plcamark and execute
>    GeoGraphicsItem::setHighlightStyle( GeoDataStyle *highlightStyle ) for each graphics items if the corresponding 
>    placemark's style url is set to a style map which has a entry for highlight styleId.
>    
> -> GeoGraphicsItem::setHighlightStyle(..) assigns this style to GeoGraphicsItemPrivate::m_highlightStyle makes 
>    GeoGraphicsItemPrivate::m_isHighlighted true. m_isHighlight decides which style ( normal or highlight ) to use
>    to paint the graphics item.
> 
> 
> That's it . Need feedback :)
> 
> 
> Diffs
> -----
> 
>   data/maps/earth/political/political.dgml 3bf3b1b 
>   src/lib/marble/GeoGraphicsScene.h 869c3d8 
>   src/lib/marble/GeoGraphicsScene.cpp f84c532 
>   src/lib/marble/MarbleInputHandler.cpp 6a46a0d 
>   src/lib/marble/MarbleMap.h ab51f62 
>   src/lib/marble/MarbleMap.cpp 25007ac 
>   src/lib/marble/MarbleModel.h 9643186 
>   src/lib/marble/MarbleModel.cpp 3bd408c 
>   src/lib/marble/MarbleWidget.h dfc1aa7 
>   src/lib/marble/MarbleWidget.cpp d4c7ff4 
>   src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp b161d06 
>   src/lib/marble/geodata/handlers/dgml/DgmlAttributeDictionary.h cf4dd76 
>   src/lib/marble/geodata/handlers/dgml/DgmlAttributeDictionary.cpp 85f10fb 
>   src/lib/marble/geodata/handlers/dgml/DgmlMapTagHandler.cpp fb7ff0f 
>   src/lib/marble/geodata/scene/GeoSceneMap.h 498bb3a 
>   src/lib/marble/geodata/scene/GeoSceneMap.cpp 7f94c11 
>   src/lib/marble/graphicsview/GeoGraphicsItem.h 653fef0 
>   src/lib/marble/graphicsview/GeoGraphicsItem.cpp 1227316 
>   src/lib/marble/graphicsview/GeoGraphicsItem_p.h 618cddf 
>   src/lib/marble/layers/GeometryLayer.h 8d86aed 
>   src/lib/marble/layers/GeometryLayer.cpp fbfe9a3 
>   src/lib/marble/MarbleInputHandler.h 58107e5 
> 
> Diff: https://git.reviewboard.kde.org/r/118717/diff/
> 
> 
> Testing
> -------
> 
> 
> File Attachments
> ----------------
> 
> pn2DataNew.zip
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/06/13/e541a355-80be-40a8-8158-afea0677bd0e__pn2DataNew.zip
> color10.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/06/13/f04644e8-6318-4b9a-a8d1-2747b405f73b__color10.png
> color11.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/06/13/5f07bac3-3bcb-4086-b8d6-5b836b8f18dc__color11.png
> 
> 
> Thanks,
> 
> Abhinav Gangwar
> 
>

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

More information about the Marble-devel mailing list