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

Tue Aug 13 21:03:08 UTC 2013

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

Second half of the review.

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

    can we have an optional QWidget* parent argument for the constructor which gets passed to QDialog? Havent a parent is useful e.g. to avoid duplicate taskbar entries

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

    who deletes d?

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

    QTimer::singleShot( 1000, d->progressBar, SLOT(hide()) );
    should work as well (to avoid the hideListDownloadProgressbar SLOT)

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

    Shouldn't this be called updateNoRouteLabel?

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

    const?

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

    const?

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

    const?

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

    const?

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

    const bool & => bool

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

    const bool & => bool

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

    const?

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

    const?

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

    I think this is right and results in m_routeSyncManager = 0, but I'd rather have it explicit, i.e. m_routeSyncManager( 0 )

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

    true (when changing default in settings)

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

    who deletes d?

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

    What about calling it setRoutingManager()? Maybe also delete d->m_routeSyncManager before creating the new one

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

    const QUrl &

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

    const QVector<RouteItem> &

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

    const QString & (both)

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

    no parameter => not explicit

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

    no need for a pointer here

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

    no need for a pointer here as well

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

    delete d here. Also m_cacheDir and m_network unless you go for the non-pointer version, see comment above

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

    Please use << operator instead of + here

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

    if you save as jpg, please indicate that both in the filename suffix and the mime type above

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

    Please delete mapWidget here

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

    + => <<

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

    Newlines missing. QList also has a removeLast() method.

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

    Imho it should be a timestamp type (e.g. QDateTime) or have a different name like identifier

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

    QString sounds wrong as type. Double probably, mentioning the unit (meter?)

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

    QString sounds wrong as type. Double probably, with a comment mentioning the unit (seconds?)

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

    this shouldn't be commented?

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

    Remove if empty

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

    no parameter => not explicit

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

    const QVector &

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

    const QString &

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

    The code duplication with OwncloudSyncBackend::routeName is a refactoring candidate

src/lib/routing/RoutingWidget.cpp
<http://git.reviewboard.kde.org/r/112035/#comment27925>

    why a timer here?

src/lib/routing/RoutingWidget.cpp
<http://git.reviewboard.kde.org/r/112035/#comment27926>

    shouldn't that connection be done by the manager itself?

- 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/56d8aeed/attachment-0001.html>