<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/123764/">https://git.reviewboard.kde.org/r/123764/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 13th, 2015, 7:51 a.m. UTC, <b>Boudewijn Rempt</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;">Thanks! As you said, we're sloppy checking for what particular mouse events mean, but I wonder though whether checking for QEvent::ContextMenu in bool KisDocumentSectionView::viewportEvent(QEvent *e) wouldn't help?</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;">If that could be done it would indeed be a better solution for fixing this particular bug, or at least it wouldn't cause a change in behaviour (however minor). This was actually the first thing I tried.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Sadly, ContextMenu events are processed after MouseButtonPress events, so by the time we know that the context menu was triggered, the icon has already been toggled. If I could inspect the QApplication event queue, this may still be feasible; check if there is a ContextMenu event in the queue and early out, but as far as I know there's no way to inspect the event queue (correct me if I'm wrong, because that would be useful!)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Another thought is whether we want arbitrary mouse clicks to toggle buttons/icons? Typical UI convention dictates that only the primary mouse button does this. Looking at the code it appears that Qt::LeftButton is assumed to be the primary button, but if this is defined somewhere I'd much rather use that!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">An alternative solution is to check for ContextMenu events and simply discard them if MouseButtonPress has been processed. This would mean that the context menu can't be used while the cursor is above an icon. And as mentioned I'm not sure we want to toggle icons on what is traditionally used as the context menu button, unless we have a valid use-case for that.</p></pre>
<br />
<p>- Victor</p>
<br />
<p>On May 13th, 2015, 3:18 a.m. UTC, Victor Wåhlström 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 Calligra and Boudewijn Rempt.</div>
<div>By Victor Wåhlström.</div>
<p style="color: grey;"><i>Updated May 13, 2015, 3:18 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=347541">347541</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
calligra
</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 patch limits mouse click interaction with icons to left mouse button.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Note: This fix assumes that context menu events are triggered via right mouse button. This is in theory OS specific, but it looks like other code is already making this assumption.</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;">I haven't done extensive testing outside of the layers widget. Worst case it'll disable middle mouse button and right mouse button as alternatives for clicking on icons, which should be fine.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Context menu events are handled elsewhere and still works as expected.</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>krita/ui/KisDocumentSectionDelegate.cpp <span style="color: grey">(2efde33)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123764/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>