[Marble-devel] Review Request 116525: Added support to download remote images/icons

[Thibaut Gridel]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/116525/#review61123
-----------------------------------------------------------


Hi, sorry for loong delay in reviewing.
You patch looks good, those are some nitpicks for it to be perfect.
Have not tested yet.

Can you close the earlier issues that were raised as it seems you resolved most already?


src/lib/marble/PlacemarkLayout.cpp
<https://git.reviewboard.kde.org/r/116525/#comment42569>

    I think you do not want to connect multiple times, only at mark creation above



src/lib/marble/RemoteIconLoader.h
<https://git.reviewboard.kde.org/r/116525/#comment42571>

    There are private: methods and a RemoteIconLoaderPrivate class, you should target one option



src/lib/marble/RemoteIconLoader.h
<https://git.reviewboard.kde.org/r/116525/#comment42570>

    naming convention, please use an active, not a passive event. eg storeIcon
    
    also if it's not intended for public sight, it should be in the RemoteIconLoaderPrivate



src/lib/marble/RemoteIconLoader.cpp
<https://git.reviewboard.kde.org/r/116525/#comment42572>

    The class is used through a static, and those attributes sound like they should be parameters passing in the methods. Otherwise the code would not be reentrant.
    
    Try adding parameters in eg searchLocalDir and initiateDownload and those attributes should become useless.



src/lib/marble/RemoteIconLoader.cpp
<https://git.reviewboard.kde.org/r/116525/#comment42574>

    Is searchLocalDir different from what GeoDataIconStyle::icon() does using resolvePath ?
    
    Can you precise in comments maybe which one is used for which?



src/lib/marble/VisiblePlacemark.h
<https://git.reviewboard.kde.org/r/116525/#comment42573>

    Not sure this is needed, it can be a local variable in the ctor in order to do the connect()


- Thibaut Gridel


On March 21, 2014, 2:32 a.m., Abhinav Gangwar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116525/
> -----------------------------------------------------------
> 
> (Updated March 21, 2014, 2:32 a.m.)
> 
> 
> Review request for Marble, Bernhard Beschow and Thibaut Gridel.
> 
> 
> Bugs: 310464
>     http://bugs.kde.org/show_bug.cgi?id=310464
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> The patch adds support to download remote images
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/CMakeLists.txt bb6f312 
>   src/lib/marble/PlacemarkLayout.cpp 408607b 
>   src/lib/marble/RemoteIconLoader.h PRE-CREATION 
>   src/lib/marble/RemoteIconLoader.cpp PRE-CREATION 
>   src/lib/marble/VisiblePlacemark.h 879e384 
>   src/lib/marble/VisiblePlacemark.cpp 8811598 
>   src/lib/marble/geodata/data/GeoDataIconStyle.h 9a4abbf 
>   src/lib/marble/geodata/data/GeoDataIconStyle.cpp b8310cf 
> 
> Diff: https://git.reviewboard.kde.org/r/116525/diff/
> 
> 
> Testing
> -------
> 
> Works fine on my system
> 
> 
> Thanks,
> 
> Abhinav Gangwar
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20140628/0a3e8613/attachment.html>