<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/112447/">http://git.reviewboard.kde.org/r/112447/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">the Plasma/Shell package definition should live in libplasma along with the others. this will remove the need for the PluginLoader subclass and ensure that Corona can be used by any application that simply links to libplasma, which is obviously a requirement.
the other classes should perhaps find a more appropriate home as well rather than creating a whole new library. really, this is not the "Plasma View Library" but the "if you are writing a QML shell with libplasma you will find these couple of classes useful..." library. most optimal would be if this could be tucked away with the rest of the QML support for plasma, perhaps in the script engine library itself.
the config classes also scream out to be made internal and not public API. as the main class used is a QQuickView, putting this in the QML support would be sensible and then all of this could be nicely hidden away.
the classes also seem to be in need of some API review before we commit to binary compatibility for them ... something for one of the upcoming monday meetings?</pre>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/112447/diff/1/?file=186293#file186293line169" style="color: black; font-weight: bold; text-decoration: underline;">src/plasmaview/configview.h</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">169</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1">//</span><span class="cs">TODO</span><span class="c1">: the config view for the containment should be a subclass</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">this TODO seems to be done</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/112447/diff/1/?file=186293#file186293line170" style="color: black; font-weight: bold; text-decoration: underline;">src/plasmaview/configview.h</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">170</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1">//</span><span class="cs">TODO</span><span class="c1">: is it possible to move this in the shell?</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">is this TODO still valid given that this code is moving into a library?</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/112447/diff/1/?file=186294#file186294line259" style="color: black; font-weight: bold; text-decoration: underline;">src/plasmaview/configview.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">259</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">qDebug</span><span class="p">()</span> <span class="o"><<</span> <span class="s">" XXX loaded QALM"</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">seems less than useful debug output at this point ...</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/112447/diff/1/?file=186295#file186295line34" style="color: black; font-weight: bold; text-decoration: underline;">src/plasmaview/containmentconfigview_p.h</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">34</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c1">//</span><span class="cs">TODO</span><span class="c1">: is it possible to move this in the shell?</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">another stray TODO?</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/112447/diff/1/?file=186296#file186296line51" style="color: black; font-weight: bold; text-decoration: underline;">src/plasmaview/containmentconfigview_p.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">51</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Plasma</span><span class="o">::</span><span class="n">Package</span> <span class="n">pkg</span> <span class="o">=</span> <span class="n">Plasma</span><span class="o">::</span><span class="n">PluginLoader</span><span class="o">::</span><span class="n">self</span><span class="p">()</span><span class="o">-></span><span class="n">loadPackage</span><span class="p">(</span><span class="s">"Plasma/Generic"</span><span class="p">);</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">52</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">pkg</span><span class="p">.</span><span class="n">setDefaultPackageRoot</span><span class="p">(</span><span class="s">"plasma/wallpapers"</span><span class="p">);</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">53</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">pkg</span><span class="p">.</span><span class="n">setPath</span><span class="p">(</span><span class="n">m_containment</span><span class="o">-></span><span class="n">wallpaper</span><span class="p">());</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">54</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QFile</span> <span class="nf">file</span><span class="p">(</span><span class="n">pkg</span><span class="p">.</span><span class="n">filePath</span><span class="p">(</span><span class="s">"config"</span><span class="p">,</span> <span class="s">"main.xml"</span><span class="p">));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">why is this using Plasma/Generic instead of a Plasma/QmlWallpaper (or whatever) package?
sprinkling our code with setDefaultPackageRoot and filePath calls with literal values defeats the purpose of Plasma::Package: to encapsulate those details.
if we were to move where the wallpaper packages were kept, then what? we search every single bit of code that might use wallpapers?
no, that's why we have Plasma::Package subclasses! there should be a package type for these wallpapers.</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/112447/diff/1/?file=186296#file186296line67" style="color: black; font-weight: bold; text-decoration: underline;">src/plasmaview/containmentconfigview_p.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">67</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">setSource</span><span class="p">(</span><span class="n">QUrl</span><span class="o">::</span><span class="n">fromLocalFile</span><span class="p">(</span><span class="n">m_containment</span><span class="o">-></span><span class="n">containment</span><span class="p">()</span><span class="o">-></span><span class="n">corona</span><span class="p">()</span><span class="o">-></span><span class="n">package</span><span class="p">().</span><span class="n">filePath</span><span class="p">(</span><span class="s">"containmentconfigurationui"</span><span class="p">)));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">why calling m_containmnet->containment()?
it already is a containment, so ... if !m_containment->isContainment() we have a problem so perhaps that could be added as an assert in the ctor, but calling m_containment->containment() should never be necessary when we are dealing with a Plasma::Containment*</pre>
</div>
<br />
<p>- Aaron J.</p>
<br />
<p>On September 2nd, 2013, 12:53 p.m. UTC, Giorgos Tsiapaliokas wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Plasma.</div>
<div>By Giorgos Tsiapaliokas.</div>
<p style="color: grey;"><i>Updated Sept. 2, 2013, 12:53 p.m.</i></p>
<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;">This patch creates a new library out of the shell dir.
We need this library in order to implement plasmoidviewer 2.0</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;">The code is locate at git@git.kde.org:clones/plasma-framework/tsiapaliwkas/pf5.git .
how to test it
* git clone git@git.kde.org:clones/plasma-framework/tsiapaliwkas/pf5.git
* cd pf5
* git checkout split7
* build it and install it
* use plasma-shell
I haven't noticed any issues.</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/CMakeLists.txt <span style="color: grey">(281d146)</span></li>
<li>src/plasmaview/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/PlasmaViewConfig.cmake.in <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/configview.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/configview.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/containmentconfigview_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/containmentconfigview_p.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/currentcontainmentactionsmodel_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/currentcontainmentactionsmodel_p.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/includes/PlasmaView/ConfigView <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/includes/PlasmaView/ContainmentConfigView <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/includes/PlasmaView/ShellPluginLoader <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/includes/PlasmaView/View <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/shellpackage_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/shellpackage_p.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/shellpluginloader.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/shellpluginloader.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/view.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasmaview/view.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/shell/CMakeLists.txt <span style="color: grey">(3da019f)</span></li>
<li>src/shell/configview.h <span style="color: grey">(2e8f68f)</span></li>
<li>src/shell/configview.cpp <span style="color: grey">(fea5a73)</span></li>
<li>src/shell/containmentconfigview.h <span style="color: grey">(619fa14)</span></li>
<li>src/shell/containmentconfigview.cpp <span style="color: grey">(235a33f)</span></li>
<li>src/shell/currentcontainmentactionsmodel.h <span style="color: grey">(db94da1)</span></li>
<li>src/shell/currentcontainmentactionsmodel.cpp <span style="color: grey">(c955bef)</span></li>
<li>src/shell/shellcorona.cpp <span style="color: grey">(ffdbfe8)</span></li>
<li>src/shell/shellpackage.h <span style="color: grey">(99dc460)</span></li>
<li>src/shell/shellpackage.cpp <span style="color: grey">(74aea5c)</span></li>
<li>src/shell/shellpluginloader.h <span style="color: grey">(1d3dade)</span></li>
<li>src/shell/shellpluginloader.cpp <span style="color: grey">(8b2e1dd)</span></li>
<li>src/shell/view.h <span style="color: grey">(7e6b2d9)</span></li>
<li>src/shell/view.cpp <span style="color: grey">(a0c6168)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/112447/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>