<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/121775/">https://git.reviewboard.kde.org/r/121775/</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 3rd, 2015, 6:23 p.m. MSK, <b>Milian Wolff</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 wonder why I never saw this issue. Can you explain what you are encountering? Also, adding a unit test would help here to ensure we never run into this issue again.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Generally, the code for checkForDependencies ensures that all dependencies are available and none are disabled. Only then should potential dependencies be loaded. Your code now always tries to load dependencies, no? Even when one of them is disabled or not existing.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">so -1 from my side until you give more information on what issue you are encountering and trying to solve.</p></pre>
</blockquote>
<p>On January 5th, 2015, 4:30 p.m. MSK, <b>Sergey Kalinichev</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;">Generally, the code for checkForDependencies ensures that all dependencies are available and none are disabled.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not exactly. It checks all loaded plugins to see if dependencies satisfied. And returns all interfaces that's not yet loaded/not available</p>
<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;">Only then should potential dependencies be loaded. </p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes.</p>
<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;">Your code now always tries to load dependencies, no?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No. That's what happens now. Run KDevelop with debug channels enabled and search for "Missing dependencies:" messages. You won't find any because checkForDependencies checks all available plugins instead of loaded ones. So at the point after checkForDependencies call there could be some plugins that not yet loaded. And then loadDependencies tries to load all dependendencies (even if some are already loaded), and only because of that everything works.</p>
<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;">I wonder why I never saw this issue.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Because currently everything works</p>
<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;">so -1 from my side until you give more information on what issue you are encountering and trying to solve</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Some plugins didn't load for me. So I began investigating and encountered this weird behaviour. But in the end turned out that they didn't load because weren't rebuild properly.</p>
<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;">Also, adding a unit test would help here to ensure we never run into this issue again.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't think we need this. If it dosn't work some plugins won't load -> some unit tests won't pass.</p></pre>
</blockquote>
<p>On January 5th, 2015, 4:53 p.m. MSK, <b>Milian Wolff</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'm not convinced by your analysis of what checkForDependencies is currently doing. Especially, if everything works, then why do we need to change anything? I don't get any "missing dependencies" because all plugins are normally enabled. If I disable e.g. git, the kdeprovider plugin won't be loaded either. Is that not the case? I can't test right now, but will do so later. Please also test this with your patch.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I still think that if this goes in, it should get a unit test. Failing "integration tests" are much harder to debug, compared to a simple test failure in "TestPluginController::testDependencyLoading".</p></pre>
</blockquote>
<p>On January 5th, 2015, 5:18 p.m. MSK, <b>Sergey Kalinichev</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;">Especially, if everything works, then why do we need to change anything?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Like I already told, it works only by accident.</p>
<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 disable e.g. git, the kdeprovider plugin won't be loaded either. Is that not the case?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, it's.</p>
<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;">I don't get any "missing dependencies" because all plugins are normally enabled.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In your case kdeprovider depends on git. Let's say first kdeprovider is loaded. The controller sees, that it depends on git and it's not yet loaded, so git is a "missing dependency" for kdeprovider and must be loaded. That how it works with the patch.
But now it works like this: First kdeprovider is loaded. The controller sees no dependencies (as checkForDependencies checks all <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">available</em> plugins). And then it tries to load <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">all</em> dependencies anyway(including git)!</p>
<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;">Failing "integration tests" are much harder to debug, compared to a simple test failure in "TestPluginController::testDependencyLoading".</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, I agree. Still I'm very reluctand to do so. Also there is e.g. a standardoutputview test, that'll assert if the plugin is not loaded, so it shouldn't be very hard to find the culprit...
But, if you insist I'll see what I could do.</p></pre>
</blockquote>
<p>On January 5th, 2015, 5:44 p.m. MSK, <b>Milian Wolff</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;">checkForDependencies is simply not taking loaded/unloaded into account, and there is no reason it should. It only cares about enabled/disabled, which is something else alltogether. As you say, the code ensures all dependencies are loaded, which is fine at that point, since we verified before that all dependencies are enabled and can thus be fulfilled.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Why do you think this is an "accident"?</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;">checkForDependencies is simply not taking loaded/unloaded into account, and there is no reason it should. It only cares about enabled/disabled</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ok, but there is already loadDependencies that does exactly the same.</p>
<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;">Why do you think this is an "accident"?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Well, once again, please, look at the code!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Especially at: plugincontroller.cpp\:511 At that point missingInterfaces can never contain anything (because if it contains something checkForDependencies returns false). That should already tell you, that whoever wrote the code didn't fully understand how it works.
Second, with current approach, what is the point in having checkForDependencies? No matter what dependencies it finds, they never used, instead code below loads <strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">all</strong> dependencies anew. Equally well you could simply remove checkForDependencies and call directly loadDependencies, and if can't load something then report an error.</p></pre>
<br />
<p>- Sergey</p>
<br />
<p>On January 5th, 2015, 4:30 p.m. MSK, Sergey Kalinichev 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 KDevelop.</div>
<div>By Sergey Kalinichev.</div>
<p style="color: grey;"><i>Updated Jan. 5, 2015, 4:30 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</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;">It fixes 2 bugs:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">1) checkForDependencies returns false if there are some dependencies, but the code that calls this method threats false return code as an error, so it doesn't try to load any dependencies at all.
2) checkForDependencies must check loaded plugins (instead of all available) for satisfied dependencies. As a result currently it almost always returns true</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So because of these 2 bugs interconnected currently everything works.</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>shell/plugincontroller.h <span style="color: grey">(89b0b8b)</span></li>
<li>shell/plugincontroller.cpp <span style="color: grey">(f70fc64)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121775/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>