<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/115604/">https://git.reviewboard.kde.org/r/115604/</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;">I initially thought I would reject this patch because, as I commented in the bug report, I would like to see this fixed by removing event loop nesting. But after taking a look at the patch, I like it a lot: it is an elegant way to deal with event loop nesting and realistically we might never have more than one level of nesting so dealing with it this way should be enough.
Yet, the fact that your implementation of KigPart::queryClose() will only return true made me wonder what's the default behavior and made me rethink the whole queryClose situation as it is now and I came up with this idea: why don't we forward the whole Kig::queryClose() to KigPart::queryClose() which will in turn forward to ReadWritePart::queryClose() and if this one returns true, then KigPart::queryClose() checks if we are in the middle of a construction (just like your patch is checking that), cancels it and then return whatever ReadWrite::queryClose() returned... makes sense? I haven't thought deeply about it but from what I see in ReadWritePart's code and what we have in Kig::queryClose I think the behavior should be the same.
Would you have time to look into this?</pre>
<br />
<p>- David Narváez</p>
<br />
<p>On February 9th, 2014, 10:20 p.m. UTC, Jacob Welsh wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Edu.</div>
<div>By Jacob Welsh.</div>
<p style="color: grey;"><i>Updated Feb. 9, 2014, 10:20 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="http://bugs.kde.org/show_bug.cgi?id=173384">173384</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kig
</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;">Modification of three year old patch from Christoph Feck. See bug for details.</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;">Attempted to quit the application while in a construction mode, both though close button and Quit action. With the patch, it quits without crashing. Also tested with unsaved changes; the save dialog continues to work as expected.</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>kig/kig.cpp <span style="color: grey">(b3f5c8b)</span></li>
<li>kig/kig_part.h <span style="color: grey">(7a9b151)</span></li>
<li>kig/kig_part.cpp <span style="color: grey">(17eb0f2)</span></li>
<li>modes/mode.cc <span style="color: grey">(cb5b932)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115604/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>