<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/120029/">https://git.reviewboard.kde.org/r/120029/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On settembre 1st, 2014, 4: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;">I need some the concept explaining, why would a developer set a fallbackpackage?<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Is it not always org.kde.breeze.desktop?</p></pre>
</blockquote>
<p>On settembre 1st, 2014, 5:18 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;">It would usually be the packagestructure setting it, in initPackage(), so setFallbackPackage follows the same pattern as addFileDefinition(), setRequired() and the likes.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">so, for the packages of type Plasma/LookAndFeel it would be org.kde.breeze.desktop<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
for packages of types Plasma/Shell, it would be org.kde.desktop</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">even tough at some point it will probably have to be something more complicated, like the default lookandfeel one depending from the current shell, if ever needed, will be feasible.</p></pre>
</blockquote>
<p>On settembre 1st, 2014, 6:31 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;">Ah, makes sense. Thanks.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Do you really expect a linked link big enough to cycle? I can't really see it going 3 deep. The cycle check is call cool though :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Could you add a warning in setFallbackPackage() if it finds a cycle and no-ops. Could be confusing to debug otherwise.</p></pre>
</blockquote>
<p>On settembre 1st, 2014, 6:38 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;">added warning..<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
i see it very unlikely that it's going to go deep, but better safe than sorry :p</p></pre>
</blockquote>
<p>On settembre 3rd, 2014, 6:39 a.m. UTC, <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;">"Is it not always org.kde.breeze.desktop?"</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No, because of third party developers. Not every project targets the desktop.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That said, this applies to any use of Plasma::Package, which is meant to be a generic concept not something just for the limited set of package types plasmashell uses. It is helpful to think of it as a concept on its own rather than in the context of Plasma Desktop. Other Frameworks using applications really ought to be able to use Plasma::Package for their "bundles of data" needs.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"It would usually be the packagestructure setting it, in initPackage()"</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In the case of Shell and Look & Feel packages I would caution against doing this, as it biases them to Plasma Desktop. The tablet interface in Plasma Active will need its own "root" L&F; falling back to components from breeze.desktop will likely be both an oversight and create undesirable results. It also would mean that every time someone touches a component in breeze.desktop that is used by the tablet UX, they will be required to ensure it remains appropriate on touch. For devices even further afield from desktop than tablets this becomes even more of an issue, and those devices may not want to ship breeze.desktop at all. Of course, there is the alternative which is to just tell system integrators to call their root package "org.kde.breeze.desktop" but I'm not sure that would be desirable.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">While setting this in initPackage is a reasonable stop-gap for now, this really ought to be something that gets defined in e.g. the shell package and set in plasmashell so that it can be tied to the shell. If no fallback is defined in the shell package, fair enough -> no fallback :)</p></pre>
</blockquote>
<p>On settembre 3rd, 2014, 8:34 a.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;">"While setting this in initPackage is a reasonable stop-gap for now"<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
for now it does that, I think it's fine for a first step to get rid of lookandfeelaccess. I would advise to provide a full complete package anyways, soat start wouldn't matter much.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Then, after this, how to make it depend from the shell? it may depend on the shell kded, but it would depend from quite a lot of boilerplate, also it should stay possible to configure it, so in the config file it would end up with pairs shell/l&f packages i guess.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Or, to make it a bit more primitive, but still functional for the scope, the fallback could be from a configuration file as well, so integrators could just ship such fine.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
For the shell package instead, the fallback <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">may</em> be defined in its metadata desktop file? (not sure this could really work for the l&f one)</p></pre>
</blockquote>
<p>On settembre 3rd, 2014, 9:16 a.m. UTC, <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;">"how to make it depend from the shell?"</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Looking only at the question of default fallback (not user config) then your suggestion should work:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"the fallback may be defined in its metadata desktop file?"</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This would indeed allow L&F customizations to define the source theme as its fallback. (We'll ignore dependency resolution..) The default fallback could be defined in the shell package's metadata file (so org.kde.desktop would set that to org.kde.breeze.desktop, e.g.) so L&F without a defined fallback would get the default fallback.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For shell packages, I would agree that having complete packages is desirable and safest and there should perhaps be no default fallback for them. All or nothing, or else an explicitly defined fallback in its metadata (as with any other package supporting fallbacks)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"it would depend from quite a lot of boilerplate"</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The default fallback should be determined in one central place (plasmashell or an external daemon, exposing the setting as a DBus property on a well-known DBus service?) which only leaves applying it appropriately wherever these packages are used. That indeed means some boilerplate, but <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">that</em> could go into the Plasma/LookAndFeel package structure. So it wouldn't have a literal value, but the logic necessary to fetch what the current fallback value is and apply it.</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;">"That indeed means some boilerplate, but that could go into the Plasma/LookAndFeel package structure"<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
hmm for that I can think it could make sense having the structure changing the path of all its packages after a signal from given kded..<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
but the structure bookeeping the packages kinda stinks..</p></pre>
<br />
<p>- Marco</p>
<br />
<p>On settembre 1st, 2014, 7:04 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 KDE Frameworks and Plasma.</div>
<div>By Marco Martin.</div>
<p style="color: grey;"><i>Updated Set. 1, 2014, 7:04 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;">introduces the concept of fallback for packages.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
this will replace the private statically linked class LookAndFeelAccess in workspace and desktop, but be more generic so will be usable for things like the shell package as well.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
The feature has an autotest as well to check it's actually working and doesn't break other stuff.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the package structures that will use this, will just set a package in their structure::initPackage()<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
tough for the user of Package in c++ is possible as well to set the fallback package outside the structure if custm things are neede (it's guarded that cycles don't occur in the fallback chain)</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>autotests/CMakeLists.txt <span style="color: grey">(4e64f38)</span></li>
<li>autotests/data/testfallbackpackage/contents/ui/main.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/testfallbackpackage/metadata.desktop <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/data/testpackage/contents/ui/otherfile.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/fallbackpackagetest.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/fallbackpackagetest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/plasma/data/servicetypes/plasma-shell.desktop <span style="color: grey">(e2c83ba)</span></li>
<li>src/plasma/package.h <span style="color: grey">(2c686d7)</span></li>
<li>src/plasma/package.cpp <span style="color: grey">(6ad3321)</span></li>
<li>src/plasma/private/package_p.h <span style="color: grey">(d902eb1)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120029/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>