<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/110446/">http://git.reviewboard.kde.org/r/110446/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 15th, 2013, 8:38 p.m. UTC, <b>Roney Gomes</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/110446/diff/1/?file=143905#file143905line103" style="color: black; font-weight: bold; text-decoration: underline;">mainwindow.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">102</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_main</span><span class="o">-></span><span class="n">m_size</span><span class="o">-=</span><span class="mi">33</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;">Please, try to make it a constant. 33 by itself has absolutely no meaning.
That's why Albert had to ask.</pre>
</blockquote>
<p>On May 15th, 2013, 8:44 p.m. UTC, <b>Albert Astals Cid</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;">A constant would be better, but I don't think a it is correct, from your description it seems to me that this is probably style dependant. Also the old code doesn't seem to need this, why do you need it?</pre>
</blockquote>
<p>On May 15th, 2013, 9:16 p.m. UTC, <b>Roney Gomes</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;">Yes. In the end it's all a matter of style. But in this is case it's a good one.
A constant would help future developers to maintain the code, making it clear what's the purpose of that magic number. Also, if he wants to stop the scroll bars from appearing he could have used QGraphicsView's methods:
setHorizontalScrollBarPolicy(Qt::ScrollBarAlwaysOff);
setVerticalScrollBarPolicy(Qt::ScrollBarAlwaysOff);</pre>
</blockquote>
<p>On May 15th, 2013, 9:24 p.m. UTC, <b>Anant Pushkar</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;">Its not about just the scrollbars. Earlier QLayout::SetFixedSize was used (at line 51 , mainwindow.cpp) that set the size to that given by sizeHint() and did not allow any further resizing. So, the earlier code never needed any such hard coded thresholding, but now that this patch allows resizing, we need to change the size of the basic layout itself otherwise things start to look weird. Anyways I have added a constant. Hope this works fine.</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;">OK, I was using the wrong height value to update m_size I have corrected this in the new diff.</pre>
<br />
<p>- Anant </p>
<br />
<p>On May 16th, 2013, 9:31 a.m. UTC, Anant Pushkar 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 KDE Games.</div>
<div>By Anant Pushkar.</div>
<p style="color: grey;"><i>Updated May 16, 2013, 9:31 a.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;">This is in reference to Bug 181978 - request for an adjustable zoom interface
With this patch included, the player can resize the window. The size of the playarea gets itself adjusted.</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>mainarea.h <span style="color: grey">(9757201)</span></li>
<li>mainarea.cpp <span style="color: grey">(6681147)</span></li>
<li>mainwindow.h <span style="color: grey">(1bb82ed)</span></li>
<li>mainwindow.cpp <span style="color: grey">(ad17cca)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110446/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>