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

[Torsten Rahn]

> 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?
> 
> hjain wrote:
>     Thank you Torsten :)
>     This change created the problem of display of unimportant cities.

Well, I think that using QString is the right thing. We just need to be careful about performance and feature regressions. So you need to figure out which part that checks the QString leads to the display of the unimportant cities with this change :-)


> 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 ...).
> 
> hjain wrote:
>     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.

Ok. :-)


> 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. :-)
> 
> hjain wrote:
>     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.

No. We first check whether the file name is absolute (because we don't have a method for creating cache names for absolute file names). If the file name is not absolute then we check whether there is a (KML) source file available. And only if this is the case we then check whether there is a cache file available already. If that source file is available and if the cache file is not available we create a new cache file.

So all of these checks are necessary. It's just that the actual check for the availability of the cache file got removed at some point. This resulted in the cache file never getting created (because with no cache file path saveFile was never invoked). Your change only turned the check around resulting in the cache file always being saved. So your change is not a solution. 


- Torsten


-----------------------------------------------------------
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
> 
>