Merge lp:~donadigo/switchboard-plug-pantheon-shell/fix-wallpaper-permissions into lp:~elementary-apps/switchboard-plug-pantheon-shell/trunk

[Adam Bieńkowski]

* Sets right permissions for wallpapers to have them visible in the greeter (finally)
* Various code optimizations

The branch fixes some issues and makes optimizations to current wallpaper
* Sets right permissions for wallpapers to have them visible in the greeter (finally)
* Various code optimizations

[The Lemon Man (lemonboy)]

Hmm, I'd be against chmod'ing the user files.

[Akshay Shekher (voldyman)]

Why don't you ask the user if they want switchboard to fix the file permissions if they are not 0644?

[Adam Bieńkowski (donadigo)]

voldyman: Why we would 'ask' to fix the 'file permissions' to just change the wallpaper? No one is doing that and it seems reduntant to ask user for these things.

[Danielle Foré (danrabbit)]

I don't think using a symlink is a good idea. Now you're giving me something to remember: not to delete that wallpaper that's in a random location on my file system. Or worse, it could be on an external drive and now I can't unmount that drive without losing my wallpaper. Copying ensures that the wallpaper won't be removed accidentally.

I do think we should change the permissions without asking. It's a wallpaper, I think it'll be okay. Users expect this feature to work and I don't think we should have to bother less experienced users knowing what permissions even are and why they're needed for this feature.

review: Needs Fixing

[Danielle Foré (danrabbit)]

I can confirm however that this fixes the issue :D

[lp:~donadigo/switchboard-plug-pantheon-shell/fix-wallpaper-permissions]

394. By Adam Bieńkowski on 2015-10-29

Copy the wallpaper to directory

395. By Adam Bieńkowski on 2015-10-29

Fix not changing wallpaper in greeter

396. By Adam Bieńkowski on 2015-10-29

Add missing permission line

397. By Adam Bieńkowski on 2015-10-29

Fixed: when choosing external source in the plug it does not change it in the greeter

[Danielle Foré (danrabbit)]

Hey right on. I can confirm that it works both with the contract and with the plug. Nice work :D

review: Approve (ux)

[Danielle Foré (danrabbit)]

Hrm, I found an issue. Set a non-default wall in the plug. Close the plug. Open it again. Gala background plugin crashes.

review: Needs Fixing

[kay van der Zander (kay20)]

If the plug creates a copy of the wallpaper the user wants. Why not only change the copy his permissions. The original wallpaper will be untouched (besides copying).

currently the original wallpaper is changed and the copy isn't
and why change all wallpapers there premissions (diff line 68)

[Adam Bieńkowski (donadigo)]

kay: line 68 is there to e.g: if you choose wallpaper directly from the plug, permissions won't be changed, and it will not be visible in the plug.

[lp:~donadigo/switchboard-plug-pantheon-shell/fix-wallpaper-permissions]

398. By Adam Bieńkowski on 2015-10-30

Simplify wallpaper path's to one local path for both dbus and gsettings; wallpaper folder now in .config

399. By Adam Bieńkowski on 2015-10-30

Fix some issues

400. By Adam Bieńkowski on 2015-10-31

Copy requested wallpaper to /var/lib/lightdm-data/user/wallpaper

401. By Adam Bieńkowski on 2015-10-31

Move greeter_file variable

[Adam Bieńkowski (donadigo)]

Updated.

[Danielle Foré (danrabbit)]

Okay alright, it seems to be working as expected. I can confirm that:
* walls are copied into .local/share/backgrounds
* The current wall is copied to /var/lib/lightdm-data/user/wallpaper
* LightDM is able to read the wall
* This works both with the contract and with Drag N Drop
* I can't reproduce any of the other issues/crashes we had previously experienced

Good work :)

review: Approve (ux)

[Akshay Shekher (voldyman)]

please use const's and not hardcoded strings so that they can easily be changed later, we don't want to grep throught the code to find all the mentions of /usr/share/backgrounds, etc.

[lp:~donadigo/switchboard-plug-pantheon-shell/fix-wallpaper-permissions]

402. By Adam Bieńkowski on 2015-10-31

Replace /usr/share/backgrounds with constant variable

403. By Adam Bieńkowski on 2015-10-31

Access specifier

[The Lemon Man (lemonboy)]

Some nitpicks have been outlined in the inline comments.

review: Needs Fixing (code)

[lp:~donadigo/switchboard-plug-pantheon-shell/fix-wallpaper-permissions]

404. By Adam Bieńkowski on 2015-11-15

Single build_filename call; use SYSTEM_BACKGROUNDS_PATH variable everywhere

[Adam Bieńkowski (donadigo)]

Updated.

[The Lemon Man (lemonboy)]

Code-wise it's ok. There's some code that could be factored out but let's get the ball rolling.

review: Approve (code)

=== modified file 'set-wallpaper-contract/set-wallpaper.vala'
--- set-wallpaper-contract/set-wallpaper.vala	2015-09-21 16:34:26 +0000
+++ set-wallpaper-contract/set-wallpaper.vala	2015-11-15 21:44:26 +0000
@@ -19,6 +19,8 @@
         </transition>
     """;
 
+    const string SYSTEM_BACKGROUNDS_PATH = "/usr/share/backgrounds";
+
     private int delay_value = 60;
 
     [DBus (name = "org.freedesktop.Accounts.User")]
@@ -100,16 +102,43 @@
         return folder;
     }
 
-    private bool copy_to_local_folder (File source) {
-        try {
-            var dest = File.new_for_path (get_local_bg_location () + source.get_basename ());
-            source.copy (dest, FileCopyFlags.OVERWRITE|FileCopyFlags.NOFOLLOW_SYMLINKS);
-        } catch (Error e) {
-            warning ("%s\n", e.message);
-            return false;
+    private File? copy_for_library (File source) {
+        File? dest = null;
+
+        try {
+            dest = File.new_for_path (get_local_bg_location () + source.get_basename ());
+            source.copy (dest, FileCopyFlags.OVERWRITE | FileCopyFlags.ALL_METADATA);
+        } catch (Error e) {
+            warning ("%s\n", e.message);
+        }   
+
+        return dest;    
+    }
+
+    private File? copy_for_greeter (File source) {
+        File? dest = null;
+        try {
+            string greeter_data_dir = Path.build_filename (Environment.get_variable ("XDG_GREETER_DATA_DIR"), "wallpaper");
+
+            var folder = File.new_for_path (greeter_data_dir);
+            if (folder.query_exists ()) {
+                var enumerator = folder.enumerate_children ("standard::*", FileQueryInfoFlags.NOFOLLOW_SYMLINKS);
+                FileInfo? info = null;
+                while ((info = enumerator.next_file ()) != null) {
+                    enumerator.get_child (info).@delete ();
+                }
+            } else {
+                folder.make_directory_with_parents ();
+            }
+
+            dest = File.new_for_path (Path.build_filename (greeter_data_dir, source.get_basename ()));
+            source.copy (dest, FileCopyFlags.OVERWRITE | FileCopyFlags.ALL_METADATA);
+        } catch (Error e) {
+            warning ("%s\n", e.message);
+            return null;
         }
 
-        return true;
+        return dest;
     }
 
     public static int main (string[] args) {
@@ -129,17 +158,27 @@
         var files = new List<File> ();
         for (var i = 1; i < args.length; i++) {
             var file = File.new_for_path (args[i]);
-            var localfile = File.new_for_path (get_local_bg_location () + file.get_basename ());
 
             if (file != null) {
-                if (copy_to_local_folder (file)) {
-                    files.append (localfile);
-                } else {
-                    files.append (file);
+
+                string path = file.get_path ();
+                File append_file = file;
+                if (!path.has_prefix (SYSTEM_BACKGROUNDS_PATH)) {
+                    var local_file = copy_for_library (file);
+                    if (local_file != null) {
+                        append_file = local_file;
+                    }
+                }
+
+                files.append (append_file);
+
+                var greeter_file = copy_for_greeter (file);
+                if (greeter_file != null) {
+                    path = greeter_file.get_path ();
                 }
 
                 try {
-                    accounts_service.set_background_file (file.get_path ());
+                    accounts_service.set_background_file (path);
                 } catch (Error e) {
                     warning ("%s\n", e.message);
                 }        

=== modified file 'src/Wallpaper.vala'
--- src/Wallpaper.vala	2015-10-29 20:50:16 +0000
+++ src/Wallpaper.vala	2015-11-15 21:44:26 +0000
@@ -96,7 +96,9 @@
 
     //name of the default-wallpaper-link that we can prevent loading it again
     //(assumes that the defaultwallpaper is also in the system wallpaper directory)
-    static string default_link = "file:///usr/share/backgrounds/elementaryos-default";
+    static string default_link = "file://%s/elementaryos-default".printf (SYSTEM_BACKGROUNDS_PATH);
+
+    const string SYSTEM_BACKGROUNDS_PATH = "/usr/share/backgrounds";
 
     public Wallpaper (Switchboard.Plug _plug) {
         plug = _plug;
@@ -201,8 +203,23 @@
         */
         try {
             var file = File.new_for_uri (current_wallpaper_path);
-
-            accountsservice.set_background_file (file.get_path ());
+            string uri = file.get_uri ();
+            string path = file.get_path ();
+
+            if (!path.has_prefix (SYSTEM_BACKGROUNDS_PATH)) {
+                var localfile = copy_for_library (file);
+                if (localfile != null) {
+                    uri = localfile.get_uri ();
+                }
+            }
+
+            var greeter_file = copy_for_greeter (file);
+            if (greeter_file != null) {
+                path = greeter_file.get_path ();
+            }
+
+            settings.set_string ("picture-uri", uri);
+            accountsservice.set_background_file (path);
         } catch (Error e) {
             warning (e.message);
         }
@@ -213,7 +230,6 @@
 
         if (!(children is SolidColorContainer)) {
             current_wallpaper_path = children.uri;
-            settings.set_string ("picture-uri", current_wallpaper_path);
             update_accountsservice ();
 
             if (active_wallpaper == solid_color) {
@@ -296,8 +312,8 @@
         } else if (folder_combo.get_active () == 1) {
             clean_wallpapers ();
 
-            var system_uri = "file:///usr/share/backgrounds";
-            var user_uri = GLib.File.new_for_path (GLib.Environment.get_user_data_dir () + "/backgrounds").get_uri ();
+            var system_uri = "file://" + SYSTEM_BACKGROUNDS_PATH;
+            var user_uri = GLib.File.new_for_path (get_local_bg_location ()).get_uri ();
 
             load_wallpapers (system_uri, cancellable);
             load_wallpapers (user_uri, cancellable);
@@ -447,6 +463,52 @@
         Cache.clear ();
     }
 
+    private string get_local_bg_location () {
+        return Path.build_filename (Environment.get_user_data_dir (), "backgrounds") + "/";
+    }
+
+    private File? copy_for_library (File source) {
+        File? dest = null;
+
+        try {
+            dest = File.new_for_path (get_local_bg_location () + source.get_basename ());
+            source.copy (dest, FileCopyFlags.OVERWRITE | FileCopyFlags.ALL_METADATA);
+        } catch (Error e) {
+            warning ("%s\n", e.message);
+        }   
+
+        return dest;    
+    }
+
+    private File? copy_for_greeter (File source) {
+        File? dest = null;
+        try {
+            string greeter_data_dir = Path.build_filename (Environment.get_variable ("XDG_GREETER_DATA_DIR"), "wallpaper");
+            if (greeter_data_dir == "") {
+                greeter_data_dir = Path.build_filename ("/var/lib/lightdm-data/", Environment.get_user_name (), "wallpaper");
+            }
+
+            var folder = File.new_for_path (greeter_data_dir);
+            if (folder.query_exists ()) {
+                var enumerator = folder.enumerate_children ("standard::*", FileQueryInfoFlags.NOFOLLOW_SYMLINKS);
+                FileInfo? info = null;
+                while ((info = enumerator.next_file ()) != null) {
+                    enumerator.get_child (info).@delete ();
+                }
+            } else {
+                folder.make_directory_with_parents ();
+            }
+
+            dest = File.new_for_path (Path.build_filename (greeter_data_dir, source.get_basename ()));
+            source.copy (dest, FileCopyFlags.OVERWRITE | FileCopyFlags.ALL_METADATA);
+        } catch (Error e) {
+            warning ("%s\n", e.message);
+            return null;
+        }
+
+        return dest;
+    }
+
     void on_drag_data_received (Widget widget, Gdk.DragContext ctx, int x, int y, SelectionData sel, uint information, uint timestamp) {
         if (sel.get_length () > 0) {
             File file = File.new_for_uri (sel.get_uris ()[0]);
@@ -457,29 +519,14 @@
                 return;
             }
 
-
-            string display_name = Filename.display_basename (file.get_path ());
-
-            var dest_folder = File.new_for_path (Environment.get_user_data_dir () + "/backgrounds");
-            var dest = File.new_for_path (Environment.get_user_data_dir () + "/backgrounds/" + display_name);
-            if (!dest_folder.query_exists ()) {
-                try {
-                    dest_folder.make_directory ();
-                } catch (Error e) {
-                    warning ("Creating local wallpaper directory failed: %s", e.message);
-                }
-            }
-
-            try {
-                file.copy (dest, 0);
-            } catch (Error e) {
-                warning ("Copying wallpaper to local directory failed: %s", e.message);
-            }
-
-            string uri = dest.get_uri ();
+            string local_uri = file.get_uri ();
+            var dest = copy_for_library (file);
+            if (dest != null) {
+                local_uri = dest.get_uri ();
+            }
 
             // Add the wallpaper name and thumbnail to the IconView
-            var wallpaper = new WallpaperContainer (uri);
+            var wallpaper = new WallpaperContainer (local_uri);
             wallpaper_view.add (wallpaper);
             wallpaper.show_all ();