<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/110318/">http://git.reviewboard.kde.org/r/110318/</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 6th, 2013, 2:48 p.m. UTC, <b>Kurt Hindenburg</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/110318/diff/1/?file=142269#file142269line243" style="color: black; font-weight: bold; text-decoration: underline;">src/ColorSchemeEditor.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 ColorSchemeEditor::setup(const ColorScheme* scheme)</pre></td>
</tr>
</tbody>
<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">240</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// set the widget height to the table content</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;">This will force the table to be vertical enough to show all the colors. I don't see a reason to do this.</pre>
</blockquote>
<p>On May 7th, 2013, 9:08 a.m. UTC, <b>renan fargetton</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;">Since intense colors are in a third column with this patch, this table is half the height it used to be. Even with the fixed height this dialog is still smaller than its parent (EditProfileDialog) minumum height, so I don't think it is really an issue. With my settings colorTable is ~320px height, and the whole dialog ~500px.
The alternative would be to let it with Prefered sizePolicy and resize it to the good default size. I tried it first but I didn't get it to work well : since the table is inside a layout, it looks like it is the parent dialog that must be resized, making it complicate to calculate the right height. There is probably a good way to do this, but while I was into the documentation, I thought it was not really worth it, event considering usability : the widget is small enough and the scrolling bar is just making the interface more complicated for no real benefit.
</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;">Yes I see - since the EditDialog is better the size of the ColorEdit shouldn't matter.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 6th, 2013, 2:48 p.m. UTC, <b>Kurt Hindenburg</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;">Thanks for the code and testing. I think it would be best to split this up into smaller patches. One for the table fixes and another for the save/apply. There are also a few minor style issues but that's no big deal. I'll see if I can get this in before the freeze this month.</pre>
</blockquote>
<p>On May 7th, 2013, 9:08 a.m. UTC, <b>renan fargetton</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 have an intermediary commit with the table fix, so it easy to make two patches. Should I make two other review request ? (This is my first patch to kde so I don't know the workflow)
I would be happy to learn what are the style issues are since I'm learning. In order to avoid them next time...</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;">I have a patch for the dialog/table ready to go in so you can look at it when I commit it. If you want to redo the rest of this it would be appreciated. We still have a few weeks until the soft cutoff
- http://techbase.kde.org/Schedules/KDE4/4.11_Release_Schedule
In general, no tabs, watch spaces around "()," and try to keep lines < 80 (really minor) - we try to follow the kdelibs sytle - http://techbase.kde.org/Policies/Kdelibs_Coding_Style</pre>
<br />
<p>- Kurt</p>
<br />
<p>On May 5th, 2013, 3:45 p.m. UTC, renan fargetton 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 Konsole.</div>
<div>By renan fargetton.</div>
<p style="color: grey;"><i>Updated May 5, 2013, 3:45 p.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;">Improvements in ColorSchemeEditor dialog :
- colorTable has one more row for intense colors. This way each color and the corresponding intense color is side by side (the data model colorScheme is unchanged, only the ui has the additional column)
- colorTable has fixed height such that the table fit exactly inside.
- an Apply button is added, so that it is possible to save modified ColorScheme directly from colorSchemeEditor dialog. This way it is easier to preview the result of the color theme change, no need to close/open the dialog to get the preview.
* Most of the changes are in EditProfileDialog.cpp where the Kdialog is set up (EditProfileDialog::showColorSchemeEditor() ).
* Saving is done in a separated slot EditProfileDialog::saveColorScheme(const ColorScheme& scheme)</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 it on my machine / create and modify colorSchemes with the dialog</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>src/ColorSchemeEditor.h <span style="color: grey">(9b1e4b2)</span></li>
<li>src/ColorSchemeEditor.cpp <span style="color: grey">(e75cacb)</span></li>
<li>src/EditProfileDialog.h <span style="color: grey">(33b3930)</span></li>
<li>src/EditProfileDialog.cpp <span style="color: grey">(74b0237)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110318/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>