[Marble-devel] Review Request 112035: Route synchronization with ownCloud

Dennis Nienhüser earthwings at gentoo.org
Sat Aug 17 18:22:40 UTC 2013 

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


Thanks, we're getting there :-)


src/lib/CMakeLists.txt
<http://git.reviewboard.kde.org/r/112035/#comment28127>

    Not all headers should be installed to include/marble/, but only those that 3rd party developers should be able to use in their code. In doubt, put no header here for the time being.
    



src/lib/MarbleModel.cpp
<http://git.reviewboard.kde.org/r/112035/#comment28128>

    Where's the implementation of const CloudSyncManager* cloudSyncManager() const?



src/lib/cloudsync/CloudSyncManager.cpp
<http://git.reviewboard.kde.org/r/112035/#comment28129>

    I'd prefer this being renamed m_workOffline as well



src/lib/cloudsync/OwncloudSyncBackend.cpp
<http://git.reviewboard.kde.org/r/112035/#comment28130>

    just m_routeList() is enough



src/lib/cloudsync/OwncloudSyncBackend.cpp
<http://git.reviewboard.kde.org/r/112035/#comment28131>

    return here.



src/lib/cloudsync/OwncloudSyncBackend.cpp
<http://git.reviewboard.kde.org/r/112035/#comment28132>

    return here



src/lib/cloudsync/RouteItem.h
<http://git.reviewboard.kde.org/r/112035/#comment28134>

    The reason for that is that CloudRouteModel works with copies of RouteItem when downloading and assigning preview icons:
    
    RouteItem route = d->m_previewQueue.take( reply );
    
    Only because of the d-pointer sharing assigning a new icon here has an effect on the RouteItem instance in the vector. This is wrong behavior however.
    
    The quickest (in terms of code changes) way to fix it is changing
    
    QMap<QNetworkReply*, RouteItem> m_previewQueue;
    to 
    QMap<QNetworkReply*, RouteItem*> m_previewQueue;
    
    and work with a pointer in CloudRouteModel::setPreview.
    
    In CloudRouteModel::preview you need to change
    
    d->m_previewQueue.insert( reply, d->m_items[index.row()] );
    to
    d->m_previewQueue.insert( reply, &d->m_items[index.row()] );
    
    This isn't the best approach however because it is not safe: Once setItems() would be called again, you'd end up with dangling pointers, so you'd have to clean the queues at least in setItems().
    
    A better approach to fix it would be to change 
    
    QMap<QNetworkReply*, RouteItem> m_previewQueue;
    to
    QMap<QNetworkReply*, QString> m_previewQueue;
    
    where the QString would be the identifier of the RouteItem and you'd locate the instance to work on from d->m_items.
    



src/lib/cloudsync/RouteSyncManager.h
<http://git.reviewboard.kde.org/r/112035/#comment28133>

    const?



src/marble_part.cpp
<http://git.reviewboard.kde.org/r/112035/#comment28126>

    "folder-sync" seems a better choice than "flag". "network-server" would work as well.


- Dennis Nienhüser


On Aug. 16, 2013, 7:54 p.m., Utku Aydın wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112035/
> -----------------------------------------------------------
> 
> (Updated Aug. 16, 2013, 7:54 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Description
> -------
> 
> First review request for "Marble meets ownCloud" Google Summer of Code 2013 project.
> 
> Details:
> - Marble can upload routes to ownCloud, using the API that Marble's very own 3rd party ownCloud application (https://github.com/AndreiDuma/marble-app).
> - Users can save ownCloud server and credentials information using Marble's (and Marble Qt's) configuration interface.
> - This patch also introduces base classes for other synchronization operations (i.e. bookmark sync) which will be implemented in the future.
> 
> Known issues:
> - For now, adding KMLs to local cache manually and uploading them to cloud via Cloud Routes dialog is not supported.
> - Offline mode shows download button after user removes a route from device (cache).
> - Route upload progressbar might appear multiple times.
> 
> If you want to test but don't want to install ownCloud yourself, contact me for server and account details.
> 
> 
> Diffs
> -----
> 
>   src/QtMainWindow.cpp 913d0d4 
>   src/icons/cloud-download.png PRE-CREATION 
>   src/icons/cloud-upload.png PRE-CREATION 
>   src/lib/CMakeLists.txt 88c188a 
>   src/lib/MarbleCloudSyncSettingsWidget.ui PRE-CREATION 
>   src/lib/MarbleModel.h 973e77c 
>   src/lib/MarbleModel.cpp 77e0969 
>   src/lib/MarbleWidget.h 6493a6c 
>   src/lib/QtMarbleConfigDialog.h 9eb3cfc 
>   src/lib/QtMarbleConfigDialog.cpp 47ed016 
>   src/lib/cloudsync/AbstractSyncBackend.h PRE-CREATION 
>   src/lib/cloudsync/AbstractSyncBackend.cpp PRE-CREATION 
>   src/lib/cloudsync/CloudRouteModel.h PRE-CREATION 
>   src/lib/cloudsync/CloudRouteModel.cpp PRE-CREATION 
>   src/lib/cloudsync/CloudRoutesDialog.h PRE-CREATION 
>   src/lib/cloudsync/CloudRoutesDialog.cpp PRE-CREATION 
>   src/lib/cloudsync/CloudRoutesDialog.ui PRE-CREATION 
>   src/lib/cloudsync/CloudSyncManager.h PRE-CREATION 
>   src/lib/cloudsync/CloudSyncManager.cpp PRE-CREATION 
>   src/lib/cloudsync/OwncloudSyncBackend.h PRE-CREATION 
>   src/lib/cloudsync/OwncloudSyncBackend.cpp PRE-CREATION 
>   src/lib/cloudsync/RouteItem.h PRE-CREATION 
>   src/lib/cloudsync/RouteItem.cpp PRE-CREATION 
>   src/lib/cloudsync/RouteItemDelegate.h PRE-CREATION 
>   src/lib/cloudsync/RouteItemDelegate.cpp PRE-CREATION 
>   src/lib/cloudsync/RouteParser.h PRE-CREATION 
>   src/lib/cloudsync/RouteParser.cpp PRE-CREATION 
>   src/lib/cloudsync/RouteSyncManager.h PRE-CREATION 
>   src/lib/cloudsync/RouteSyncManager.cpp PRE-CREATION 
>   src/lib/routing/RoutingWidget.h 9622ea9 
>   src/lib/routing/RoutingWidget.cpp afe1cdd 
>   src/marble.kcfg d3b5505 
>   src/marble.qrc dc7e8ae 
>   src/marble_part.h 1c3dd74 
>   src/marble_part.cpp 6cbb319 
> 
> Diff: http://git.reviewboard.kde.org/r/112035/diff/
> 
> 
> Testing
> -------
> 
> I manually tested every feature on two different servers. There is no unit test available at the moment.
> 
> 
> Thanks,
> 
> Utku Aydın
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20130817/9d3e4d8b/attachment.html>

More information about the Marble-devel mailing list