<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/126750/">https://git.reviewboard.kde.org/r/126750/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 15th, 2016, 7:20 a.m. UTC, <b>Martin Gräßlin</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The KIconDialog itself works fine, but the "Browse..." sub dialog, which is a grand child of the modal dialog, is opened in the background and cannot be used</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">this sounds like a Qt bug or a KFileDialog bug. The sub dialog should set a transient hint and that's respected by the window manager with and without having the modal flag.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">My suggestion is that we verify it's a bug, locate it and fix it and don't work around it. To verify that it is a bug: xprop and xwininfo of all three windows are needed.</p></pre>
</blockquote>
<p>On January 18th, 2016, 8:36 p.m. UTC, <b>Frank Reininghaus</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 Martin. I'm not familiar with these window management issues, so your feedback is greatly appreciated. I attached the xprop and xwinfo output for the three windows to the bug report: https://bugs.kde.org/show_bug.cgi?id=355310#c8</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It would be great if you could take a look.</p></pre>
</blockquote>
<p>On March 10th, 2016, 7:01 p.m. UTC, <b>Christoph Feck</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;">Martin, does the xinfo attached to the above bug report help to identify the issue?
(point Thomas here, if you are lacking time :)</pre>
</blockquote>
<p>On March 10th, 2016, 7:12 p.m. UTC, <b>Martin Gräßlin</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;">sorry, somehow I missed this one :-(</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The choose icon dialog (0x4c000a9) is a transient for the Add entry dialog (0x4c00020).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The file dialog is a transient for a window (0x4c00005) which is not in the output.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So something sounds wrong here. If I understand it correctly the file dialog should have been a transient for the choose icon dialog?</p></pre>
</blockquote>
<p>On June 18th, 2016, 1:42 a.m. UTC, <b>Christoph Feck</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;">If I understand this patch correctly, this is what it tries to fix? It is possible that this would cause blocking in applications (in particular the Plasma shell), so it needs a review from someone who actually runs Plasma 5 and tries it with this patch applied.</pre>
</blockquote>
<p>On June 18th, 2016, 9:24 a.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;">In KDeclarative we have QML bindings for the IconDialog which we use in Plasma and in there we also use WindowModal by default.</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If I understand this patch correctly, this is what it tries to fix? It is possible that this would cause blocking in applications (in particular the Plasma shell), so it needs a review from someone who actually runs Plasma 5 and tries it with this patch applied.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've just tested Plasma shell with this patch active (Selecting application launcher preferences and launching the browse icon dialog from there) and it does not block the application. It can be used normally. The panel becames grayed however, but it continues working.</p></pre>
<br />
<p>- José</p>
<br />
<p>On January 14th, 2016, 11:36 p.m. UTC, Frank Reininghaus 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.</div>
<div>By Frank Reininghaus.</div>
<p style="color: grey;"><i>Updated Jan. 14, 2016, 11:36 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=355310">355310</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kiconthemes
</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, KIconDialog::showDialog() sets the modality to Qt::NonModal by calling</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">setModal(false);</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I found that this was done in https://quickgit.kde.org/?p=kdelibs.git&a=commit&h=d0d2639c126f88a44c852b738550a9427c6260bb in order to prevent that a modal dialog locks an entire application, such as Plasma.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Unfortunately, this has an unwanted side effect in the "Places" of Dolphin and the file dialog: the KIconDialog is the child of a modal "Add Places Entry" dialog there. The KIconDialog itself works fine, but the "Browse..." sub dialog, which is a grand child of the modal dialog, is opened in the background and cannot be used (it could be that this worked for some reason in Qt4 times - I guess we would have received bug reports about this issue earlier otherwise).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This can be fixed by setting the modality to Qt::WindowModal, which ensures that the dialogs block their respective parents (but not the entire application - that would happen if the modality was set to Qt::ApplicationModal, for example by calling setModal(true)).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Note that there are two setModal(true) calls in the KIconDialog constructors. They have no effect if the dialog is opened with showDialog() (which is what happens if the dialog is opened by clicking a KIconButton) because the modality is overwritten there. I'm not sure though if there are any other uses of KIconDialog which would break if the apparently superfluous calls were removed. This might need further investigation.</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 "Browse..." sub dialog of the icon dialog works fine again in Dolphin and KWrite's file dialog when creating a new "Place".</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I could not test if this affects Plasma somehow because I currently do not have a full self-built Plasma session running. It could probably be checked by opening the "Properties..." of a file in FolderView, clicking the icon, and then opening the "Browse..." sub dialog of the KIconDialog. This should hopefully not lock the entire Plasma session (because the dialogs are window modal, and not application modal). If anyone finds problems with that or has ideas for improvement, please let me know!</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/kicondialog.cpp <span style="color: grey">(cca4ed3)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126750/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>