<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/119988/">https://git.reviewboard.kde.org/r/119988/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 29th, 2014, 2 p.m. CEST, <b>David Edmundson</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/119988/diff/1/?file=308188#file308188line99" style="color: black; font-weight: bold; text-decoration: underline;">src/plasma/packagestructure.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">private:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">98</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#define K_EXPORT_PLASMA_PACKAGE(libname, classname) \</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">99</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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;">would removing a #define count as a SIC?</p></pre>
</blockquote>
<p>On August 29th, 2014, 2:12 p.m. CEST, <b>Aaron J. Seigo</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;">Yes, but I don't think it maters for two reasons:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">1) DataEngine also had both forms but only one actually currently works; so it is on the face of it <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">broken</em> .. removing that is the same as this change at the end of the day<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
2) There are no Plasma 5 PackageStructure plugins out there except the share dataengine in plasma-workspace (which itself was done in a very odd manner)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Neither reason is particularly <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">good</em> from the perspective of strict policy adherence, but they demonstrate that it is a harmless violation. May as well get it right imho.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'd add a third consideration as well: It is quite evident that plasma-framework was release before it was ready. The plugin loading is entirely inconsistent with DataEngines using JSON and the Qt5 plugin loading an the rest of the plugins not. Some of the plugins are obviously not being used at all due to changes brought about with QML2 and that probably contributed to the lack of attention. However, PluginLoader quite obviously went through a partial refactoring that was never completed. It's a little brash to commit to source and binary compatibility when things are in that shape.</p></pre>
</blockquote>
<p>On August 29th, 2014, 2:21 p.m. CEST, <b>Hrvoje Senjan</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;">Yes, but I don't think it maters for two reasons</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Can users of Plasma 5.0.x can expect a broken desktop once KF5 5.2 is out (if this patch goes in)?</p></pre>
</blockquote>
<p>On August 29th, 2014, 2:25 p.m. CEST, <b>David Edmundson</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;">Whilst I agree that the frameworks situation sucks, I'm not sure we have a choice.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Frameworks 5.2 releases independently of plasma-workspace. At which point there is a point in time where one can't compile plasma-workspace.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
We could make it go via a no-op that does nothing.</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;">if one doesn't recompile it should still work, shouldn't it? Otherwise I'd suggest to wrap the define in a ifndef NO_DEPRECATED block to keep SIC (even if it means that we carry deprecated stuff for the time being, but after all that's the point of deprecation).</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On August 29th, 2014, 1:51 p.m. CEST, Aaron J. Seigo 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 Frameworks and Plasma.</div>
<div>By Aaron J. Seigo.</div>
<p style="color: grey;"><i>Updated Aug. 29, 2014, 1:51 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-framework
</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;">This patch set does the following:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">tidy up the data engine plugin loading code</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">make PackageStructure plugins use the json method as with DataEngines</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">remove ShellPackage; it moves to live with plasmashell (review #119989)</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The goal here is to get rid of the plasmaquick library as much as possible. It was unnecessary in the first place since PackageStructure supports plugins. The only potentially controversial change here is to move PackageStructure to use the json-based plugin loading. That seems to be the more modern approach, but plugin loading in libplasma is currently a mix of the old and the new. As PackageStructure changed API in plasma-framework, meaning any existing plugins from 4.x would need updating anyways, this seems a safe enough change to make as it should impact exactly zero plugins out there currently.</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;">Ran a full Plasma Desktop session.</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>src/plasma/packagestructure.h <span style="color: grey">(fb32c22a6e7df1528b3d7a5b30b94c60a85a93e3)</span></li>
<li>src/plasma/pluginloader.cpp <span style="color: grey">(d2ba5ca2d3a96fe6f1ce26be41df3b0954b924df)</span></li>
<li>src/plasma/private/packages.cpp <span style="color: grey">(5eb6f0021392257634dfd958c940b2945989e48b)</span></li>
<li>src/plasma/private/packages_p.h <span style="color: grey">(0833a4ed1b5704efffccade5e52589878e8b4957)</span></li>
<li>src/plasmaquick/CMakeLists.txt <span style="color: grey">(1ed7c67efcba0e6dbef1ff929b176090786503de)</span></li>
<li>src/plasmaquick/private/packages.h <span style="color: grey">(7498832d0537611903c13e544db6486bab163dd3)</span></li>
<li>src/plasmaquick/private/packages.cpp <span style="color: grey">(52758482230d271712e4bb3b6d33f8fdeaa848a8)</span></li>
<li>src/plasmaquick/shellpluginloader.h <span style="color: grey">(6c56e5f7b269c3af7587a58cbe104468a2c679c4)</span></li>
<li>src/plasmaquick/shellpluginloader.cpp <span style="color: grey">(2824760e6f64a694bd14b46d2f80151304e3e4d3)</span></li>
<li>src/plasma/dataengine.h <span style="color: grey">(d87a6f8361c892a249374a5c9da7e84836195156)</span></li>
<li>src/plasma/package.cpp <span style="color: grey">(6ad332167bb83c2f794f9f5d059e9f369ad33841)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119988/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>