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

Dennis Nienhüser earthwings at gentoo.org
Sun Sep 7 20:40:31 UTC 2014 

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


Looks good. The comments below mostly address possible code style and readability improvements.


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

    "Selcted Items contain" => "Contains"



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

    For code readability I'd like to refactor this method a bit. Currently it looks like a setter, but actually does three things at the same time, which leads to the somewhat surprising parameters and the need for a return value. Right now it does
    - retrieve the highlight style
    - apply it to the item
    - mark the item as selected
     
    One way around it would be to have two methods:
    - highlightStyle(Document, StyleMap)
    - selectAndHighlight(Item, Style)
     
    or even three when splitting the second one up also.
     
    The method seems to be used only internally, so instead you could also move it to the private section and rename it applyHighlightStyle(). Then have the API docs explain what it does.



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

    please remove



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

    Result is not checked. What happens e.g. for mouse clicks outside the map (stars)?



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

    This API comment could be autogenerated from the method signature, so I'd rather leave it out (if a comment adds no value it's better not to have it at all)



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

    API docs would be nice



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

    Can we put it on the stack instead (i.e. use GeoDataStyle style instead of GeoDataStlye* style)



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

    why use name() and not id()?
    
    Both name() and id() will often be empty. Is that a problem? If not, I'd rather go for plain "normal" with no suffix in any case.



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

    Toggle whether regions are highlighted when users select them



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

    Please name the parameters for those that don't remember our particular order preference for lon/lat vs lat/lon



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

    It should be possible to call this several times in a row with the same parameters, e.g.
    
    setHightlightEnabled(true);
    setHightlightEnabled(true);
    
    Currently this will lead to multiple connections or shell warnings.



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

    + "GeoGraphicsItem takes ownership of the passed style and deletes it when appropriate".



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

    Imho this should be removed as it is an unexpected side-effect and people should insted be forced to call setHighlighted(true) on their own.



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

    Same as above, I would not mess with p()->m_highlightStyle here, instead just save the passed value and do nothing else.



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

    Add parameter names, please.



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

    Add parameter names please. I suppose it passes the now highlighted ones, but it could as well be the previously highlighted ones and the parameter name should resolve that ambiguity.



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

    Please guard with a Q_ASSERT(dynamic_cast...) on d->m_model to be safe from future refactoring
    
    (If we ever were to change d->m_model to be something else than GeoDataTreeModel*, then this would silently crash here some time after calling this method.)
    
    Alternatively change d->m_model to be a GeoDataTreeModel* in the first place.



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

    multigeom => multiGeometry



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

    This relies on a current implementation detail, namely us having GeoDataLineString and GeoDataLinearRing with the former returning false and the latter true for isClosed unconditionally. This would break subtly however once we decided to have a "smart" GeoDataLineString which returns true if first()==last() or introduce another line ring class. So please check the nodeType() instead of calling isClosed()



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

    Please rename multiIter



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

    Subtly overrides the previous end variable in this scope. Please call it multiEnd;



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

    same as above, please check nodeType()



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

    checking for linearRing is redundant after a static_cast: Either lineString is 0 and the check above catches that already, or it will be casted whatsoever.



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

    Is this the right place to put this comment? Shouldn't it be where MarbleMap connects the signal?


- Dennis Nienhüser


On Aug. 21, 2014, 12:49 p.m., Abhinav Gangwar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118717/
> -----------------------------------------------------------
> 
> (Updated Aug. 21, 2014, 12:49 p.m.)
> 
> 
> Review request for Marble, Dennis Nienhüser, Torsten Rahn, and Thibaut Gridel.
> 
> 
> Bugs: 148771
>     http://bugs.kde.org/show_bug.cgi?id=148771
> 
> 
> 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.h 58107e5 
>   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 
> 
> 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/20140907/79b11236/attachment-0001.html>

More information about the Marble-devel mailing list