<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/128201/">https://git.reviewboard.kde.org/r/128201/</a>
</td>
</tr>
</table>
<br />
<div>
<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/128201/diff/2/?file=469229#file469229line90" style="color: black; font-weight: bold; text-decoration: underline;">shell/progresswidget/statusbarprogresswidget.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">84</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">bool</span> <span class="n">macWidgetStyle</span> <span class="o">=</span> <span class="n">QApplication</span><span class="o">::</span><span class="n">style</span><span class="p">()</span><span class="o">-></span><span class="n">objectName</span><span class="p">()</span> <span class="o">==</span> <span class="n">QLatin1String</span><span class="p">(</span><span class="s">"macintosh"</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">Technically this should check m_pProgressBar->style() rather than QApp::style().</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I see why this check is needed: the mac style doesn't care for the contents size in CT_ProgressBar, the return value is indeed "fixed".</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I guess the alternative to a hardcoded "==macintosh" would be a runtime test on the style,
i.e. calling CT_ProgressBar with maximumHeight as input, and checking if the return value is smaller than that. Then we know the style isn't taking into accounts the contents size, whatever the name of the style is. This might be a more future-proof and general approach.</p></pre>
</div>
</div>
<br />
<div>
<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/128201/diff/2/?file=469229#file469229line95" style="color: black; font-weight: bold; text-decoration: underline;">shell/progresswidget/statusbarprogresswidget.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">89</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// that is from sizeHint(), for a regular button that's from minimumSizeHint().</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">I don't understand that comment, this is about a progressbar, not a button (there's no call to minimumSizeHint in here)</p></pre>
</div>
</div>
<br />
<div>
<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/128201/diff/2/?file=469229#file469229line96" style="color: black; font-weight: bold; text-decoration: underline;">shell/progresswidget/statusbarprogresswidget.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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">90</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">int</span> <span class="n">pbHeight</span> <span class="o">=</span> <span class="n">m_pProgressBar</span><span class="o">-></span><span class="n">sizeHint</span><span class="p">().</span><span class="n">height</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<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;">Wouldn't this logic work everywhere, not just on mac?</p></pre>
</div>
</div>
<br />
<p>- David Faure</p>
<br />
<p>On June 18th, 2016, 5:47 p.m. UTC, René J.V. Bertin 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 KDevelop and David Faure.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated June 18, 2016, 5:47 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</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;">I came across this old bit of code, judging from the use of the deprecated <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Q_OS_MAC</code> token.
Some quick testing suggests that using <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">WA_LayoutUsesWidgetRect</code> achieves what I think is the intended effect without platform-specific code. (That platform test ought really check for using the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">macintosh</code> widget style btw, rather than "are we running on OS X").</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;">For now only on OS X, with the native theme as well as QtCurve, Breeze and Oxygen.</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>shell/progresswidget/statusbarprogresswidget.cpp <span style="color: grey">(1a32ea8)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128201/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/06/16/1498e8dc-4541-473b-b6db-2c2ae138cbc3__Screen_Shot_2016-06-17_at_00.17.17.png">OS X/Mac native *without* the patch</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/06/17/dd352994-1753-4b33-a0d2-cd2a3132df48__progress-stock.png">stock widget under Linux (same layout using Breeze)</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/06/18/8e6a0761-b760-4f41-be67-7f9ba7318349__Screen_Shot_2016-06-18_at_19.21.52.png">OS X/native with the new patch</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/06/18/3535be4c-3034-493d-88c0-61ab9a13844d__Screen_Shot_2016-06-18_at_19.24.46.png">OS X using QtCurve with the new patch</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/06/18/359d5f57-bdaa-45a7-a0b0-6acb5609459f__Screen_Shot_2016-06-18_at_19.16.19.png">Linux, Breeze, new patch</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/06/18/f5eb0902-884a-47e5-bf84-a3581a21bece__Screen_Shot_2016-06-18_at_19.14.17.png">Linux, QtCurve, new patch</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/06/18/536d4f09-8409-4eb4-80ca-27d3f744d6a0__Screen_Shot_2016-06-18_at_19.16.56.png">Linux, Oxygen, new patch</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/06/19/ee2d687e-69ea-493f-9ae4-87d90be614e2__progress-patched-breeze.png">Linux, Breeze + Breeze icons, new patch</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/06/19/7e3cfc5c-cb80-491c-b4d2-4d6f9c839a48__progress-patched-qtcurve.png">Linux, QtCurve + Breeze icons, new patch</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/06/19/0b10d97d-3ba6-4d05-a663-3a03e29bc1c9__progress-patched-oxygen.png">Linux,Oxygen+Breeze icons, new patch</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>