<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/111838/">http://git.reviewboard.kde.org/r/111838/</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 1st, 2013, 5:17 p.m. UTC, <b>Torsten Rahn</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Is this really a good idea? While I agree on the choice for view classes (like GeoGraphicsItems). I'm concerned about usage of QPixmap in GeoData* classes: They are models and could be generated in threads. However creation of QPixmaps in threads is a bad idea since they are created e.g. on the X-Server which only knows about the GUI thread. We already get warnings when using the graphics system X11 for this reason. For this reason I suggest to use QPixmap only in the view classes while preferring QImages on the model side. </pre>
</blockquote>
<p>On August 1st, 2013, 5:22 p.m. UTC, <b>Bernhard Beschow</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">For memory efficiency, it would be better to store pixmaps in the GeoData* classes and use them directly for rendering. For example, consider loading a pixmap in a single style instance once and use it for 1000+ GeoGraphicsItems.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">So what do you suggest for the quite common usecase where a GeoDataDocument is created in a thread? I think this is something we should support since it does happen already and since it can barely be reasonably discouraged?
Your intentions are reasonable. But maybe we should do it differently altogether and just store the Url to the icon in the style instead of instantiating the pixmap/image already. Only once rendered the image would then get loaded.</pre>
<br />
<p>- Torsten</p>
<br />
<p>On August 1st, 2013, 4:25 p.m. UTC, René Küttner 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 René Küttner.</div>
<p style="color: grey;"><i>Updated Aug. 1, 2013, 4:25 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;">GeoDataIconStyle stores its icon as a QImage at the moment. Since these styles are meant for rendering and QImage is rather slow for this purpose, I changed it to use QPixmap internally.</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 applied the patch to master and did compile marble. I also compared the visual output of the icon style before and after the patch.</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/lib/MarbleWidgetPopupMenu.cpp <span style="color: grey">(ba27f36)</span></li>
<li>src/lib/VisiblePlacemark.cpp <span style="color: grey">(42c1103)</span></li>
<li>src/lib/geodata/data/GeoDataFeature.h <span style="color: grey">(4cd0374)</span></li>
<li>src/lib/geodata/data/GeoDataFeature.cpp <span style="color: grey">(8a99a85)</span></li>
<li>src/lib/geodata/data/GeoDataIconStyle.h <span style="color: grey">(5b4644f)</span></li>
<li>src/lib/geodata/data/GeoDataIconStyle.cpp <span style="color: grey">(d68d0bb)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111838/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>