<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/125433/">https://git.reviewboard.kde.org/r/125433/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 28th, 2015, 7:02 a.m. UTC, <b>Marcus Meissner</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;">according to git blame I added this to help autoload the camera list ... although I have not much recall of this :/</p></pre>
</blockquote>
<p>On September 28th, 2015, 7:30 a.m. UTC, <b>Christian Butcher</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;">A run of 'gdb systemsettings5' with 'break KKameraConfig::load()' and corresponding calls to 'bt' informs me that with the load() call inside displayGPSuccessDialogue, first it is called by the displayGPSuccessDialogue:</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%">#0 KKameraConfig::load (this=0x9d3310) at /home/christian/kde-git/Extras/kamera/kcontrol/kamera.cpp:203
#1 0x00007fffd3e08bec in KKameraConfig::displayGPSuccessDialogue (this=0x9d3310)
at /home/christian/kde-git/Extras/kamera/kcontrol/kamera.cpp:171
#2 0x00007fffd3e07c04 in KKameraConfig::KKameraConfig (this=0x9d3310, parent=0x9c3240)
at /home/christian/kde-git/Extras/kamera/kcontrol/kamera.cpp:67
#3 0x00007fffd3e0c89e in KPluginFactory::createInstance<KKameraConfig, QWidget> (parentWidget=0x0, parent=0x9c3240, args=...)
at /opt/kde/include/KF5/KCoreAddons/kpluginfactory.h:477
#4 0x00007ffff34df827 in KPluginFactory::create(char const*, QWidget*, QObject*, QList<QVariant> const&, QString const&) ()
from /opt/kde/lib64/libKF5CoreAddons.so.5
#5 0x00007ffff798ed10 in KCModuleLoader::loadModule(KCModuleInfo const&, KCModuleLoader::ErrorReporting, QWidget*, QStringList const&) () from /opt/kde/lib64/libKF5KCMUtils.so.5
#6 0x00007ffff799785c in KCModuleProxyPrivate::loadModule() () from /opt/kde/lib64/libKF5KCMUtils.so.5
...
#67 0x00.... in main ()
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">and then again by KCModule::qt_static_metacall:</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%">#0 KKameraConfig::load (this=0x9d3310) at /home/christian/kde-git/Extras/kamera/kcontrol/kamera.cpp:203
#1 0x00007ffff6a4e9d9 in KCModule::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) ()
from /opt/kde/lib64/libKF5ConfigWidgets.so.5
#2 0x00007ffff3075832 in QObject::event(QEvent*) () from /opt/qt5/lib64/libQt5Core.so.5
#3 0x00007ffff4be74c7 in QWidget::event(QEvent*) () from /opt/qt5/lib64/libQt5Widgets.so.5
#4 0x00007ffff4ba699c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /opt/qt5/lib64/libQt5Widgets.so.5
#5 0x00007ffff4babb40 in QApplication::notify(QObject*, QEvent*) () from /opt/qt5/lib64/libQt5Widgets.so.5
#6 0x00007ffff3046ae3 in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /opt/qt5/lib64/libQt5Core.so.5
#7 0x00007ffff3048d53 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) ()
from /opt/qt5/lib64/libQt5Core.so.5
#8 0x00007ffff309bac3 in ?? () from /opt/qt5/lib64/libQt5Core.so.5
#9 0x00007fffed7b778d in g_main_dispatch (context=0x7fffdc0016f0) at ../../glib/gmain.c:3122
#10 g_main_context_dispatch (context=context@entry=0x7fffdc0016f0) at ../../glib/gmain.c:3737
#11 0x00007fffed7b7a38 in g_main_context_iterate (context=context@entry=0x7fffdc0016f0, block=block@entry=1,
dispatch=dispatch@entry=1, self=<optimized out>) at ../../glib/gmain.c:3808
#12 0x00007fffed7b7adc in g_main_context_iteration (context=0x7fffdc0016f0, may_block=1) at ../../glib/gmain.c:3869
#13 0x00007ffff309beb7 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) ()
from /opt/qt5/lib64/libQt5Core.so.5
#14 0x00007ffff3044682 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /opt/qt5/lib64/libQt5Core.so.5
#15 0x00007ffff304c18d in QCoreApplication::exec() () from /opt/qt5/lib64/libQt5Core.so.5
#16 0x000000000040f108 in main ()
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I would guess that the second call occurs immediately after (but I didn't step through, so I could be horribly wrong). </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">load() does indeed call populateDeviceListView(), then gp_list_free(list). gp_list_new(&list) is called in the same function, so I don't think there's any risk of double free, but something strange happens.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If I watch carefully, sometimes the correct kcm loads and is then replaced by the scrollbars between debug statements to 'kamera.kcontrol' in ~/.xsession-errors (these debug statements aren't in the git repository since they're fairly useless for general use I imagine). I'm sure the reliability of 'tail -f' for judging timing is pretty non-existent but I find it qualitatively convincing.</p></pre>
</blockquote>
<p>On September 28th, 2015, 6:47 p.m. UTC, <b>Marcus Meissner</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;">I tried to comment it out, it does not seem to change behaviour so it seems good.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(the config dialog is broken though... :( )</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;">Should I commit? (I added you as a reviewer also.)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How do you mean broken? If you mean it remains empty, then obviously I shouldn't commit, and should work out why it fixes for me, but not you.
If something else, perhaps I can look at that next?</p></pre>
<br />
<p>- Christian</p>
<br />
<p>On October 8th, 2015, 7:22 a.m. UTC, Christian Butcher 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 Graphics, Plasma and Marcus Meissner.</div>
<div>By Christian Butcher.</div>
<p style="color: grey;"><i>Updated Oct. 8, 2015, 7:22 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kamera
</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;">The displayGPSuccessDialogue(void) function calls load() as its last statement currently.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Documentation for the KCModule api (both <a href="http://api.kde.org/4.x-api/kdelibs-apidocs/kdeui/html/classKCModule.html" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">kde4</a> and <a href="http://api.kde.org/frameworks-api/frameworks5-apidocs/kconfigwidgets/html/classKCModule.html" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">frameworks</a>) informs the reader that load() is called at the end of construction - consequently it seems that the additional load() call leads to the function (which is implemented as KKameraConfig::load() ) being called twice.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><a href="https://bugs.kde.org/show_bug.cgi?id=236844" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Bug 236844</a> may be related but at least on my KF5 based system, this does not cause or prevent a crash, which is referenced in the bug.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Separately, COPYING-CMAKE-SCRIPTS is referenced by the FindGphoto2.cmake file, and seems to have missed the commit in <a href="https://git.reviewboard.kde.org/r/125230/" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">RR-125230</a>. If this was intentional, then my apologies for readding here, but if so, the FindGphoto2.cmake file will need changing in some way.</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;">When the call is present, opening the Kamera KCM within systemsettings after saving changes and going back to the main systemsettings view leads to a strange set of scrollbars.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">With the additional call removed, the KCM-Kamera opens properly on the second attempt, after saving changes.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As a camera to test, the first (in the scroll list) serial port camera is 'Achiever Digital Adc65'. 'Barbie' may be easier to find as the first entry for 'B', and is also more memorable... I used a serial camera, since then the kcm will load images even without a camera connected</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>COPYING-CMAKE-SCRIPTS <span style="color: grey">(PRE-CREATION)</span></li>
<li>kcontrol/kamera.cpp <span style="color: grey">(2d8f61449a5a6f2650d864c864652ebea780a422)</span></li>
<li>kcontrol/kameradevice.h <span style="color: grey">(b0ecb974f8fcb765228afe1965aeebab8e7656ed)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125433/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/09/28/74e8bd93-4c4a-4103-9fb1-7bd106258058__kcm1.png">scrollbars</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>