<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/127354/">https://git.reviewboard.kde.org/r/127354/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 19th, 2016, 5:04 p.m. UTC, <b>David Narváez</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks for the patch! I would be in favor of dropping the QRect/QPoint usage in these files completely. Could you modify your patch to see how that would look like?</p></pre>
</blockquote>
<p>On March 19th, 2016, 7:45 p.m. UTC, <b>Maurizio Paolini</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">After some trying around I realize that dropping QRect in (e.g.) the KigPainter class cannot be done easily. In particular one should
understand the "overlay" concept... If I remember, at least in the early stages of kig development, overlays where used in order to
allow for efficent redrawing of the graphic primitives. I am not even sure that this is still used now! This was taken from the kgeo
project when first starting kig. If I place an "assert (false)" in "circleOverlayRecurse", then kig crashes... however I don't see the
need of using overlays for drawing circles if we use a Qt primitive for doing that.</p></pre>
</blockquote>
<p>On March 19th, 2016, 8:01 p.m. UTC, <b>David Narváez</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Interesting, I will check that. I did a quick pass through the code last weekend and was under the impression that there are needs for QRect in several places but KigPainter could do without QRect, but I must have missed this.</p></pre>
</blockquote>
<p>On March 19th, 2016, 8:13 p.m. UTC, <b>Maurizio Paolini</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now, if you ask me, I have the strong suspect that overlays are not used anymore... If this is the case, there is plenty of dead code in kigpainter. For example I tried to comment out the line
if( mNeedOverlay ) circleOverlay( center, radius );
in drawCircle and nothing (apparently) happened. However who knows...</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So this is my suggestion:
- commit this small patch keeping it minimal
- evaluate the whole "overlay" affair (under the suspect that it is no longer used and some code overhead could be dropped)
- reconsider removal of QRect/QPoint in favour of QRectF/QPointF in kigpainter once the mOverlay member is no longer present</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I recall that overlays where a mean to improve dynamic drawing and avoid flickering. In kig it was reportedly imported by
Dominique Devriese from the previous "kgeo" project, now defunct (by Marc Bartsch), and also had some contribution by me in the
drawing of circles part (circleOverlay).
If someone has better knowledge of overlays, please contribute!</p></pre>
<br />
<p>- Maurizio</p>
<br />
<p>On March 19th, 2016, 9:05 a.m. UTC, Maurizio Paolini wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Edu, David Narváez and Rex Dieter.</div>
<div>By Maurizio Paolini.</div>
<p style="color: grey;"><i>Updated March 19, 2016, 9:05 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=359805">359805</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kig
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QRect object in qt uses integers. This causes annoying loss of drawing precision when drawing circles (https://bugs.kde.org/show_bug.cgi?id=359805). Using the QRectF object seems more appropriate.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is a minimal patch to allow using QRectF when drawing circles. Perhaps using QRect
(and QPoint) could be dropped completely in favour of QRectF and QPointF.</p></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>misc/kigpainter.h <span style="color: grey">(e36c153)</span></li>
<li>misc/kigpainter.cpp <span style="color: grey">(e547bf2)</span></li>
<li>misc/screeninfo.h <span style="color: grey">(b7f94c4)</span></li>
<li>misc/screeninfo.cc <span style="color: grey">(8fdf115)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127354/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/03/19/48f5447f-0904-43ee-8c56-8fc07c03febb__circles_old.png">tangent circles - old behaviour</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/03/19/5684b1c5-c32c-447b-bed6-53be380f1b87__circles_new.png">tangent circles: new behaviour</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/03/19/08937b54-e313-4acf-b482-4bd899d24b30__circles_old_zoom.png">zoom of old behaviour</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/03/19/e7c0b8fb-af81-43da-b70e-bed3462d73a6__circles_new_zoom.png">zoom of new behaviour</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>