[Marble-devel] Review Request: Implement what the KML spec says on placemarks regarding MultiGeometry and icons

[Guillaume Martres]

> On Sept. 15, 2011, 4:48 p.m., Torsten Rahn wrote:
> > src/lib/geodata/data/GeoDataPlacemark.h, line 90
> > <http://git.reviewboard.kde.org/r/102591/diff/1/?file=35971#file35971line90>
> >
> >     Hm, this looks strange to me and smells of the boolean trap. Why not have a separate
> >     
> >     bool hasIconAtCoordinates() const
> >     
> >     method?

It's not really the same as the "boolean trap" since the boolean here is an output parameter, lots of Qt methods take a "bool *ok" parameter. I could make a hasIconAtCoordinates() method, but it would result in a lot of code duplication with coordinate(). The right way would be for the default GeoDataCoordinates constructor to return an invalid GeoDataCoordinates, and to add an isValid() method. But doing that without breaking stuff seems tricky.


> On Sept. 15, 2011, 4:48 p.m., Torsten Rahn wrote:
> > src/lib/geodata/data/GeoDataPlacemark.cpp, line 97
> > <http://git.reviewboard.kde.org/r/102591/diff/1/?file=35972#file35972line97>
> >
> >     btw: we usually prefer preincrement to postincrement whenever it's meant. In case of iterators this good practice can even prevent that you accidently create copies of objects (which might only happen if someone modifies your code later on without thought ...).

Sure, I can change that.


- Guillaume


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102591/#review6539
-----------------------------------------------------------


On Sept. 12, 2011, 6:03 p.m., Guillaume Martres wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102591/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2011, 6:03 p.m.)
> 
> 
> Review request for Marble and Thibaut Gridel.
> 
> 
> Summary
> -------
> 
> Implement what the KML spec says on placemarks regarding MultiGeometry and icons
> 
> From https://code.google.com/apis/kml/documentation/kmlreference.html#placemark
> "To give the user something to click in the 3D viewer, you would need to
> create a MultiGeometry object that contains both a Point and the other Geometry
> object."
> 
> 
> Diffs
> -----
> 
>   src/lib/geodata/data/GeoDataPlacemark.h 46842d7 
>   src/lib/geodata/data/GeoDataPlacemark.cpp 14634ed 
>   src/lib/layers/PlacemarkLayout.h bc2ceda 
>   src/lib/layers/PlacemarkLayout.cpp 34d7b7f 
> 
> Diff: http://git.reviewboard.kde.org/r/102591/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Guillaume
> 
>

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