<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 5th, 2016, 3:57 p.m. CET, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/126993/diff/1/?file=442765#file442765line183" style="color: black; font-weight: bold; text-decoration: underline;">drkonqi/drkonqi.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">179</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1">// when using QWeakPointer<QFileDialog> dlg, the code did this check - is that really a possible occurrence?</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">180</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1">// if (dlg.isNull()) {</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">181</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1">// //Dialog is invalid, it was probably deleted (ex. via DBus call)</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">182</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1">// //return and do not crash</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">183</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1">// return;</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">184</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1">// }</span></pre></td>
</tr>
</tbody>
</table>
<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;">yes this is possible and there is even a Krazy check for this. The QWeakPointer is the proper fix to this problem.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What exactly was the compile issue you run into with the QWeakPointer?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I assume that:
QWeakPointer<QFileDialog> dlg(new QFileDialog(parent, defname));</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">would solve your compile error.</p></pre>
</blockquote>
<p>On February 5th, 2016, 4:01 p.m. CET, <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;">found the explanation: https://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0</p></pre>
</blockquote>
<p>On February 5th, 2016, 4:35 p.m. CET, <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;">Ah, yes, I have already seen similar situations where the possibiity of using the Quit menu on OS X was clearly something that the application didn't foresee. I must admit I have been shrugging that off.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">According to Frank's blog:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">If was deleted during the exec(), exec() will return QDialog::Rejected.
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Isn't that still the case? I was counting on it, and if it is true my modification should be fine (and have the advantage of being less complex). The code doesn't use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">dlg</code> if <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">exec()</code> returned anything other than <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QDialog::Accepted</code>.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also: DrKonqi doesn't seem to register on the DBus at all. At least not under its own name (there's just a ToDo note about implementing a proper DBus interface, who knows how old).</p></pre>
</blockquote>
<p>On February 5th, 2016, 5:06 p.m. CET, <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;">To answer the question: I don't have the exact error message anymore, but it was about an impossible assignment because of a missing ctor.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As to your assumed fix: looks like it gives just about the same error as the original code did:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">cd $build.dir <span style="color: #666666">&&</span> <span style="border: 1px solid #FF0000">/usr/bin/c++ -DKCOREADDONS_LIB -DKDE_DEFAULT_DEBUG_AREA=1410 -DPROJECT_VERSION=\"5.5.4\" -DQT_CORE_LIB -DQT_DBUS_LIB -DQT_GUI_LIB -DQT_NO_DEBUG -DQT_WIDGETS_LIB -DQT_X11EXTRAS_LIB -DQT_XML_LIB -D_GNU_SOURCE -D_LARGEFILE64_SOURCE -I<options> -isystem /opt/local/include/KF5/KI18n -isystem /opt/local/include/KF5 -isystem /opt/local/include/qt5 -isystem /opt/local/include/qt5/QtCore -isystem /opt/local/share/qt5/mkspecs/linux-g++-64 -isystem /opt/local/include/KF5/KCoreAddons -isystem /opt/local/include/KF5/KService -isystem /opt/local/include/KF5/KConfigCore -isystem /opt/local/include/KF5/KConfigWidgets -isystem /opt/local/include/KF5/KCodecs -isystem /opt/local/include/KF5/KWidgetsAddons -isystem /opt/local/include/qt5/QtWidgets -isystem /opt/local/include/qt5/QtGui -isystem /opt/local/include/KF5/KConfigGui -isystem /opt/local/include/qt5/QtXml -isystem /opt/local/include/KF5/KAuth -isystem /opt/local/include/KF5/KJobWidgets -isystem /opt/local/include/KF5/KIOCore -isystem /opt/local/include/KF5/KCrash -isystem /opt/local/include/KF5/KCompletion -isystem /opt/local/include/qt5/QtDBus -isystem /opt/local/include/KF5/KXmlRpcClient -isystem /opt/local/include/KF5/KXmlRpcClient/kxmlrpcclient -isystem /opt/local/include/KF5/KWallet -isystem /opt/local/include/qt5/QtX11Extras -O3 -march=native -g -DNDEBUG -std=c++0x -fno-exceptions -Wall -Wextra -Wcast-align -Wchar-subscripts -Wformat-security -Wno-long-long -Wpointer-arith -Wundef -Wnon-virtual-dtor -Woverloaded-virtual -Werror=return-type -fPIC -o CMakeFiles/drkonqi.dir/drkonqi.cpp.o -c /.../plasma-workspace-5.5.4/drkonqi/drkonqi.cpp</span>
<span style="border: 1px solid #FF0000">/.../plasma-workspace-5.5.4/drkonqi/drkonqi.cpp:169:71: error: no matching function for call to ‘QWeakPointer<QFileDialog>::QWeakPointer(QFileDialog*)’</span>
QWeakPointer<span style="color: #666666"><</span>QFileDialog<span style="color: #666666">></span> dlg(<span style="color: #008000; font-weight: bold">new</span> QFileDialog(parent, defname));
<span style="color: #666666">^</span>
In file included from <span style="color: #666666">/</span>opt<span style="color: #666666">/</span>local<span style="color: #666666">/</span>include<span style="color: #666666">/</span>qt5<span style="color: #666666">/</span>QtCore<span style="color: #666666">/</span>qsharedpointer.h<span style="color: #666666">:42:0</span>,
from <span style="color: #666666">/</span>opt<span style="color: #666666">/</span>local<span style="color: #666666">/</span>include<span style="color: #666666">/</span>qt5<span style="color: #666666">/</span>QtCore<span style="color: #666666">/</span>QWeakPointer<span style="color: #666666">:1</span>,
from <span style="color: #666666">/</span>...<span style="color: #666666">/</span>plasma<span style="color: #666666">-</span>workspace<span style="color: #666666">-5.5</span>.<span style="color: #666666">4/</span>drkonqi<span style="color: #666666">/</span>drkonqi.cpp<span style="color: #666666">:44:</span>
<span style="border: 1px solid #FF0000">/opt/local/include/qt5/QtCore/qsharedpointer_impl.h:696:12: note: candidate: template<class X> QWeakPointer<T>::QWeakPointer(X*, bool)</span>
inline QWeakPointer(X <span style="color: #666666">*</span>ptr, bool) <span style="color: #666666">:</span> d(ptr <span style="color: #666666">?</span> Data<span style="color: #666666">::</span>getAndRef(ptr) <span style="color: #666666">:</span> <span style="color: #666666">0</span>), value(ptr)
<span style="color: #666666"><</span>snip<span style="color: #666666">></span>
</pre></div>
</p></pre>
</blockquote>
<p>On February 5th, 2016, 5:07 p.m. CET, <b>Aleix Pol Gonzalez</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;">Use QPointer.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">QPointer<QFileDialog> . Yes, that seems to work.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Let's say that this code will be ready the day someone implements that org.kde.drkonqi service ...</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On February 5th, 2016, 3:13 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, 3:13 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>