<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/126523/">https://git.reviewboard.kde.org/r/126523/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 27th, 2015, 9:21 a.m. UTC, <b>Heiko Tietze</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Perfect use of the message panel. However, the red close icon does not work (well) with colored backgrounds.</p></pre>
</blockquote>
<p>On December 27th, 2015, 3:34 p.m. UTC, <b>Kai Uwe Broulik</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Do we even need the close button to begin with? It's not like it appearing is really under your direct control anyway.</p></pre>
</blockquote>
<p>On December 27th, 2015, 4:48 p.m. UTC, <b>Thomas Pfeiffer</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I agree with Kai: No need for a close button there, since the message widget disappears anyway as soon as the user fixes the problem.</p></pre>
</blockquote>
<p>On December 27th, 2015, 7:12 p.m. UTC, <b>Heiko Tietze</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not so sure. Popups should have means to get closed manually. Otherwise you may argue that plasma notifications also do not need any close interaction. And the HIG talks about the option to dismiss the panel. So either we change it everywhere and update the HIG or we find a good reason why it is not neccessary here. My position is clear: Don't use color as the primary indicator, i.e. do not color the whole icon. Colored decorations are okay.</p></pre>
</blockquote>
<p>On December 30th, 2015, 3:38 p.m. UTC, <b>Elvis Angelaccio</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So, should I leave it? If the close button needs to be fixed, I think should be done independently from this patch.</p></pre>
</blockquote>
<p>On December 30th, 2015, 3:50 p.m. UTC, <b>Thomas Pfeiffer</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The red close icon and the close button on panels are two independent issues, which are both independent from this patch.
Therefore, neither of them would block you from shipping this patch.
If the close icon is changed, you do not have to change anything in your code, anyway. If we decide that being able to close message panels manually is not necessary in all cases (this is a decision that has to be made on a HIG-level, not on an individual basis) we'll get in touch with you and ask you to remove the close button here (in a separate patch).
That's why Heiko already gave a "Ship it" from the usability side (the technical review is independent from this).</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">All right. Waiting for the code ship-it, then :)</p></pre>
<br />
<p>- Elvis</p>
<br />
<p>On December 26th, 2015, 10:18 a.m. UTC, Elvis Angelaccio 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 KDE Frameworks, KDE Usability, Kai Uwe Broulik, and Christoph Feck.</div>
<div>By Elvis Angelaccio.</div>
<p style="color: grey;"><i>Updated Dec. 26, 2015, 10:18 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kwidgetsaddons
</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;">Currently a KNewPasswordDialog makes use of a KTitleWidget to display messages to the user about the password status (e.g. "password too short", etc.). This looks out of place, and so does the associated icon (see screenshots in RR 125619: https://git.reviewboard.kde.org/r/125619/ )</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch replaces the KTitleWidget with a KMessageWidget. I also "downgraded" some of the previous "error" red icons to a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KMessageWidget::Warning</code> type (the orange one in the screenshots), because those kind of messages may be displayed before the user even started typing something as password.</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;">The KMessageWidgets behaves exactly like the KTitleWidget before: initially is hidden, then is displayed as soon as the user interacts with the password fields.</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>src/knewpassworddialog.cpp <span style="color: grey">(1d7ca33ac010a2a1d0971a152d0bef23f78c414d)</span></li>
<li>src/knewpassworddialog.ui <span style="color: grey">(2b492d2a2984296105959d366a1d5306c80af54f)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126523/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/12/26/f4870422-d064-4da9-b216-056fe5bec665__pwd-dialog-1.png">pwd-dialog-1.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/12/26/3d8b6c2c-6717-4608-b61c-18ef9b224706__pwd-dialog-2.png">pwd-dialog-2.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/12/26/3a71ff56-f540-405a-9f3e-868e8d652687__pwd-dialog-3.png">pwd-dialog-3.png</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>