<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/126244/">https://git.reviewboard.kde.org/r/126244/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 4th, 2015, 4:28 p.m. UTC, <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/126244/diff/1/?file=420970#file420970line128" style="color: black; font-weight: bold; text-decoration: underline;">src/plasma/private/packages.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">128</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">package</span><span class="o">-></span><span class="n">metadata</span><span class="p">().</span><span class="n">serviceTypes</span><span class="p">().</span><span class="n">contains</span><span class="p">(</span><span class="s">"Plasma/Containment"</span><span class="p">))</span> <span class="p">{</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;">Having the content structure change when you load a specific package is going to result in weird behaviour. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">package.files() should be usable from just the structure.</p></pre>
</blockquote>
<p>On December 4th, 2015, 4:53 p.m. UTC, <b>Marco Martin</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;">do you think so? I can make this always work but i think is more correct to have the file definition only valid in containments.
this is already used for some things, like fallback packages (like custom shells that fall back onthe desktop shell) and seems to be fine.
what changes is that filePath would change between resolving something and just failing in different packages.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">perhaps some more autotests in kpackage to check this is not having weird side effects?</p></pre>
</blockquote>
<p>On December 10th, 2015, 3:53 p.m. UTC, <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;">Right, but I said .files() will miss something if used without a path and you've responded with something about filePath which is completely different.</p></pre>
</blockquote>
<p>On December 10th, 2015, 4:03 p.m. UTC, <b>Marco Martin</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;">true, that wasn't never really tought to matter much (for wallpaper packages pretty much the same thing happens, file definitions change depending from what's in the package).
if you think it's important, i can add it unconditionally. would just seems weird that this would be available for applets as well</p></pre>
</blockquote>
<p>On December 10th, 2015, 8:42 p.m. UTC, <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;">I'm not too fussed, I just wanted you to have considered it and any possible consequences.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ship it</p></pre>
</blockquote>
<p>On December 10th, 2015, 9:35 p.m. UTC, <b>Marco Martin</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 didn't see particular consequencies, but yeah, if you can find any please shout any moment, can always be changed :)</p></pre>
</blockquote>
<p>On December 10th, 2015, 9:44 p.m. UTC, <b>Marco Martin</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;">uh! idea:) maybe containment packages may be yet another subclass of the package structure with that file more, so it doesn't do weird things when the path changes</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;">gah, nevermind, since the package is loaded in the applet ctor, isContainment() doesn't work yet</p></pre>
<br />
<p>- Marco</p>
<br />
<p>On December 4th, 2015, 6:24 p.m. UTC, Marco Martin 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 Plasma.</div>
<div>By Marco Martin.</div>
<p style="color: grey;"><i>Updated Dec. 4, 2015, 6:24 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;">the CompactApplet file from the shell package defines the behavior of the poopup applets in the panel (it implements an internal dialog and all that jazz)
implementing a simplified systray(both the one for the phone and a separate one for the desktop), i noticed that a containment may have different ideas on how to expand an applet: the systray would have for instance a single popup dialog and put all of its applet full representtions in the same Dialog , this lets containment representation to override that file (<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">if</em> won't get abused, that's the only thing makes me a bit on the fence about this)
It would make possible also fairly different designs that have been proposed in the past, such as the bug sidebar similar to the "charm bar"</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/private/packages.cpp <span style="color: grey">(c04d36d)</span></li>
<li>src/plasma/private/packages_p.h <span style="color: grey">(a7c4cb1)</span></li>
<li>src/plasmaquick/appletquickitem.cpp <span style="color: grey">(efe8611)</span></li>
<li>src/plasmaquick/private/appletquickitem_p.h <span style="color: grey">(79c1a2e)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126244/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>