[Marble-devel] Review Request: Code cleanup: Move some static methods from TileLoaderHelper directly to GeoSceneTexture and use TileId as parameter.

[Bernhard Beschow]

> On 2010-04-13 16:42:00, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneTexture.h, line 84
> > <http://reviewboard.kde.org/r/3588/diff/1/?file=23485#file23485line84>
> >
> >     I wonder if we should leave a “simple” getter here as we had before.

IMO, we shouldn't keep it since it is dead code.


> On 2010-04-13 16:42:00, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneTexture.h, line 89
> > <http://reviewboard.kde.org/r/3588/diff/1/?file=23485#file23485line89>
> >
> >     Please add the documentation here.
> >     Perhaps we should change the function name to “themeString” so we don't have an abbreviation here.

Hmm, I don't see any real difference between themeStr() and sourceDir(). Perhaps they can be merged? But that should be done by somebody who knows the code very good.


> On 2010-04-13 16:42:00, Bastian Holst wrote:
> > trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneTexture.h, line 81
> > <http://reviewboard.kde.org/r/3588/diff/1/?file=23485#file23485line81>
> >
> >     This comment doesn't seem very reasonable here now.

I'd expect this method to be const in the first place. The comment justifies why it is not. Of course, it doesn't look like a getter any more...


- Bernhard


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


On 2010-04-13 14:59:45, Bernhard Beschow wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3588/
> -----------------------------------------------------------
> 
> (Updated 2010-04-13 14:59:45)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> This patch represents milestone 1 of a series of patches to add more (server-side) storage layouts. In this series of patches, the GeoSceneTexture::StorageLayoutMode enum will be replaced by an abstract class, allowing for easy addition of server-side storage layouts. See [1] for all patches.
> 
> [1] http://github.com/shentok/marble/commits/custom-storage
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdeedu/marble/src/lib/StackedTileLoader.cpp 1114412 
>   trunk/KDE/kdeedu/marble/src/lib/TileLoader.cpp 1114412 
>   trunk/KDE/kdeedu/marble/src/lib/TileLoaderHelper.h 1114412 
>   trunk/KDE/kdeedu/marble/src/lib/TileLoaderHelper.cpp 1114412 
>   trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneTexture.h 1114412 
>   trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneTexture.cpp 1114412 
> 
> Diff: http://reviewboard.kde.org/r/3588/diff
> 
> 
> Testing
> -------
> 
> Works for me, please test.
> 
> 
> Thanks,
> 
> Bernhard
> 
>