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

Mon May 17 22:41:51 CEST 2010

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


/trunk/KDE/kdeedu/marble/data/placemarks/HOWTO-cities.txt
<http://reviewboard.kde.org/r/4006/#comment5356>

    Ew, you are completely right here. This is an important oversight.


/trunk/KDE/kdeedu/marble/src/lib/FileLoader.cpp
<http://reviewboard.kde.org/r/4006/#comment5357>

    Hm. No. Why?


/trunk/KDE/kdeedu/marble/src/lib/FileLoader.cpp
<http://reviewboard.kde.org/r/4006/#comment5358>

    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.


/trunk/KDE/kdeedu/marble/src/lib/FileLoader.cpp
<http://reviewboard.kde.org/r/4006/#comment5359>

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


/trunk/KDE/kdeedu/marble/src/lib/FileLoader.cpp
<http://reviewboard.kde.org/r/4006/#comment5360>

    Yes this looks reasonable. Looking at the code above it looks like saveFile() always takes just the filename. Looks good. :-)


/trunk/KDE/kdeedu/marble/src/lib/MarblePlacemarkModel.h
<http://reviewboard.kde.org/r/4006/#comment5362>

    Why do you need an ASCII name role? This sounds really odd :-)


/trunk/KDE/kdeedu/marble/src/lib/PlacemarkInfoDialog.cpp
<http://reviewboard.kde.org/r/4006/#comment5361>

    Just mDebug needed ...


/trunk/KDE/kdeedu/marble/src/lib/PlacemarkInfoDialog.cpp
<http://reviewboard.kde.org/r/4006/#comment5363>

    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?


/trunk/KDE/kdeedu/marble/src/lib/PlacemarkInfoDialog.cpp
<http://reviewboard.kde.org/r/4006/#comment5364>

    I don't understand the motivation behind this code. Maybe you wanted to provide a latin1-encoding representation for the location name? I don't know. But this code doesn't make sense to me so it either needs some explanation or it needs to get fixed :-)


/trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataPlacemark.h
<http://reviewboard.kde.org/r/4006/#comment5365>

    Again. What do we need the ascii name for? :-)
    
    We need some representations of a name:
    
    1) the english name in latin1 encoding. (e.g. "Moscow")
    2) the native representation in Utf8: ??????
    
    Those two are the most basic imho. The 1) could be used as a general reference to the name that can be easily sorted and compared. The second one would be there to have some represenation that works "untranslated" in each country.
    
    In the future we could then add 
    
    3) the translated version in utf8. For German this would be "Moskau" for the english name "Moscow".
    
    Once we have tackled 1, 2 and 3 we could think about storing alternative names as well (like Bombay in addition to Mumbai).
    
    Since this is a lot of data I think this stuff should get stored in the placemark's ExtendedData section.


/trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlAsciiNameTagHandler.h
<http://reviewboard.kde.org/r/4006/#comment5366>

    Please copy the license header from other Marble files. For licensing consistency is important. :-)


/trunk/KDE/kdeedu/marble/tools/asc2kml/asc2kml.cpp
<http://reviewboard.kde.org/r/4006/#comment5368>

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


/trunk/KDE/kdeedu/marble/tools/asc2kml/asc2kml.cpp
<http://reviewboard.kde.org/r/4006/#comment5367>

    Oh now maybe I understand how you got the asciiname idea. Well actually the asc2kml is more of a latin12kml converter ;-)


- Torsten


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