[Marble-devel] Review Request: patch for i18n

[hjain.itbhu at gmail.com]

> On 2010-06-10 09:20:50, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFeature.h, line 230
> > <http://reviewboard.kde.org/r/4194/diff/5/?file=28237#file28237line230>
> >
> >     Here you are using a reference for the return type as well. Usually this should be avoided. See:
> >     
> >     http://techbase.kde.org/Policies/Library_Code_Policy#Const_References
> >     
> >     A good reason for a reference would be speed. But I guess for most use cases where speed matters we'd just query an attribute from the ExtendedData stored inside a GeoDataFeature. I guess we just could have a method inside GeoDataFeature which would allow to query the value of a single attribute. This would avoid GeoDataExtendedData copies. So I think we can avoid returning a reference and return by value instead.
> 
> Bastian Holst wrote:
>     Ok valueRef() and value() and addValue() seems to be more reasonable, but not valueRef() and data()

In KmlExtendedDataTagHandler.cpp, we need to return the address of GeoDataExtendedData as GeoNode pointer. Thus, we need to get the reference of GeoDataExtendedData. Passing by value will not be helpful. Thus, I am returning a reference instead of value in extendedData() function. Same is the reason to use valueRef() function.


- hjain


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


On 2010-06-09 12:41:54, hjain wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4194/
> -----------------------------------------------------------
> 
> (Updated 2010-06-09 12:41:54)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> This patch consists of KML handler and geodata classes for ExtendedData where custom data for Marble can be stored. This patch still requires to write code to use this data.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataData.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataData.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataData_p.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataExtendedData.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataExtendedData.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataExtendedData_p.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFeature.h 1135207 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFeature.cpp 1135207 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/data/GeoDataFeature_p.h 1135207 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlDataTagHandler.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlDataTagHandler.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlDisplayNameTagHandler.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlDisplayNameTagHandler.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlElementDictionary.h 1135187 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlElementDictionary.cpp 1135187 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlExtendedDataTagHandler.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlExtendedDataTagHandler.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlValueTagHandler.h PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/kml/KmlValueTagHandler.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/parser/GeoDataTypes.h 1135187 
>   /trunk/KDE/kdeedu/marble/src/lib/geodata/parser/GeoDataTypes.cpp 1135187 
> 
> Diff: http://reviewboard.kde.org/r/4194/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> hjain
> 
>