[Marble-devel] Review Request: Dgml Time tag handler

Torsten Rahn rahn at kde.org
Tue Aug 17 12:57:47 CEST 2010 


> 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 :)
> 
> hjain wrote:
>     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 :)
>     
>

No. setTextureMapper doesn't "map" the projection. And setMapTheme() also doesn't have the intention of mapping. 
What setMapTheme does is it sets the map theme to a new map theme. While doing so it might initialize a new texturemapper (via setupTextureMapper). This means that the texturemapper is getting fully initialized (slow long loud process that wakes up every one, so you want to avoid calling it).

So what you are doing here is _completely_ initializing a new texture mapper object every time the time is set. This doesn't make sense, since you don't create a new texture mapper but just setup the one that was setup already. So why set it up again? :-) That's insanely expensive and obviously is not what you want to do. You just want to trigger that the texturemapper will update its output. Yes, your approach also updates the screen (because d->notifyModelChanged()/modelChanged() is called) after the long process of initialization. But that's just a side product.  

>From what I can see you just want to emit the signal "modelChanged()" instead of calling mapTexture. That's how it would be done properly. 


> 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&.
> 
> hjain wrote:
>     Added documentation to it and made class private.

Even for private classes we require proper naming. Please choose a name that better gets the idea across what is happening here.


> 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.
> >     
> >
> 
> hjain wrote:
>     Added documentation to it and made class private.

Even for private classes we require proper naming. Please choose a name that better gets the idea across what is happening here.


> 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?
> 
> hjain wrote:
>     I think that the previous code was inserting only textureLayers into qhash.

Please ask Jens-Michael (jmho) to review this code :)


> 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&"
> 
> hjain wrote:
>     Made it private and added comment to it. Also changed the name of function to datetimeMatch()

dateTimeMatch still doesn't tell me anything. What does it match?

Even for private classes we require proper naming. Please choose a name that better gets the idea across what is happening here.


- Torsten


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


On 2010-08-17 09:19:43, hjain wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4988/
> -----------------------------------------------------------
> 
> (Updated 2010-08-17 09:19:43)
> 
> 
> 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/492c1596/attachment-0001.htm

More information about the Marble-devel mailing list