Review Request 120050: Wallpaper package structure fixes

[Sebastian Kügler]

> On Sept. 20, 2014, 6:22 a.m., Luca Beltrame wrote:
> > This causes a regression: normal images can no longer be loaded. Plasmashell outputs
> > 
> > No metadata file in the package, expected it at: "/path/to/parent/dir/of/wallpaper/metadata.desktop"
> 
> Aaron J. Seigo wrote:
>     this has been fixed.
> 
> Aleix Pol Gonzalez wrote:
>     Sounds like it should be unit tested.
> 
> Aaron J. Seigo wrote:
>     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.
>     
>     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.
> 
> Hrvoje Senjan wrote:
>     seems there is still regression in one case - of default wallpaper (i.e. defined in theme).
> 
> Sebastian Kügler wrote:
>     Is this being addressed? We're getting closer to release...
> 
> Hrvoje Senjan wrote:
>     yep, it is fixed in master, but would also need a backport to 5.1 branch

Was it fixed after tagging? Do you have a commit id for the backport? (Also, if you want to backport it yourself, that's of course fine and appreciated.)


- Sebastian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120050/#review67019
-----------------------------------------------------------


On Sept. 19, 2014, 2:28 p.m., Aaron J. Seigo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120050/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2014, 2:28 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> This patch set does two things:
> 
> 1. restore the ability to set the wallpaper to be used to the name of the wallpaper package
> 
> 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.
> 
> 2. Move the wallpaper package structure into a plugin so that wallpapers other than Image can use it.
> 
> 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?
> 
> 
> Diffs
> -----
> 
>   wallpapers/image/wallpaperpackage.cpp 1ee524e34470acdec3e79238b1ba7151584ca858 
>   shell/packageplugins/wallpaper/wallpaper.h  
>   shell/packageplugins/wallpaper/wallpaper.cpp  
>   shell/packageplugins/wallpaperimages/CMakeLists.txt PRE-CREATION 
>   shell/packageplugins/wallpaperimages/plasma-packagestructure-wallpaperimages.desktop PRE-CREATION 
>   shell/packageplugins/wallpaperimages/wallpaperpackage.cpp PRE-CREATION 
>   shell/scripting/applet.h 082a930637c5ffafab89cf44e875e95fc4996cf5 
>   shell/scripting/applet.cpp 43a8acc053836fd04a640b8ae2f296d8a1e064d5 
>   shell/scripting/containment.cpp fc7fa82b6b9005ef298e18b6d8150cf2f2794351 
>   shell/scripting/scriptengine.cpp f458becb5df33116a6251ba3876aa6e06d16e2bf 
>   wallpapers/image/CMakeLists.txt 50d1f1c8849a110a3e2c6a8d76ee96f5b8eb4fea 
>   wallpapers/image/backgroundlistmodel.h d5690e6859053c3debeac3ebf53bc0d947c0e8d8 
>   wallpapers/image/backgroundlistmodel.cpp ccb9d103ac2815c613783121d238ef5bd35c5655 
>   wallpapers/image/image.h c517c28d09e3187994db7085b5e9584f2d446b48 
>   wallpapers/image/image.cpp d672dfebcfb6849776aaf3faf4362a19182f7045 
>   wallpapers/image/wallpaperpackage.h 45e8736685ce50cb0b1c21a4ab80734f304ccde2 
>   shell/packageplugins/CMakeLists.txt 16ea8c03fb4a4ec1ee64ca506be5021d6db09d8b 
>   shell/packageplugins/wallpaper/CMakeLists.txt  
>   shell/packageplugins/wallpaper/plasma-packagestructure-wallpaper.desktop  
> 
> Diff: https://git.reviewboard.kde.org/r/120050/diff/
> 
> 
> Testing
> -------
> 
> Tested in a full plasmashell desktop session, configuration, etc. All worked as expected.
> 
> Also tested with a shell package that sets wallpaper data on start, that works now too.
> 
> 
> Thanks,
> 
> Aaron J. Seigo
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140929/8a29a514/attachment-0001.html>