<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/128097/">https://git.reviewboard.kde.org/r/128097/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 5th, 2016, 2:11 p.m. CEST, <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;">First of all, as for any visual changes: Please provide before/after screenshots so that designers can do visual reviews.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Second: I am strictly against making this an option. Either it's an improvement, then it should always be like that, or it isn't an improvement, than it should be dropped. We should not offer a config option for every tine tweak, it just overcomplicates our config UIs for little gain.
This is not something so fundamental that that some users <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">need</em> it while others <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">can't stand it</em>.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So: The VDG will decide - after we've seen screenshots - whether checked menu items should use a checkmark or not, but making this an option will never get a "ship it" from the usability side.</p></pre>
</blockquote>
<p>On June 5th, 2016, 2:40 p.m. CEST, <b>René J.V. Bertin</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'm sorry, but that's should be up to the QtCurve developers to decide. QtCurve is also a pure Qt style, and AFAIK not at the same level as Oxygen and Breeze. It's the highly-configurable widget style, and as such the style's users should have the final say what options are to get in or not, in my opinion. Having a VDG is nice, but it should never have complete dictatorship over everything.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And yes, I do think it's exactly something that some might need (distinguishing exclusive menu items from non-exclusive ones) while some cannot stand that because such a visual difference doesn't correspond to their idea of HIG.</p></pre>
</blockquote>
<p>On June 5th, 2016, 2:57 p.m. CEST, <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;">A yes, sorry, I did not read that this was about QtCurve because it showed up in the Plasma list.
Corry on, then, do whatever you please, sorry to interrupt.</p></pre>
</blockquote>
<p>On June 5th, 2016, 2:59 p.m. CEST, <b>René J.V. Bertin</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;">One more thing: QtCurve provides its own configuration UI, there's no question of modifying any "official" UI that's also used by other components.</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;">Ah, ok. Whew. Apologies accepted :) I only added the Plasma list because I hoped someone there might have constructive feedback esp. about the UI implementation. "Error" corrected.</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On June 4th, 2016, 9:53 p.m. CEST, René J.V. Bertin 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 Software on Mac OS X and Yichao Yu.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated June 4, 2016, 9:53 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
qtcurve
</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;">This introduces an option (hidden for now) to adorn checked menu items with only a check mark rather than the same widget that is used for checkboxes or radio buttons (for sets of mutually exclusive menu items).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Initially I implemented this by simply skipping the widget "box" and drawing only the checkbox tick for both kinds of menu items (cf. https://bugs.kde.org/show_bug.cgi?id=363895). I then realised that this looks weird when the user uses a very tall or tiny font (or has a high DPI screen). Therefore the check is now generated using the UniCode <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Check Mark</code> glyph (? cf. http://www.fileformat.info/info/unicode/char/2713/index.htm) rendered in the menu font (or <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Apple Symbols</code>, on OS X).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">A new member is introduced in the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Options</code> structure that controls this new behaviour. Its value is read from and written to the config file, but I have not yet implemented its UI control through the configuration interface. I'll want some guidance for that step.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I propose to make this the default behaviour on OS X, so that popup menus can be closer in appearance to the native menus from the toplevel menubar (those menus are not rendered through Qt, and use a single check mark too).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The GTk2 style already rendered checked menu items like this so I did not change anything there (and don't really plan to touch that code at all).</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;">On OS X 10.9 and Linux with Qt 4.8.7 and Qt 5.6.0 .</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>gtk2/common/common.h <span style="color: grey">(cb0ec87)</span></li>
<li>gtk2/common/config_file.cpp <span style="color: grey">(96936e2)</span></li>
<li>qt4/common/common.h <span style="color: grey">(313db33)</span></li>
<li>qt4/common/config_file.cpp <span style="color: grey">(c58ad1a)</span></li>
<li>qt4/style/qtcurve.cpp <span style="color: grey">(951ec1a)</span></li>
<li>qt5/common/common.h <span style="color: grey">(bb103fd)</span></li>
<li>qt5/common/config_file.cpp <span style="color: grey">(362381a)</span></li>
<li>qt5/style/qtcurve_api.cpp <span style="color: grey">(b8535da)</span></li>
<li>qt5/style/qtcurve_primitive.cpp <span style="color: grey">(a8a2bed)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128097/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>