<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/119527/">https://git.reviewboard.kde.org/r/119527/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 29th, 2014, 4:19 p.m. UTC, <b>Jarosław Staniek</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="https://git.reviewboard.kde.org/r/119527/diff/1/?file=293998#file293998line53" style="color: black; font-weight: bold; text-decoration: underline;">libs/koreport/items/check/KoReportDesignerItemCheck.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void KoReportDesignerItemCheck::init(QGraphicsScene * scene)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">53</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">setSceneRect</span><span class="p">(</span><span class="n">QPointF</span><span class="p">(</span><span class="mi">0</span><span class="p">,</span> <span class="mi">0</span><span class="p">),</span> <span class="n">QSizeF</span><span class="p">(</span><span class="mi">15</span><span class="p">,</span> <span class="mi">15</span><span class="p">));</span> <span class="c1">//default size</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">53</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">m_userWidth</span> <span class="o"><</span> <span class="mi">15</span> <span class="o">||</span> <span class="n">m_userHeight</span> <span class="o"><</span> <span class="mi">15</span><span class="p">)</span> <span class="p">{</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;"><ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">How about moving this code to KoReportDesignerItemRectBase ctor?</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And add virtual QSizeF KoReportDesignerItemRectBase::minimumSize() const = 0; which you'll implement in subclasses. There, sometimes you want to return getTextRect().size(), sometimes yoyu want to return QSizeF(5, 5), etc.</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">m_userWidth/m_userHeight are not needed if you can just use r->countHeight() and r->countWidth(). That's typical overuse of member variables.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Give the fixes I propsed below, m_user* members can be equal to -1. Check this and if either is -1, use default size.</p>
</li>
</ol></pre>
</blockquote>
<p>On August 1st, 2014, 10:10 p.m. UTC, <b>Wojciech Kosowicz</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;">This is the situaion that occurs: <br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
1. setRectScene must be called in constructor's init. Before all the values are set for 0<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
2. KoReportDesignerItemRectBase::minimumSize() I propose to return QRectF<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
3. To return QRectF I would need two more member variables <br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
m_userHeight = r->countHeight();<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
m_userWidth = r->countWidth();<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
m_pressX = r->m_pressX;<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
m_pressY = r->m_pressY;<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
theoretically by modyfying init(giving it specific pointer) method I could use them without writing theirs values to member variables but it be would rather significant change in reports. and I would still need pressX, pressY members :)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
4. Regarding second point I agree to initialize members to -1 but I think the way I proposed of checking for minimum size by comparison to minimum value is enought</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">By the way when I learnt about design patterns :)about year and also thought of using template method here :) I didn't propose it as I wanted your opinion but I'm glad it came up :)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
I would like to share your opinions regarding the points presented above as any change here goes with changes within about 20 files so I would like to have it more or less clear when starting modification:)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Regards :)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Wojtek</p></pre>
</blockquote>
<p>On August 2nd, 2014, 3:45 p.m. UTC, <b>Wojciech Kosowicz</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;">The solution above would also require two new getters for m_pressX and m_pressY</p></pre>
</blockquote>
<p>On August 2nd, 2014, 9:47 p.m. UTC, <b>Jarosław Staniek</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;">setRectScene must be called in constructor's init. Before all the values are set for 0</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Then it can be moved to KoReportDesignerItemRectBase::init(), right?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">KoReportDesignerItemRectBase<span style="color: #666666">::</span>minimumSize() I propose to <span style="color: #008000; font-weight: bold">return</span> QRectF
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If the position of the rect is always equal to (r->m_pressX, r->m_pressY), then there is no point in returing rect, right? Size is enough.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Or do you expect some implementations will return some other position?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">To <span style="color: #008000; font-weight: bold">return</span> QRectF I would need two more member variables
m_userHeight <span style="color: #666666">=</span> r<span style="color: #666666">-></span>countHeight();
m_userWidth <span style="color: #666666">=</span> r<span style="color: #666666">-></span>countWidth();
m_pressX <span style="color: #666666">=</span> r<span style="color: #666666">-></span>m_pressX;
m_pressY <span style="color: #666666">=</span> r<span style="color: #666666">-></span>m_pressY;
theoretically by modyfying init(giving it specific pointer) method I could use them without writing theirs values to member variables but it be would rather significant change in reports. and I would still need pressX, pressY members <span style="color: #666666">:</span>)
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No opinion for now untill we decide if we return QSizeF() or not, see above.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">Regarding second point I agree to initialize members to <span style="color: #666666">-1</span> but I think the way I proposed of checking <span style="color: #008000; font-weight: bold">for</span> minimum size by comparison to minimum value is enought
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">OK</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">By the way when I learnt about design patterns <span style="color: #666666">:</span>)about year and also thought of using template method here <span style="color: #666666">:</span>) I didn<span style="border: 1px solid #FF0000">'</span>t propose it as I wanted your opinion but I<span style="border: 1px solid #FF0000">'</span>m glad it came up <span style="color: #666666">:</span>)
I would like to share your opinions regarding the points presented above as any change here goes with changes within about <span style="color: #666666">20</span> files so I would like to have it more or less clear when starting modification<span style="color: #666666">:</span>)
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">OK!</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks!! I agree size is enough to return from minimum size method. What is now left is whether init method arguments should be modified or should two more member variables and getter be introduced :)</p></pre>
<br />
<p>- Wojciech</p>
<br />
<p>On July 28th, 2014, 10:24 p.m. UTC, Wojciech Kosowicz 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 Calligra, Adam Pigg and Jarosław Staniek.</div>
<div>By Wojciech Kosowicz.</div>
<p style="color: grey;"><i>Updated July 28, 2014, 10:24 p.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=334967">334967</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
calligra
</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;">If the specified dimensions are improper or too small elements get their defaults size</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;">When adding any element in kexi report it will the same size as specified by the user at the moment of adding</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>libs/koreport/items/check/KoReportDesignerItemCheck.cpp <span style="color: grey">(5bebc6c)</span></li>
<li>libs/koreport/items/field/KoReportDesignerItemField.cpp <span style="color: grey">(0dc9325)</span></li>
<li>libs/koreport/items/image/KoReportDesignerItemImage.cpp <span style="color: grey">(87e46a1)</span></li>
<li>libs/koreport/items/label/KoReportDesignerItemLabel.cpp <span style="color: grey">(8974a80)</span></li>
<li>libs/koreport/items/text/KoReportDesignerItemText.cpp <span style="color: grey">(51e12d3)</span></li>
<li>libs/koreport/wrtembed/KoReportDesigner.h <span style="color: grey">(adc712a)</span></li>
<li>libs/koreport/wrtembed/KoReportDesigner.cpp <span style="color: grey">(63152a7)</span></li>
<li>libs/koreport/wrtembed/KoReportDesignerItemLine.h <span style="color: grey">(894cec9)</span></li>
<li>libs/koreport/wrtembed/KoReportDesignerItemLine.cpp <span style="color: grey">(1b0474d)</span></li>
<li>libs/koreport/wrtembed/KoReportDesignerItemRectBase.h <span style="color: grey">(b53a58e)</span></li>
<li>libs/koreport/wrtembed/KoReportDesignerItemRectBase.cpp <span style="color: grey">(53c3727)</span></li>
<li>libs/koreport/wrtembed/reportsceneview.h <span style="color: grey">(cc3ab9f)</span></li>
<li>libs/koreport/wrtembed/reportsceneview.cpp <span style="color: grey">(2b679a4)</span></li>
<li>plugins/reporting/barcode/KoReportDesignerItemBarcode.cpp <span style="color: grey">(b4fb621)</span></li>
<li>plugins/reporting/maps/KoReportDesignerItemMaps.cpp <span style="color: grey">(f624db3)</span></li>
<li>plugins/reporting/web/KoReportDesignerItemWeb.cpp <span style="color: grey">(63e1cc9)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119527/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>