[Marble-devel] Review Request: Marble: Geonames.org support

[hjain.itbhu at gmail.com]

> On 2010-05-17 20:41:56, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/FileLoader.cpp, line 116
> > <http://reviewboard.kde.org/r/4006/diff/1/?file=26860#file26860line116>
> >
> >     No. The if clause checks whether the file name is absolute. Nothing more and nothing less. Why should this indicate whether there isn't a cache file name available? This sounds wrong.

If clause checks whether the cache file exists or not. 


> On 2010-05-17 20:41:56, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/tools/asc2kml/asc2kml.cpp, line 101
> > <http://reviewboard.kde.org/r/4006/diff/1/?file=26877#file26877line101>
> >
> >     Does geonames.org store the amount of population in thousands of people? Note that it's important here to get the correct numbers: 
> >     Marble is using the population as a means to filter the data for "importance". So if this value is wrong then you get lots and lots of unimportant cities displayed instead of the few big ones (So big cities like Mumbai or Hamburg would be missing on the map ...).

In data from world-gazetteer.com the population was given in unit "kilo inhabitants" while in new data from  geonames.org the population is given in "inhabitants". Thus, I don't think display of unimportant cities is due to change done here.


> On 2010-05-17 20:41:56, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/PlacemarkInfoDialog.cpp, line 72
> > <http://reviewboard.kde.org/r/4006/diff/1/?file=26864#file26864line72>
> >
> >     Ok, using QChar is pretty limited in terms of naming. So using QString would certainly make sense. Have you checked whether this will have a significant impact on performance in some places?

Thank you Torsten :)
This change created the problem of display of unimportant cities. 


> On 2010-05-17 20:41:56, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/FileLoader.cpp, line 120
> > <http://reviewboard.kde.org/r/4006/diff/1/?file=26860#file26860line120>
> >
> >     Ok. So which is the situation where the defaultcachename is not empty? Right. There is no situation like that: The only prior appearance of defaultcachename is its initialization to an empty string. With your code marble will _always_ save the cache file on any startup. This is not the intended behaviour.
> >     Obviously somebody broke the code here. And we need a proper fix which only save the cache file if there is none available.  
> >     
> >     Please try to fully understand the code. Trial-and-Error fixing will only make things worse. :-)

This if will be always true. Thus, the statement under if clause should be brought outside the if clause and if clause should be removed. Removing of if clause will not always save the cache file because this statement is inside the else block of if condition which checks the existence of cache file. 


- hjain


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


On 2010-05-17 14:46:27, hjain wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4006/
> -----------------------------------------------------------
> 
> (Updated 2010-05-17 14:46:27)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> This patch is to use Geonames.org support for city placemarks. This patch also gives kml support for asciiname. Presently Marble is not generating cache for KML file. This issue is also solved.
> 
> Note:- It seems that there are more cities shown by default if you see the earth from far away
> 
> 
> This addresses bugs 232261 and 232450.
>     https://bugs.kde.org/show_bug.cgi?id=232261
>     https://bugs.kde.org/show_bug.cgi?id=232450
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/data/placemarks/HOWTO-cities.txt 1127478 
>   /trunk/KDE/kdeedu/marble/data/placemarks/baseplacemarks.cache UNKNOWN 
>   /trunk/KDE/kdeedu/marble/data/placemarks/boundaryplacemarks.cache UNKNOWN 
>   /trunk/KDE/kdeedu/marble/data/placemarks/cityplacemarks.cache UNKNOWN 
>   /trunk/KDE/kdeedu/marble/data/placemarks/elevplacemarks.cache UNKNOWN 
>   /trunk/KDE/kdeedu/marble/data/placemarks/otherplacemarks.cache UNKNOWN 
>   /trunk/KDE/kdeedu/marble/src/lib/FileLoader.cpp 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleDirs.cpp 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/MarblePlacemarkModel.h 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/MarblePlacemarkModel.cpp 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/PlacemarkInfoDialog.cpp 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/PlacemarkManager.cpp 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFeature.h 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFeature.cpp 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFeature_p.h 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.h 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.cpp 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark_p.h 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlAsciiNameTagHandler.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlAsciiNameTagHandler.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlElementDictionary.h 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlElementDictionary.cpp 1127478 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlRoleTagHandler.cpp 1127478 
>   /trunk/KDE/kdeedu/marble/tools/asc2kml/asc2kml.cpp 1127478 
> 
> Diff: http://reviewboard.kde.org/r/4006/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> hjain
> 
>