<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, 6:19 p.m. CEST, <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 2nd, 2014, 12:10 a.m. CEST, <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, 5:45 p.m. CEST, <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, 11:47 p.m. CEST, <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>
<p>On August 3rd, 2014, 4:52 p.m. CEST, <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;">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>
</blockquote>
<p>On August 3rd, 2014, 5:38 p.m. CEST, <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;">regarding another point KoReportDesignerItemRectBase::minimumSize() const = 0; can it be without const at the end? I noticed that when I was testing the solution discussed my this looked like<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
QSizeF KoReportDesignerItemField::minimumSize() const<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
{<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
if (m_userWidth < getTextRect().width() || m_userHeight < getTextRect().height()) {<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
return(QSizeF(getTextRect().width(), getTextRect().height()));<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
}<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
return QSizeF(m_userWidth, m_userHeight);<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
}</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What I noticed was the error discards qualifiers [-fpermissive]." I tried making the getTextRect const but also got the same error that regarded inside of the operations within getTextRect(). The quickest solution seems to be removing const from minium size method. Do you agree?</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;">What is now left is whether init method arguments should be modified or should two more member variables and getter be introduced :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I am all for changing args of init methods.</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%">I tried making the getTextRect <span style="color: #008000; font-weight: bold">const</span> but also got the same error that regarded inside of the operations within getTextRect().
The quickest solution seems to be removing <span style="color: #008000; font-weight: bold">const</span> from minium size method. Do you agree<span style="color: #666666">?</span>
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It shall be quite easy to add const where needed until it compiles. Really, koreports deserve deeper look at this issue independently, BTW.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
I see dataSourceAndObjectTypeName() needs to be also converted, etc. But this isn't too hard to change, is it? The overall code quality will benefit, no need to continue this practice :)</p></pre>
<br />
<p>- Jarosław</p>
<br />
<p>On July 29th, 2014, 12:24 a.m. CEST, 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 29, 2014, 12:24 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=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>