<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/112035/">http://git.reviewboard.kde.org/r/112035/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 17th, 2013, 6:22 p.m. UTC, <b>Dennis Nienhüser</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/112035/diff/2/?file=181574#file181574line26" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/cloudsync/RouteItem.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">26</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1">// d-pointer but they break route preview feature.</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Dennis</p>
<br />
<p>On August 16th, 2013, 7:54 p.m. UTC, Utku Aydın wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Marble.</div>
<div>By Utku Aydın.</div>
<p style="color: grey;"><i>Updated Aug. 16, 2013, 7:54 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I manually tested every feature on two different servers. There is no unit test available at the moment.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/QtMainWindow.cpp <span style="color: grey">(913d0d4)</span></li>
<li>src/icons/cloud-download.png <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/icons/cloud-upload.png <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/CMakeLists.txt <span style="color: grey">(88c188a)</span></li>
<li>src/lib/MarbleCloudSyncSettingsWidget.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/MarbleModel.h <span style="color: grey">(973e77c)</span></li>
<li>src/lib/MarbleModel.cpp <span style="color: grey">(77e0969)</span></li>
<li>src/lib/MarbleWidget.h <span style="color: grey">(6493a6c)</span></li>
<li>src/lib/QtMarbleConfigDialog.h <span style="color: grey">(9eb3cfc)</span></li>
<li>src/lib/QtMarbleConfigDialog.cpp <span style="color: grey">(47ed016)</span></li>
<li>src/lib/cloudsync/AbstractSyncBackend.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/AbstractSyncBackend.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/CloudRouteModel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/CloudRouteModel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/CloudRoutesDialog.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/CloudRoutesDialog.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/CloudRoutesDialog.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/CloudSyncManager.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/CloudSyncManager.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/OwncloudSyncBackend.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/OwncloudSyncBackend.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/RouteItem.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/RouteItem.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/RouteItemDelegate.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/RouteItemDelegate.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/RouteParser.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/RouteParser.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/RouteSyncManager.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/cloudsync/RouteSyncManager.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/routing/RoutingWidget.h <span style="color: grey">(9622ea9)</span></li>
<li>src/lib/routing/RoutingWidget.cpp <span style="color: grey">(afe1cdd)</span></li>
<li>src/marble.kcfg <span style="color: grey">(d3b5505)</span></li>
<li>src/marble.qrc <span style="color: grey">(dc7e8ae)</span></li>
<li>src/marble_part.h <span style="color: grey">(1c3dd74)</span></li>
<li>src/marble_part.cpp <span style="color: grey">(6cbb319)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112035/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>