<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://svn.reviewboard.kde.org/r/5609/">http://svn.reviewboard.kde.org/r/5609/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">So some people don't like the nice shiny icons in menu items? That's sad :-)
Anyway, patch looks good, except for one bug. Feel free to commit if you agree with the suggested change.</pre>
<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="http://svn.reviewboard.kde.org/r/5609/diff/1/?file=39170#file39170line1119" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdelibs/kdeui/kernel/kglobalsettings.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 KGlobalSettings::Private::propagateQtSettings()</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">1119</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">showIcons</span> <span class="o">=</span> <span class="n">cg</span><span class="p">.</span><span class="n">readEntry</span><span class="p">(</span><span class="s">"ShowIconsInMenuItems"</span><span class="p">,</span> <span class="o">!</span><span class="n">QApplication</span><span class="o">::</span><span class="n">testAttribute</span><span class="p">(</span><span class="n">Qt</span><span class="o">::</span><span class="n">AA_DontShowIconsInMenus</span><span class="p">));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This default value surprises me. It basically says "by default, whatever Qt does, it must be wrong, let me toggle it".
I think you meant cg.readEntry("ShowIconsInMenuItems", true), which matches the current default behavior and the default value in the similar KCM code.</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On October 12th, 2010, 4:58 a.m., Christoph Feck wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.orgrb/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 kdelibs and Hugo Pereira Da Costa.</div>
<div>By Christoph Feck.</div>
<p style="color: grey;"><i>Updated 2010-10-12 04:58:51</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;">The first part of this patch makes the Qt::AA_DontShowIconsInMenus configurable in kdelibs via a KDE config entry.
The second part exposes this config entry in the Styles KCM, just right between the settings to hide icons on buttons or toolbars.
Note that GNOME also has this configuration option.
</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;">Hiding the menu icons works with all styles (even pure Qt styles), because Qt itself respects this attribute when setting up the menus.
The only "bug" is a missing dialog informing the user that changing this option only affects applications after restarting (same as Xft font settings or locale options). But before I add that, I want some feedback.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=253339">253339</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/KDE/kdebase/workspace/kcontrol/style/finetuning.ui <span style="color: grey">(1184959)</span></li>
<li>/trunk/KDE/kdebase/workspace/kcontrol/style/kcmstyle.cpp <span style="color: grey">(1184959)</span></li>
<li>/trunk/KDE/kdelibs/kdeui/kernel/kglobalsettings.cpp <span style="color: grey">(1184959)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5609/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>