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

Torsten Rahn rahn at kde.org
Sun Aug 15 12:44:19 CEST 2010 

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



/trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h
<http://reviewboard.kde.org/r/4988/#comment7136>

    What is the difference compared to timeChanged()?  
    Please add some documentation.



/trunk/KDE/kdeedu/marble/src/lib/MarbleClock.h
<http://reviewboard.kde.org/r/4988/#comment7137>

    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.



/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h
<http://reviewboard.kde.org/r/4988/#comment7130>

    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&"



/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h
<http://reviewboard.kde.org/r/4988/#comment7131>

    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&.



/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h
<http://reviewboard.kde.org/r/4988/#comment7132>

    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.
    
    



/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h
<http://reviewboard.kde.org/r/4988/#comment7133>

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



/trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h
<http://reviewboard.kde.org/r/4988/#comment7134>

    What is this going to add to the model? Could you add some documentation?



/trunk/KDE/kdeedu/marble/src/lib/StackedTileLoader.cpp
<http://reviewboard.kde.org/r/4988/#comment7128>

    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?
    
    



/trunk/KDE/kdeedu/marble/src/lib/StackedTileLoader.cpp
<http://reviewboard.kde.org/r/4988/#comment7127>

    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?



/trunk/KDE/kdeedu/marble/src/lib/StackedTileLoader.cpp
<http://reviewboard.kde.org/r/4988/#comment7126>

    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?



/trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneMap.cpp
<http://reviewboard.kde.org/r/4988/#comment7125>

    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 ...
    


- Torsten


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/20100815/1b1492bd/attachment-0001.htm

More information about the Marble-devel mailing list