<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/120050/">https://git.reviewboard.kde.org/r/120050/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 20th, 2014, 8:22 a.m. CEST, <b>Luca Beltrame</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;">This causes a regression: normal images can no longer be loaded. Plasmashell outputs</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No metadata file in the package, expected it at: "/path/to/parent/dir/of/wallpaper/metadata.desktop"</p></pre>
</blockquote>
<p>On September 20th, 2014, 10:16 a.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;">this has been fixed.</p></pre>
</blockquote>
<p>On September 20th, 2014, 11:22 p.m. CEST, <b>Aleix Pol Gonzalez</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;">Sounds like it should be unit tested.</p></pre>
</blockquote>
<p>On September 21st, 2014, 12:17 a.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;">It would benefit from unit testing, but even more so from integration testing. Some of the functionality relies on screen resolution to be deterministic, but I'm sure that can be creatively worked around. The real challenge is in the integration testing. With the current design the Package classes are in libplasma, the QML interfaces are in libplasmaquick, the configuration glue is partly in the image plugin and partly in plasmashell, finding the preferred image file is partly in the wallpaper package and partly in the image plugin, some of the image wallpaper logic is in QML some is in C++ in the image plugin ... these breakages are more from integration points than unit functionality.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There is also room for refactoring which could clean up some of the integration points which in turn would make testing a bit more straightforward.</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;">seems there is still regression in one case - of default wallpaper (i.e. defined in theme).</p></pre>
<br />
<p>- Hrvoje</p>
<br />
<p>On September 19th, 2014, 4:28 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 Plasma.</div>
<div>By Aaron J. Seigo.</div>
<p style="color: grey;"><i>Updated Sept. 19, 2014, 4:28 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-workspace
</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 two things:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">restore the ability to set the wallpaper to be used to the name of the wallpaper package</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In the "old" days this was handled exclusively in setSingleImage, but the QML goes through addUrl wich did not have this feature. Now it does.</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Move the wallpaper package structure into a plugin so that wallpapers other than Image can use it.</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It did mean sacrificing the built-in ability to find the prefered image size, which stays behind in Image. This means each plugin using Wallpaper/Images will need to figure out which image to use on their own. This is still better than not having access to the package definition at all, but I imagine other plugins will want similar functionality. A bridge to cross once reached?</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;">Tested in a full plasmashell desktop session, configuration, etc. All worked as expected.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also tested with a shell package that sets wallpaper data on start, that works now too.</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>wallpapers/image/wallpaperpackage.cpp <span style="color: grey">(1ee524e34470acdec3e79238b1ba7151584ca858)</span></li>
<li>shell/packageplugins/wallpaper/wallpaper.h <span style="color: grey">()</span></li>
<li>shell/packageplugins/wallpaper/wallpaper.cpp <span style="color: grey">()</span></li>
<li>shell/packageplugins/wallpaperimages/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/packageplugins/wallpaperimages/plasma-packagestructure-wallpaperimages.desktop <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/packageplugins/wallpaperimages/wallpaperpackage.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/scripting/applet.h <span style="color: grey">(082a930637c5ffafab89cf44e875e95fc4996cf5)</span></li>
<li>shell/scripting/applet.cpp <span style="color: grey">(43a8acc053836fd04a640b8ae2f296d8a1e064d5)</span></li>
<li>shell/scripting/containment.cpp <span style="color: grey">(fc7fa82b6b9005ef298e18b6d8150cf2f2794351)</span></li>
<li>shell/scripting/scriptengine.cpp <span style="color: grey">(f458becb5df33116a6251ba3876aa6e06d16e2bf)</span></li>
<li>wallpapers/image/CMakeLists.txt <span style="color: grey">(50d1f1c8849a110a3e2c6a8d76ee96f5b8eb4fea)</span></li>
<li>wallpapers/image/backgroundlistmodel.h <span style="color: grey">(d5690e6859053c3debeac3ebf53bc0d947c0e8d8)</span></li>
<li>wallpapers/image/backgroundlistmodel.cpp <span style="color: grey">(ccb9d103ac2815c613783121d238ef5bd35c5655)</span></li>
<li>wallpapers/image/image.h <span style="color: grey">(c517c28d09e3187994db7085b5e9584f2d446b48)</span></li>
<li>wallpapers/image/image.cpp <span style="color: grey">(d672dfebcfb6849776aaf3faf4362a19182f7045)</span></li>
<li>wallpapers/image/wallpaperpackage.h <span style="color: grey">(45e8736685ce50cb0b1c21a4ab80734f304ccde2)</span></li>
<li>shell/packageplugins/CMakeLists.txt <span style="color: grey">(16ea8c03fb4a4ec1ee64ca506be5021d6db09d8b)</span></li>
<li>shell/packageplugins/wallpaper/CMakeLists.txt <span style="color: grey">()</span></li>
<li>shell/packageplugins/wallpaper/plasma-packagestructure-wallpaper.desktop <span style="color: grey">()</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120050/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>