[Marble-devel] Review Request: Dgml Time tag handler
hjain.itbhu at gmail.com
hjain.itbhu at gmail.com
Tue Aug 17 11:16:57 CEST 2010
> On 2010-08-15 10:44:29, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h, line 93
> > <http://reviewboard.kde.org/r/4988/diff/2/?file=33890#file33890line93>
> >
> > What is the difference compared to timeChanged()?
> > Please add some documentation.
fixed it.
> On 2010-08-15 10:44:29, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h, line 104
> > <http://reviewboard.kde.org/r/4988/diff/2/?file=33890#file33890line104>
> >
> > We usually use "previous" whenever we refer to a "previous" item in a growing list. "last" is just used for the last element in a container that we cycle through.
fixed it.
> On 2010-08-15 10:44:29, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h, line 334
> > <http://reviewboard.kde.org/r/4988/diff/2/?file=33892#file33892line334>
> >
> > First and foremost I'd like to keep the internal implementation concept of texturemapping as much as possible out of the model API.
> >
> > Why do you need this call anyways? Does it really have to be public?
> >
> > There is paintGlobe already. And you should consider just using that. And if you add functionality then please integrate it with existing code instead of doing copy/paste. Thanks :)
setupTextureMapper() maps the projection. In absence of mapTexture(), the texture will be mapped on the projection only by setMapTheme(). Since, we need to change the texture without the theme, we need to connect setupTextureMapper() to timeSet() and dateChanged() signals. But since, setupTextureMapper() requires the projection parameter, I have created mapTexture() which gets the current projection by RTTI and then call setupTextureMapper().
This function don't need to be public. I have fixed it.
paintGlobe is called very frequently. Changing the texture of projection is not required such frequently. Thus, it is better to have a different slot. :)
I don't know how to keep it out of model API :(
Thanks :)
> On 2010-08-15 10:44:29, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h, line 381
> > <http://reviewboard.kde.org/r/4988/diff/2/?file=33892#file33892line381>
> >
> > What is this going to add to the model? Could you add some documentation?
fixed it.
> On 2010-08-15 10:44:29, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h, line 306
> > <http://reviewboard.kde.org/r/4988/diff/2/?file=33892#file33892line306>
> >
> > MarbleModel is a public class. What is "timeMatch" supposed to do? Do you really want this class to be public? If so it needs a better name.
> >
> > Also if you provide QString as a parameter please use "const QString&"
Made it private and added comment to it. Also changed the name of function to datetimeMatch()
> On 2010-08-15 10:44:29, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h, line 308
> > <http://reviewboard.kde.org/r/4988/diff/2/?file=33892#file33892line308>
> >
> > MarbleModel is a public class. This class name doesn't explain properly what this method does related to the model. Is this really supposed to be a public class?
> >
> > Also if you use QString as a parameter please use const QString&.
Added documentation to it and made class private.
> On 2010-08-15 10:44:29, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h, line 310
> > <http://reviewboard.kde.org/r/4988/diff/2/?file=33892#file33892line310>
> >
> > MarbleModel is a public class. What are the methods supposed to do? Is this method really supposed to be public?
> >
> > Also please use "const QString&" instead of QString if you pass it as a parameter.
> >
> >
Added documentation to it and made class private.
> On 2010-08-15 10:44:29, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneMap.cpp, line 132
> > <http://reviewboard.kde.org/r/4988/diff/2/?file=33902#file33902line132>
> >
> > Hm, shouldn't this maybe be rather a flag or a parameter inside GeoSceneLayer * layer(const QString&, LayerType ) ? Otherwise we'll also have to introduce vectorlLayers() etc. at one point and have all different implementations to maintain ...
> >
Changed to QVector<GeoSceneLayer*> GeoSceneMap::layers( const QString& backend ) const
> On 2010-08-15 10:44:29, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/StackedTileLoader.cpp, line 467
> > <http://reviewboard.kde.org/r/4988/diff/2/?file=33894#file33894line467>
> >
> > Ouch, from what I see you are creating a deeply nested set of for loops here. Is this really necessary? Is this the most elegant/efficient way to get through all kinds of cases?
> >
> >
Deleted the innermost loop as it is checking whether the layer is 'texture' or not. If it is then it inserts it into qhash. And In the above loop, we are already taking only texture layers. :)
> On 2010-08-15 10:44:29, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/StackedTileLoader.cpp, line 501
> > <http://reviewboard.kde.org/r/4988/diff/2/?file=33894#file33894line501>
> >
> > Wait! I think this doesn't work!
> >
> > You are replacing code that takes all kinds of layers into account by code that just deals with textureLayers. And what about e.g. vectorLayers? I think they don't get covered by your code. Is there a reason?
same as above
> On 2010-08-15 10:44:29, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/StackedTileLoader.cpp, line 476
> > <http://reviewboard.kde.org/r/4988/diff/2/?file=33894#file33894line476>
> >
> > Wait! I think this doesn't work!
> >
> > You are replacing code that takes all kinds of layers into account by code that just deals with textureLayers. And what about e.g. vectorLayers? I think they don't get covered by your code. Is there a reason?
I think that the previous code was inserting only textureLayers into qhash.
- hjain
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4988/#review7061
-----------------------------------------------------------
On 2010-08-14 22:35:05, hjain wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4988/
> -----------------------------------------------------------
>
> (Updated 2010-08-14 22:35:05)
>
>
> Review request for marble and Bastian Holst.
>
>
> Summary
> -------
>
> This patch merges Temperature(July) and Temperature(Dec) themes into Temperature theme by using dgml time tag. Similar, for Precipitation themes.
> Kindly download the earth folder from http://uploading.com/files/5e9755bd/earth.tar.gz/ and replace it with marble/data/maps/earth folder.
>
>
> Diffs
> -----
>
> /trunk/KDE/kdeedu/marble/data/CMakeLists.txt 1162190
> /trunk/KDE/kdeedu/marble/data/maps/earth/precip-dec/precip-dec-preview.png UNKNOWN
> /trunk/KDE/kdeedu/marble/data/maps/earth/precip-dec/precip-dec.dgml 1162190
> /trunk/KDE/kdeedu/marble/data/maps/earth/precip-jul/precip-jul.jpg UNKNOWN
> /trunk/KDE/kdeedu/marble/data/maps/earth/precip/precip-preview.png UNKNOWN
> /trunk/KDE/kdeedu/marble/data/maps/earth/precip/precip.dgml PRE-CREATION
> /trunk/KDE/kdeedu/marble/data/maps/earth/temp-dec/temp-dec-preview.png UNKNOWN
> /trunk/KDE/kdeedu/marble/data/maps/earth/temp-dec/temp-dec.dgml 1162190
> /trunk/KDE/kdeedu/marble/data/maps/earth/temp-jul/temp-jul.jpg UNKNOWN
> /trunk/KDE/kdeedu/marble/data/maps/earth/temp/temp-preview.png UNKNOWN
> /trunk/KDE/kdeedu/marble/data/maps/earth/temp/temp.dgml PRE-CREATION
> /trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h 1162190
> /trunk/KDE/kdeedu/marble/src/lib/MarbleClock.cpp 1162190
> /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h 1162190
> /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp 1162190
> /trunk/KDE/kdeedu/marble/src/lib/StackedTileLoader.cpp 1162190
> /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/dgml/DgmlElementDictionary.h 1162190
> /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/dgml/DgmlElementDictionary.cpp 1162190
> /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/dgml/DgmlTimeTagHandler.h PRE-CREATION
> /trunk/KDE/kdeedu/marble/src/lib/geodata/handlers/dgml/DgmlTimeTagHandler.cpp PRE-CREATION
> /trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneLayer.h 1162190
> /trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneLayer.cpp 1162190
> /trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneMap.h 1162190
> /trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneMap.cpp 1162190
> /trunk/KDE/kdeedu/marble/src/marble_part.rc 1162190
>
> Diff: http://reviewboard.kde.org/r/4988/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> hjain
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/marble-devel/attachments/20100817/7313b4bb/attachment-0001.htm
More information about the Marble-devel
mailing list