[Marble-devel] Review Request 112035: Route synchronization with ownCloud
Dennis Nienhüser
earthwings at gentoo.org
Sat Aug 17 18:27:36 UTC 2013
> On Aug. 17, 2013, 6:22 p.m., Dennis Nienhüser wrote:
> > src/lib/cloudsync/RouteItem.h, line 26
> > <http://git.reviewboard.kde.org/r/112035/diff/2/?file=181574#file181574line26>
> >
> > 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.
> >
Thinking further on the first approach (pointers to elements of m_items) let me add that it is very unsafe since every operation on m_items (e.g. appending another item, sorting them, ...) can invalidate all pointers. So let's go for the second approach, or something in that sense.
- Dennis
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112035/#review38039
-----------------------------------------------------------
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/f4ebabc6/attachment-0001.html>
More information about the Marble-devel
mailing list