> 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 > >
_______________________________________________ Marble-devel mailing list [email protected] https://mail.kde.org/mailman/listinfo/marble-devel
