DevHeads.net

Review Request 108433: [High-dpi issues] Fix color kcm list when using big fonts

Review request for kde-workspace.

Description
This makes the row height of color list depend on the height of the "Varies" button.

The KColorButtons on the other pages also need fixing but this should be done in KColorButton in kdelibs rather than hacking in the "Varies" thing there as well.

(Not sure if I need to delete that PushButton afterwards)

Diffs
kcontrol/colors/colorscm.cpp b9b911f

Diff: <a href="http://git.reviewboard.kde.org/r/108433/diff/" title="http://git.reviewboard.kde.org/r/108433/diff/">http://git.reviewboard.kde.org/r/108433/diff/</a>

Testing
Yup, see screenshots.

File Attachments
After with normal fonts
<a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colornormalsize.png" title="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colornormalsize.png">http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colornorm...</a>
Before with huge fonts
<a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizebefore.png" title="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizebefore.png">http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigs...</a>
After with huge fonts
<a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizeafter.png" title="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizeafter.png">http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigs...</a>

Thanks,

Kai Uwe Broulik

Comments

Re: Review Request 108433: [High-dpi issues] Fix color kcm list

By Kai Uwe Broulik at 01/15/2013 - 21:56

(Updated Jan. 16, 2013, 1:56 a.m.)

Review request for kde-workspace.

Description
This makes the row height of color list depend on the height of the "Varies" button.

The KColorButtons on the other pages also need fixing but this should be done in KColorButton in kdelibs rather than hacking in the "Varies" thing there as well.

(Not sure if I need to delete that PushButton afterwards)

Diffs
kcontrol/colors/colorscm.cpp b9b911f

Diff: <a href="http://git.reviewboard.kde.org/r/108433/diff/" title="http://git.reviewboard.kde.org/r/108433/diff/">http://git.reviewboard.kde.org/r/108433/diff/</a>

Testing
Yup, see screenshots.

File Attachments
After with normal fonts
<a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colornormalsize.png" title="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colornormalsize.png">http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colornorm...</a>
Before with huge fonts
<a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizebefore.png" title="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizebefore.png">http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigs...</a>
After with huge fonts
<a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizeafter.png" title="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizeafter.png">http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigs...</a>

Thanks,

Kai Uwe Broulik

Re: Review Request 108433: [High-dpi issues] Fix color kcm list

By Kai Uwe Broulik at 12/15/2015 - 17:50

(Updated Dez. 15, 2015, 9:50 nachm.)

Status
This change has been discarded.

Review request for kde-workspace.

Repository: kde-workspace

Description
This makes the row height of color list depend on the height of the "Varies" button.

The KColorButtons on the other pages also need fixing but this should be done in KColorButton in kdelibs rather than hacking in the "Varies" thing there as well.

(Not sure if I need to delete that PushButton afterwards)

Diffs
kcontrol/colors/colorscm.cpp b9b911f

Diff: <a href="https://git.reviewboard.kde.org/r/108433/diff/" title="https://git.reviewboard.kde.org/r/108433/diff/">https://git.reviewboard.kde.org/r/108433/diff/</a>

Testing
Yup, see screenshots.

File Attachments
After with normal fonts
<a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colornormalsize.png" title="https://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colornormalsize.png">https://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colornor...</a>
Before with huge fonts
<a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizebefore.png" title="https://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizebefore.png">https://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbig...</a>
After with huge fonts
<a href="https://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizeafter.png" title="https://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizeafter.png">https://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbig...</a>

Thanks,

Kai Uwe Broulik

Re: Review Request 108433: [High-dpi issues] Fix color kcm list

By =?utf-8?Q?Thoma... at 01/16/2013 - 06:10

kcontrol/colors/colorscm.cpp
<http://git.reviewboard.kde.org/r/108433/#comment19545>

w/o having checked code it seems the view has a static row height and the last kid would by this set the game.

i'd say minHeight should be determined as qMax(minHeight, btn->sizeHint().height()) and initialized by the font height (given the font is equal for all elements, otherwise needs to be qMax'd in every row as well)

ultimately in a second pass set all row heights, while probably even setting one would be sufficient

- Thomas Lübking

On Jan. 16, 2013, 1:56 a.m., Kai Uwe Broulik wrote:

Re: Review Request 108433: [High-dpi issues] Fix color kcm list

By Kai Uwe Broulik at 01/16/2013 - 09:51

Ah, okay, but still, when the font gets small, the text in the left column would also get small? But I see your point.

Yep, was also surprised to find a table there :P The height of all rows? From what I can see this thing wants a particular row number passed and only set that?

- Kai Uwe

On Jan. 16, 2013, 1:56 a.m., Kai Uwe Broulik wrote:

Re: Review Request 108433: [High-dpi issues] Fix color kcm list

By Kai Uwe Broulik at 01/16/2013 - 09:29

minHeight is calculated by the Varies button which is the biggest element here (font + margin), the font is equal, yes.
Also, all rows are getting a minHeight (it's a for..next loop)

- Kai Uwe

On Jan. 16, 2013, 1:56 a.m., Kai Uwe Broulik wrote:

Re: Review Request 108433: [High-dpi issues] Fix color kcm list

By =?utf-8?Q?Thoma... at 01/16/2013 - 09:46

The problem is that you estimate on pre-"known" data ("varies" could be very tiny by fontsize or locle - not that the current situation would be any correct), so the minHeight should be set after the button (or stacked widget) has been created (so that the biggest element determines the height) and then set afterwards.

By "view has a static row height" i meant the "uniformItemSize" atribute is likely set but it seems that is actually a QTableWidget (lol ;-) so that the last time one calls "setRowHieght()" would determine the height of all rows. (looks much like this on the second screenshot)

- Thomas

On Jan. 16, 2013, 1:56 a.m., Kai Uwe Broulik wrote:

Re: Review Request 108433: [High-dpi issues] Fix color kcm list

By David Boosalis at 01/16/2013 - 19:28

<head><base href="local:///"></head><body data-blackberry-caret-color="#00a8df" style="background-color: rgb(255, 255, 255);"><div name="BB10" id="BB10_response_div" dir="auto" style="width:100%;background:#ffffff; font-size: initial;font-family:&quot;Calibri&quot;,&quot;Slate Pro&quot;,&quot;sans-serif&quot;;color:#1f497d"><br></div><p id="_signaturePlaceholder" name="BB10" dir="auto" style="marginTop:2px; font-size: initial;font-family:&quot;Calibri&quot;,&quot;Slate Pro&quot;,&quot;sans-serif&quot;;color:#1f497d;">Sent&nbsp;from&nbsp;my&nbsp;BlackBerry&nbsp;10&nbsp;smartphone.</p><table width="100%" style="border-spacing:0px;"> <tbody><tr><td colspan="2"><div id="_persistentHeader" style="border-style: solid none none; border-top-color: rgb(181, 196, 223); border-top-width: 1pt; padding: 3pt 0in 0in; font-family: Tahoma, 'BB Alpha Sans', 'Slate Pro'; font-size: 10pt;"><div><b>From: </b>Thomas&nbsp;Lübking</div><div><b>Sent: </b>Wednesday, January 16, 2013 8:47 AM</div><div><b>To: </b>Kai&nbsp;Uwe&nbsp;Broulik;&nbsp;Thomas&nbsp;Lübking;&nbsp;kde-workspace</div><div><b>Reply To: </b>kde-core- ... at kde dot org</div><div><b>Subject: </b>Re:&nbsp;Review&nbsp;Request&nbsp;108433:&nbsp;[High-dpi&nbsp;issues]&nbsp;Fix&nbsp;color&nbsp;kcm&nbsp;list&nbsp;when<br><br>&nbsp;using&nbsp;big&nbsp;fonts</div></div></td></tr></tbody></table><div id="_persistentHeaderEnd" style="border:none;border-top:solid #babcd1 1pt;"></div><br><div id="_originalContent" style="">

<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tbody><tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/108433/">http://git.reviewboard.kde.org/r/108433/</a>
</td>
</tr>
</tbody></table>
<br>

<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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/108433/diff/1/?file=107425#file107425line747" style="color: black; font-weight: bold; text-decoration: underline;">kcontrol/colors/colorscm.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 KColorCm::setupColorTable()</pre></td>

</tr>
</tbody>

<tbody>

<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">744</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">commonColorTable</span><span class="o">-&gt;</span><span class="n">setRowHeight</span><span class="p">(</span><span class="n">i</span><span class="p">,</span> <span class="n"><span class="hl">button</span></span><span class="o"><span class="hl">-&gt;</span></span><span class="n"><span class="hl">sizeHint</span></span><span class="p"><span class="hl">().</span></span><span class="n"><span class="hl">h</span>eight</span><span class="p"><span class="hl">()</span>);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">747</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">commonColorTable</span><span class="o">-&gt;</span><span class="n">setRowHeight</span><span class="p">(</span><span class="n">i</span><span class="p">,</span> <span class="n"><span class="hl">minH</span>eight</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;">w/o having checked code it seems the view has a static row height and the last kid would by this set the game.

i'd say minHeight should be determined as qMax(minHeight, btn-&gt;sizeHint().height()) and initialized by the font height (given the font is equal for all elements, otherwise needs to be qMax'd in every row as well)

ultimately in a second pass set all row heights, while probably even setting one would be sufficient</pre>
</blockquote>

<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;">minHeight is calculated by the Varies button which is the biggest element here (font + margin), the font is equal, yes.
Also, all rows are getting a minHeight (it's a for..next loop)</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;">The problem is that you estimate on pre-"known" data ("varies" could be very tiny by fontsize or locle - not that the current situation would be any correct), so the minHeight should be set after the button (or stacked widget) has been created (so that the biggest element determines the height) and then set afterwards.

By "view has a static row height" i meant the "uniformItemSize" atribute is likely set but it seems that is actually a QTableWidget (lol ;-) so that the last time one calls "setRowHieght()" would determine the height of all rows. (looks much like this on the second screenshot)</pre>
<br>

<p>- Thomas</p>

<br>

<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;">
<tbody><tr>
<td>

<div>Review request for kde-workspace.</div>
<div>By Kai Uwe Broulik.</div>

<p style="color: grey;"><i>Updated Jan. 16, 2013, 1:56 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">
<tbody><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 makes the row height of color list depend on the height of the "Varies" button.

The KColorButtons on the other pages also need fixing but this should be done in KColorButton in kdelibs rather than hacking in the "Varies" thing there as well.

(Not sure if I need to delete that PushButton afterwards)</pre>
</td>
</tr>
</tbody></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">
<tbody><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;">Yup, see screenshots.</pre>
</td>
</tr>
</tbody></table>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs </h1>
<ul style="margin-left: 3em; padding-left: 0;">

<li>kcontrol/colors/colorscm.cpp <span style="color: grey">(b9b911f)</span></li>

</ul>

<p><a href="http://git.reviewboard.kde.org/r/108433/diff/" style="margin-left: 3em;">View Diff</a></p>

<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>

<ul>

<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colornormalsize.png">After with normal fonts</a></li>

<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizebefore.png">Before with huge fonts</a></li>

<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/01/16/colorbigsizeafter.png">After with huge fonts</a></li>

</ul>

</td>
</tr>
</tbody></table>

</div>

<br></div> <!--end of _originalContent --></body>