<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/104735/">http://git.reviewboard.kde.org/r/104735/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 26th, 2012, 12:15 p.m., <b>Marco Martin</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;">i agree that looks better here, but i would use thins only for the toolbutton (and not doing a similar thing for pushbuttons)
the reason is in part aestetic (if they are stacked vertically they look better if they have the same width) and in part technical: plasma will try to share the rendered svgs both in memory and in disk cache, so the more elements with exactly the same size there are, the more memory is saved</pre>
</blockquote>
<p>On April 26th, 2012, 12:28 p.m., <b>Aurélien Gâteau</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;">I am not sure what you mean with "only for the toolbutton": this patch only touches ToolButton.
I agree a column of buttons look nicer if they have the same width, but defining a minimum width in ToolButton is not the correct way to go: if the text of some buttons is longer than the minimum width, those buttons won't be correctly aligned with the others (and you can't rely on the text staying short enough once translated). To ensure all widgets in a column to have the same width, you have to enforce it at the column level.
Regarding the memory reason: I believe internal optimizations should not dictate the appearance of the UI, so I don't think this is a valid reason.</pre>
</blockquote>
<p>On April 26th, 2012, 12:34 p.m., <b>Marco Martin</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;">what i said is that this patch is ok as is touches only toolbutton, i just wouldn't want a similar one for pushbutton.
as for the memory, right now decreasing memory usage is my #1 priority, we simply can't forget that constraints do exist, sorry.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">OK, thanks for the clarification, I am going to merge it then.</pre>
<br />
<p>- Aurélien</p>
<br />
<p>On April 26th, 2012, 8:27 a.m., Aurélien Gâteau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Plasma and David Edmundson.</div>
<div>By Aurélien Gâteau.</div>
<p style="color: grey;"><i>Updated April 26, 2012, 8:27 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 patch fixes two issues in ToolButton layout:
1. Make sure there is some space between the button icon and its text
2. Do not enforce a minimum width
The reason for #2 is that having a minimum width does not make much sense for a ToolButton:
- It should aim at using the minimum amount of horizontal space when used in a ToolBar.
- It looks unbalanced when used with an icon because the content is flushed to the left, leaving a large amount of white-space on the right. (See attached screenshots)
</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;">Run attached test script, you should notice the differences in spacing between button icon and text, as well as in the white-space on the right of the button.</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>plasma/declarativeimports/plasmacomponents/qml/ToolButton.qml <span style="color: grey">(0447a69)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104735/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/104735/s/545/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/04/26/toolbutton-layout-before_400x100.png" style="border: 1px black solid;" alt="Before" /></a>
<a href="http://git.reviewboard.kde.org/r/104735/s/546/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/04/26/toolbutton-layout-after_400x100.png" style="border: 1px black solid;" alt="After" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>