[Marble-devel] Review Request: Refactoring of tile loading code (all in one)

[Jens-Michael Hoffmann]

> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/TileId.cpp, line 39
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=3D19512#file19512line39>
> >
> >     .at(0).toUInt();

The Qt documentation says they are the same, but I can change it of course.


> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/TileId.cpp, line 40
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=3D19512#file19512line40>
> >
> >     .at(1).toUInt();

Same here, and I guess you meant toInt() here?


> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/TileId.cpp, line 42
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=3D19512#file19512line42>
> >
> >     .at(3).toUInt();

same as above


> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/TileId.cpp, line 41
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=3D19512#file19512line41>
> >
> >     .at(2).toUInt();

same as above


> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/AbstractScanlineTextureMapper.cpp, lin=
e 629
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=3D19490#file19490line62=
9>
> >
> >     Shouldn't we also have an
> >     =

> >     Q_ASSERT( m_tileSize.isEmpty() )
> >     =

> >     here? This case would be fatal too and would lead to division by ze=
ro elsewhere ....

Indeed, assuming you meant !m_tileSize.isEmpty() ;)


> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp, line 948
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=3D19501#file19501line94=
8>
> >
> >     There should be a comment I guess that this method will only tempor=
arily "pollute" the MarbleModel class ....

I agree that this should be reworked, also the implementation is a little b=
it hacky as it just uses the groundDataset.

> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/TextureTile.h, line 38
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=3D19508#file19508line38>
> >
> >     I don't like the "SimpleTile vs. Tile" terminology since something =
can be simple in so many ways: SimpleTileLoader sounds e.g. more like a mor=
e convenient tile loader to me. So maybe we should make that clearer by nam=
ing it "Tile vs. CompositTile" or something like that.

As we discussed the class names are now:
SimpleTile -> TextureTile (original TextureTile -> StackedTile)
SimpleTileLoader -> TileLoader (original TileLoader -> StackedTileLoader)


> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MergedLayerDecorator.cpp, line 223
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=3D19505#file19505line22=
3>
> >
> >     Arg, this was surely detaching before ... right?

Yes, it did a copy of the QImage before.


- jmho


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