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

Tue Aug 13 16:10:11 UTC 2013

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

Thanks, looks great. Comments below mostly address page 1 in reviewboard, more later.

src/QtMainWindow.cpp
<http://git.reviewboard.kde.org/r/112035/#comment27855>

    Default value should be false, not ""

src/QtMainWindow.cpp
<http://git.reviewboard.kde.org/r/112035/#comment27863>

    For readability I'd prefer to cache
    CloudSyncManager* cloudSyncManager = m_controlView->marbleWidget()->model()->cloudSyncManager();
    and then work with that in the following lines.

src/QtMainWindow.cpp
<http://git.reviewboard.kde.org/r/112035/#comment27856>

    Default value should be true, not ""

src/QtMainWindow.cpp
<http://git.reviewboard.kde.org/r/112035/#comment27862>

    For readability I'd prefer to cache
    CloudSyncManager* cloudSyncManager = m_controlView->marbleWidget()->model()->cloudSyncManager();
    and then work with that in the following lines.

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

    Can we have a const overload as well?
    const CloudSyncManager* cloudSyncManager() const;

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

    m_cloudSyncManager is not a pointer. What's happening here is that the newly created CloudSyncManager becomes a parent of the also created instance m_cloudSyncManager. Please change to
    m_cloudSyncManager()

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

    "" should be false

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

    "" => true

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

    "" => true

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

    Doxygen comments would be nice (class and methods). Maybe also a note that this is going to be extended soon (it's not an abstract class atm)

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

    const?

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

    const?

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

    const?

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

    for safety i'd call
    return parent.isValid() ? 0 : d->m_items.count();

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

    Please use the safer variant

    beginResetModel();
    d->m_items = items;
    endResetModel();

    See http://doc-snapshot.qt-project.org/4.8/qabstractitemmodel.html#reset

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

    Let's go with bool workOffline() const like the others

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

    setWorkOffline(bool), see above

src/marble.kcfg
<http://git.reviewboard.kde.org/r/112035/#comment27857>

    I'd prefer true by default

src/marble.kcfg
<http://git.reviewboard.kde.org/r/112035/#comment27858>

    I'd prefer true here as well

src/marble_part.h
<http://git.reviewboard.kde.org/r/112035/#comment27859>

    Please remove, it's not used anywhere

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

    For readability I'd prefer to cache
    CloudSyncManager* cloudSyncManager = m_controlView->marbleWidget()->model()->cloudSyncManager();
    and then work with that in the following lines.

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

    "flag" sounds wrong as icon. Please use a non-existing one if we don't have a suitable yet.

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

    For readability I'd prefer to cache
    CloudSyncManager* cloudSyncManager = m_controlView->marbleWidget()->model()->cloudSyncManager();
    and then work with that in the following lines.

- Dennis Nienhüser

On Aug. 12, 2013, 5:56 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. 12, 2013, 5:56 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/20130813/5aaa3e6f/attachment-0001.html>