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

[Dennis Nienhüser]

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


Good work, works fine here :-)

I see two general issues with the approach:
- we have download logic in the data/ classes. This can become a nasty surprise, so we have to check if it creates problems in some use cases
- the download happens asynchronously (fine), but there's no signal to alert about a download finished, and no reaction to it. So the first user of the style icon gets a null icon and uses it as long as it is reloaded by chance.
Let's ignore both issues for this review, however.


src/lib/marble/geodata/data/GeoDataIconStyle.cpp
<https://git.reviewboard.kde.org/r/116525/#comment36515>

    mDebug() instead of qDebug() and a bit more verbose:
    mDebug() << "Unable to open style icon at" << d->m_iconPath;
    
    Please check the indentation of this part as well, it does not fit with the rest.
    



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

    duplicated #include



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

    Only the constructor and this method should be in the public section, the other methods private.



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

    Please use 0 instead of NULL everywhere



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

    d must be deleted in the destructor



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

    Can be shortened to
    return d->m_iconCache.contains( QUrl( url ) );
    



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

    Qt API style suggests not to use a "get" prefix for methods, please rename to cachedIcon()
    



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

    can be shortened to
    return d->m_iconCache.value( QUrl( url );
    
    If the cache does not contain the url, QHash returns a default constructed value, i.e. a QImage().
    



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

    given that it modifies the instance state and (potentially) initiates a download, i'd rename the method to open() or load()
    



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

    Please use a subdirectory like "/cache/icons/") instead of "/" directly. Also make sure the directory exists and try to create it if needed. See http://qt-project.org/doc/qt-4.8/qdir.html#mkpath
    



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

    Why is it needed to delete the old loader? I'd like to avoid that.
    



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

    I'd overwrite existing icons in any case, but check that the new icon is not null



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

    The m_canRequest logic seems too limiting to me. HttpDownloadManager should handle multiple downloads fine, and queue things as needed. If it doesn't check for multiple requests to the same URL in the queue, I'd rather put a QImage() placeholder into m_iconCache.



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

    getFileName => cacheFileName()


- Dennis Nienhüser


On March 2, 2014, 1:37 a.m., Abhinav Gangwar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116525/
> -----------------------------------------------------------
> 
> (Updated March 2, 2014, 1:37 a.m.)
> 
> 
> Review request for Marble.
> 
> 
> 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/geodata/CMakeLists.txt 69edb65 
>   src/lib/marble/geodata/data/GeoDataIconStyle.cpp b8310cf 
>   src/lib/marble/geodata/data/RemoteIconLoader.h PRE-CREATION 
>   src/lib/marble/geodata/data/RemoteIconLoader.cpp PRE-CREATION 
>   src/lib/marble/CMakeLists.txt bb6f312 
> 
> 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/20140302/19abf5b0/attachment-0001.html>