[Marble-devel] Review Request 118717: Added 'highlight on click' feature.
Abhinav Gangwar
abhgang at gmail.com
Tue Sep 9 15:44:48 UTC 2014
> On Sept. 8, 2014, 2:10 a.m., Dennis Nienhüser wrote:
> > src/lib/marble/GeoGraphicsScene.h, line 97
> > <https://git.reviewboard.kde.org/r/118717/diff/10/?file=303806#file303806line97>
> >
> > 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.
broken the whole functionality into three method :
1. highlightStyle( const GeoDataDocument*, const GeoDataStyleMap & )
2. selecItem( GeoGraphicsItem * )
3. applyHighlightStyle( GeoGraphicsitem *item, GeoDataStyle *style )
> On Sept. 8, 2014, 2:10 a.m., Dennis Nienhüser wrote:
> > src/lib/marble/MarbleInputHandler.cpp, line 234
> > <https://git.reviewboard.kde.org/r/118717/diff/10/?file=303809#file303809line234>
> >
> > Result is not checked. What happens e.g. for mouse clicks outside the map (stars)?
Thanks for mentioning ! Quite silly of me that I missed it.
> On Sept. 8, 2014, 2:10 a.m., Dennis Nienhüser wrote:
> > src/lib/marble/MarbleModel.cpp, line 395
> > <https://git.reviewboard.kde.org/r/118717/diff/10/?file=303813#file303813line395>
> >
> > Can we put it on the stack instead (i.e. use GeoDataStyle style instead of GeoDataStlye* style)
As I understood from the comment you're saying that make the style a value attribute and avoid the "delete style" statement.
The same style is sent to FileManager::addFile(..) at last, if the file hasn't been parsed already. FileManger uses this style create a FileLoader type object. In FileLoader this style pointer has been used in if statements like "if ( m_style )" to differentiate between a valid and invalid style.
If I change the style, MarbleModel::setMapTheme(...), to a value attribute then I will need something like "isValid()" for style to replace the functionality of what "if ( m_style )" does.
Since this patch is already too big, I avoided any change in FileManager and FileLoader. Furthermore, I don't know whether it's a good idea to change FileManager and FileLoader to hadnle style as a value attribute. IMHO if it's good way to go we can include it another patch.
So, I stayed with original approach of passing assignNewStyle(..) a GeoDataStyle type and deleting it ( in assignNewStyle() itself ) at last to avoid any memory leak.
> On Sept. 8, 2014, 2:10 a.m., Dennis Nienhüser wrote:
> > src/lib/marble/MarbleModel.cpp, line 822
> > <https://git.reviewboard.kde.org/r/118717/diff/10/?file=303813#file303813line822>
> >
> > 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.
Went for only "normal" with no suffix.
> On Sept. 8, 2014, 2:10 a.m., Dennis Nienhüser wrote:
> > src/lib/marble/MarbleWidget.cpp, line 405
> > <https://git.reviewboard.kde.org/r/118717/diff/10/?file=303815#file303815line405>
> >
> > 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.
Used Qt::UniqueConnection flag, in connect method, to avoid any duplicate connection
> On Sept. 8, 2014, 2:10 a.m., Dennis Nienhüser wrote:
> > src/lib/marble/graphicsview/GeoGraphicsItem.cpp, line 93
> > <https://git.reviewboard.kde.org/r/118717/diff/10/?file=303823#file303823line93>
> >
> > 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.
yes, you're right. Now, I'm not messing with m_highlighted in setHighlightStyle() and m_highlightStyle in setHighlighted().
- Abhinav
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118717/#review66011
-----------------------------------------------------------
On Aug. 21, 2014, 6:19 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, 6:19 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/20140909/6f3266bf/attachment-0001.html>
More information about the Marble-devel
mailing list