<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/126993/">https://git.reviewboard.kde.org/r/126993/</a>
</td>
</tr>
</table>
<br />
<p>
Ship it!
</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ship It!</pre>
<br />
<p>- Aleix Pol Gonzalez</p>
<br />
<p>On February 5th, 2016, 6:18 p.m. CET, 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 Plasma.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Feb. 5, 2016, 6:18 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-workspace
</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;">While looking to build DrKonqi for OS X/MacPorts I came across a few issues that appear to be syntax/coding errors that, surprisingly, were never caught before given that they also caused compile failures when I used the same configure/build/packaging script on Linux.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The code uses methods that no longer exist in Qt 5.5+ : <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QString::fromAscii()</code>, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QHeaderView::setResizeMod()</code> (replaced with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QHeaderView::setSectionResizeMode()</code> and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QInputDialog::getInteger()</code> (instead of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QInputDialog::getInt()</code>). Those are the easy and straightforward ones fixed by this patch.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The handling of file dialogs (in 2 locations) was more problematic. Both Apple's clang "6.0.0" and gcc 5.3 (on Linux) refused statements like <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QWeakPointer<QFileDialog> dlg = new QFileDialog(parent,defname);</code> . My initial attempt was to follow the documentation for QWeakPointer, and do <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QWeakPointer<QFileDialog> dlg = QSharedPointer<QFileDialog>(new QFileDialog(parent,defname));</code> but that caused a crash as soon as I clicked on the "save backtrace as" button - on both platforms.
It turned out that <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QSharedPointer<QFileDialog>()</code> returned a valid (non-null) shared pointer, but after creating a QWeakPointer from it, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">dlg.data()</code> returned NULL -- immediately.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I had a look around in the 5.17.0 frameworks codebase, and could find no relevant examples of using QWeakPointer; some expressions appear to be equivalent to the original DrKonqi code, in other locations QWeakPointer instances are indeed created "going through" a QSharedPointer. In the end I tested using QFileDialog (and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QFileDialog::selectedUrls()</code> as kate5 does, and that works (on OS X and Linux).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That's what the attached patch does: no QFileDialog pointer at all, just an instance.
This RR is intended to fuel a discussion on the subject, as suggested by Aleix; therefore I left the comment about the possibility of invalidating/deleting the file dialog e.g. through a DBus call. Is that actually a possibility, and why wouldn't it be in Kate? Are there no other ways to prevent it, or check for its occurrence?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch also marks drkonqi as a nongui executable, a requirement on OS X and which shouldn't cause any regressions elsewhere AFAIK.</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.1 and Kubuntu 14.04, both with Qt 5.5.1 and KF5 5.17.0 installed into /opt/local</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>drkonqi/CMakeLists.txt <span style="color: grey">(deb8c40)</span></li>
<li>drkonqi/bugzillaintegration/bugzillalib.cpp <span style="color: grey">(802c5fb)</span></li>
<li>drkonqi/bugzillaintegration/reportassistantpages_bugzilla.cpp <span style="color: grey">(e60fb14)</span></li>
<li>drkonqi/bugzillaintegration/reportassistantpages_bugzilla_duplicates.cpp <span style="color: grey">(4f8f4ea)</span></li>
<li>drkonqi/drkonqi.cpp <span style="color: grey">(b12c118)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126993/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>