<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/122048/">https://git.reviewboard.kde.org/r/122048/</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I really like this feature.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">However, I think that UI wise, it looks a bit unpolished. It adds two checkboxes with long texts, that even I, being a domain expert, need some thinking to understand.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I wonder if we should offer these actions at all, or whether we should enable the features by default. They do seem useful, I can construct a use-case pretty easily, but we may get away without the UI options, still keeping the config keys, perhaps. Then wait until someone complains or reports unexpected behavior, and then rethink adding the options.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Inline, a few niggles about naming when reading the code and trying to understand it, nothing really major.</p></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="https://git.reviewboard.kde.org/r/122048/diff/1/?file=341826#file341826line10" style="color: black; font-weight: bold; text-decoration: underline;">PowerDevilSettings.kcfg</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">10</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <entry name="doNotInhibitOnLidClose" type="Bool"></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Why not inhibitOnLidClose? Less negations make it more readable. (I had to think about what it meant.)</p></pre>
</div>
</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="https://git.reviewboard.kde.org/r/122048/diff/1/?file=341828#file341828line68" style="color: black; font-weight: bold; text-decoration: underline;">daemon/actions/bundled/handlebuttonevents.h</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; ">private Q_SLOTS:</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">68</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">m_lidNoExternalMonitor</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Off-hand, I don't understand what these vars ar e doing by reading their name. Would be nice if it was more clear for the next person to look at this code. Perhaps a comment?</p></pre>
</div>
</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="https://git.reviewboard.kde.org/r/122048/diff/1/?file=341831#file341831line141" style="color: black; font-weight: bold; text-decoration: underline;">daemon/actions/bundled/handlebuttoneventsconfig.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; ">QList< QPair< QString, QWidget* > > HandleButtonEventsConfig::buildUi()</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">141</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">retlist</span><span class="p">.</span><span class="n">append</span><span class="p">(</span><span class="n">qMakePair</span><span class="o"><</span><span class="n">QString</span><span class="p">,</span> <span class="n">QWidget</span> <span class="o">*></span><span class="p">(</span><span class="n">i18nc</span><span class="p">(</span><span class="s">"Only execute action on lid close when no external monitor is connected"</span><span class="p">,</span> <span class="s">"Only when no external monitor is connected"</span><span class="p">),</span> <span class="n">m_lidCloseNoExternalMonitor</span><span class="p">));</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No super-happy with how these strings look like.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Idea:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Do not <suspend> when external monitor is connected</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As to the second option, is it clear that the lid close depends on power management being enabled? Do we need this option at all? (Both are open questions.)</p></pre>
</div>
</div>
<br />
<p>- Sebastian Kügler</p>
<br />
<p>On January 13th, 2015, 11:16 p.m. UTC, Kai Uwe Broulik wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Solid, KDE Usability, Àlex Fiestas, and Daniel Vrátil.</div>
<div>By Kai Uwe Broulik.</div>
<p style="color: grey;"><i>Updated Jan. 13, 2015, 11:16 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
powerdevil
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Less sensational headline: Skip lid action when external monitor is connected.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This brings the KScreen killer feature in the 4.x times back. Now you can watch movies and safely close the lid again!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The confusing "Never prevent an action on lid close" is also moved to the main page since it only affects the lid action and is used nowhere else. I'm not happy with the wording but "inhibition" is a difficult thing to describe for the average user.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Laptop only, monitor option on -> Suspend
Laptop only, monitor option off -> Suspend
TV connected, monitor option on -> No action
TV connected, monitor option off -> Suspend</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">PM enabled, inhibit option on -> Suspend
PM disabled, inhibit option on -> No suspend
PM enabled, inhibit option off -> Suspend
PM disabled, inhibit option off -> Suspend</p></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>CMakeLists.txt <span style="color: grey">(27f162c)</span></li>
<li>PowerDevilSettings.kcfg <span style="color: grey">(1bc7ce1)</span></li>
<li>daemon/CMakeLists.txt <span style="color: grey">(f1e6efb)</span></li>
<li>daemon/actions/bundled/handlebuttonevents.h <span style="color: grey">(8ea23f6)</span></li>
<li>daemon/actions/bundled/handlebuttonevents.cpp <span style="color: grey">(ac280f4)</span></li>
<li>daemon/actions/bundled/handlebuttoneventsconfig.h <span style="color: grey">(a55bca7)</span></li>
<li>daemon/actions/bundled/handlebuttoneventsconfig.cpp <span style="color: grey">(92f0cef)</span></li>
<li>kcmodule/global/GeneralPage.cpp <span style="color: grey">(5d9ff10)</span></li>
<li>kcmodule/global/generalPage.ui <span style="color: grey">(26204cb)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122048/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/01/13/e0956548-0a0f-4c41-b3dd-68a5f17815a5__powerdevilstuff.png">Config option</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>