<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/106130/">http://git.reviewboard.kde.org/r/106130/</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;">UI review based on the screenshots in your email to plasma-devel, plus a few fairly minor code comments.
http://ivan.fomentgroup.org/blog/wp-content/uploads/2012/08/kamdsettings.png
Try to avoid nesting GroupBoxes, especially inside a TabWidget, you get this box inside boxes inside boxes effect.
A list of radio buttons should have a question above. In this case you're using the groupbox title as the question.
You may find the first one works better simply as a label.
Remember Open Documents:
[ ] For all applications
[ ] Only for specific applications
[ ] Don't remember open documents
Also rules dictate all forms should be in a form layout.
http://img833.imageshack.us/img833/341/kamdsettings1.png
I'm afraid you're not going to like this, and this is going to cause an active discussion I think.
QML in KDE applications stands out massively, it's inconsistent with the separation we're having and inconsistent means wrong, (IMHO) we can't do it at all until QML QtStyles.
To pick this particular screenshot apart:
- You're using a different font in your delegates. It needs to be the ones the user set in Application Appearance -> Fonts.
- You'll (possibly) find a 1 or 2 px vertical padding on the large icon which is on the left of the delegate will help, you won't get it touching it at the top. And you'll avoid a clash with highlights.
- If this is mimicking a list view it needs to behave the same
- Scrollbar should be on the right hand side of the of the list view, and needs to look like the conventional
- Keyboard navigation
- Highlight on mouseover.
There's some code in KWin that does (most of) the above, after a similar discussion. Maybe we( or I?) should work on some standard library code for this as it seems quite a common thing to want to do.
http://img94.imageshack.us/img94/8151/kamdsettings2.png
Same as all the above, plasma widgets inside an application breaks the workspace, application boundaries. You've got no choice but to use the "wrong widgets", and behaving in a manner inconsistent with other KCMs or even KDE applications with the slide in shouldn't be done.
Ignoring that and focussing on particulars:
A checkbox with the label "Privacy" doesn't make sense to me. "Private" may work better. It definitely needs a tooltip if it's not got one already. Possibly even needs a full sentence label next to it to explain what this actually means. If I didn't read your blogs I'm not sure I'd know.
The icon needs to look more like a button so I know I can click it. I assume it's a Plasmoid ToolButton currently, a Button would make it clearer
Padding between buttons. Qt forms are almost always 4px apart.
</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://git.reviewboard.kde.org/r/106130/diff/1/?file=80117#file80117line92" style="color: black; font-weight: bold; text-decoration: underline;">src/workspace/settings/BlacklistedApplicationsModel.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void BlacklistedApplicationsModel::load()</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">92</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">beginRemoveRows</span><span class="p">(</span><span class="n">QModelIndex</span><span class="p">(),</span> <span class="mi">0</span><span class="p">,</span> <span class="n">d</span><span class="o">-></span><span class="n">applications</span><span class="p">.</span><span class="n">length</span><span class="p">()</span> <span class="o">-</span> <span class="mi">1</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;">if applications.length is 0, you remove 0 to -1, and that gives you a crash (well qFatal)
</pre>
</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="http://git.reviewboard.kde.org/r/106130/diff/1/?file=80117#file80117line113" style="color: black; font-weight: bold; text-decoration: underline;">src/workspace/settings/BlacklistedApplicationsModel.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">void BlacklistedApplicationsModel::load()</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">113</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">foreach</span> <span class="p">(</span><span class="n">val</span> <span class="o">&</span> <span class="n">name</span><span class="p">,</span> <span class="n">applications</span><span class="p">)</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;">const & </pre>
</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="http://git.reviewboard.kde.org/r/106130/diff/1/?file=80118#file80118line15" style="color: black; font-weight: bold; text-decoration: underline;">src/workspace/settings/CMakeLists.txt</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; "></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">15</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> MainConfigurationWidget.cpp</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;">capital letter in class filename isn't typically usual, and doesn't match any other cpp in kactivities repo.</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On August 22nd, 2012, 10:04 p.m., Ivan Čukić 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.</div>
<div>By Ivan Čukić.</div>
<p style="color: grey;"><i>Updated Aug. 22, 2012, 10:04 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;">System settings module for activities</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>cmake/modules/c++11-test-initializer-lists-N2672.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(63ed737)</span></li>
<li>src/config-features.h.cmake <span style="color: grey">(bde6b66)</span></li>
<li>src/service/Application.cpp <span style="color: grey">(5dadbca)</span></li>
<li>src/service/plugins/sqlite/StatsPlugin.cpp <span style="color: grey">(6f24be8)</span></li>
<li>src/service/plugins/virtualdesktopswitch/virtualdesktopswitch.cpp <span style="color: grey">(568e0f9)</span></li>
<li>src/workspace/CMakeLists.txt <span style="color: grey">(8a6e1bb)</span></li>
<li>src/workspace/settings/BlacklistedApplicationsModel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/workspace/settings/BlacklistedApplicationsModel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/workspace/settings/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/workspace/settings/MainConfigurationWidget.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/workspace/settings/MainConfigurationWidget.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/workspace/settings/kcm_activities.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/workspace/settings/kcm_activities.desktop <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/workspace/settings/qml/BlacklistApplicationView.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/workspace/settings/ui/MainConfigurationWidgetBase.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/workspace/settings/ui/kdeclarativeview.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/workspace/settings/ui/kdeclarativeview.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106130/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>