Merge lp:~donadigo/switchboard-plug-pantheon-shell/fix-wallpaper-permissions into lp:~elementary-apps/switchboard-plug-pantheon-shell/trunk
- fix-wallpaper-permissions
- Merge into trunk
Status: | Merged | ||||||||
---|---|---|---|---|---|---|---|---|---|
Approved by: | Danielle Foré | ||||||||
Approved revision: | 404 | ||||||||
Merged at revision: | 402 | ||||||||
Proposed branch: | lp:~donadigo/switchboard-plug-pantheon-shell/fix-wallpaper-permissions | ||||||||
Merge into: | lp:~elementary-apps/switchboard-plug-pantheon-shell/trunk | ||||||||
Diff against target: |
246 lines (+127/-41) 2 files modified
set-wallpaper-contract/set-wallpaper.vala (+53/-14) src/Wallpaper.vala (+74/-27) |
||||||||
To merge this branch: | bzr merge lp:~donadigo/switchboard-plug-pantheon-shell/fix-wallpaper-permissions | ||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
The Lemon Man (community) | code | Approve | |
Danielle Foré | ux | Approve | |
elementary Pantheon team | code | Pending | |
Review via email: mp+276153@code.launchpad.net |
Commit message
* Sets right permissions for wallpapers to have them visible in the greeter (finally)
* Various code optimizations
Description of the change
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) wrote : | # |
Akshay Shekher (voldyman) wrote : | # |
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) wrote : | # |
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) wrote : | # |
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.
Danielle Foré (danrabbit) wrote : | # |
I can confirm however that this fixes the issue :D
- 394. By Adam Bieńkowski
-
Copy the wallpaper to directory
- 395. By Adam Bieńkowski
-
Fix not changing wallpaper in greeter
- 396. By Adam Bieńkowski
-
Add missing permission line
- 397. By Adam Bieńkowski
-
Fixed: when choosing external source in the plug it does not change it in the greeter
Danielle Foré (danrabbit) wrote : | # |
Hey right on. I can confirm that it works both with the contract and with the plug. Nice work :D
Danielle Foré (danrabbit) wrote : | # |
Hrm, I found an issue. Set a non-default wall in the plug. Close the plug. Open it again. Gala background plugin crashes.
kay van der Zander (kay20) wrote : | # |
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) wrote : | # |
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.
- 398. By Adam Bieńkowski
-
Simplify wallpaper path's to one local path for both dbus and gsettings; wallpaper folder now in .config
- 399. By Adam Bieńkowski
-
Fix some issues
- 400. By Adam Bieńkowski
-
Copy requested wallpaper to /var/lib/
lightdm- data/user/ wallpaper - 401. By Adam Bieńkowski
-
Move greeter_file variable
Adam Bieńkowski (donadigo) wrote : | # |
Updated.
Danielle Foré (danrabbit) wrote : | # |
Okay alright, it seems to be working as expected. I can confirm that:
* walls are copied into .local/
* The current wall is copied to /var/lib/
* 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 :)
Akshay Shekher (voldyman) wrote : | # |
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/
- 402. By Adam Bieńkowski
-
Replace /usr/share/
backgrounds with constant variable - 403. By Adam Bieńkowski
-
Access specifier
The Lemon Man (lemonboy) wrote : | # |
Some nitpicks have been outlined in the inline comments.
- 404. By Adam Bieńkowski
-
Single build_filename call; use SYSTEM_
BACKGROUNDS_ PATH variable everywhere
Adam Bieńkowski (donadigo) wrote : | # |
Updated.
The Lemon Man (lemonboy) wrote : | # |
Code-wise it's ok. There's some code that could be factored out but let's get the ball rolling.
Preview Diff
1 | === modified file 'set-wallpaper-contract/set-wallpaper.vala' | |||
2 | --- set-wallpaper-contract/set-wallpaper.vala 2015-09-21 16:34:26 +0000 | |||
3 | +++ set-wallpaper-contract/set-wallpaper.vala 2015-11-15 21:44:26 +0000 | |||
4 | @@ -19,6 +19,8 @@ | |||
5 | 19 | </transition> | 19 | </transition> |
6 | 20 | """; | 20 | """; |
7 | 21 | 21 | ||
8 | 22 | const string SYSTEM_BACKGROUNDS_PATH = "/usr/share/backgrounds"; | ||
9 | 23 | |||
10 | 22 | private int delay_value = 60; | 24 | private int delay_value = 60; |
11 | 23 | 25 | ||
12 | 24 | [DBus (name = "org.freedesktop.Accounts.User")] | 26 | [DBus (name = "org.freedesktop.Accounts.User")] |
13 | @@ -100,16 +102,43 @@ | |||
14 | 100 | return folder; | 102 | return folder; |
15 | 101 | } | 103 | } |
16 | 102 | 104 | ||
24 | 103 | private bool copy_to_local_folder (File source) { | 105 | private File? copy_for_library (File source) { |
25 | 104 | try { | 106 | File? dest = null; |
26 | 105 | var dest = File.new_for_path (get_local_bg_location () + source.get_basename ()); | 107 | |
27 | 106 | source.copy (dest, FileCopyFlags.OVERWRITE|FileCopyFlags.NOFOLLOW_SYMLINKS); | 108 | try { |
28 | 107 | } catch (Error e) { | 109 | dest = File.new_for_path (get_local_bg_location () + source.get_basename ()); |
29 | 108 | warning ("%s\n", e.message); | 110 | source.copy (dest, FileCopyFlags.OVERWRITE | FileCopyFlags.ALL_METADATA); |
30 | 109 | return false; | 111 | } catch (Error e) { |
31 | 112 | warning ("%s\n", e.message); | ||
32 | 113 | } | ||
33 | 114 | |||
34 | 115 | return dest; | ||
35 | 116 | } | ||
36 | 117 | |||
37 | 118 | private File? copy_for_greeter (File source) { | ||
38 | 119 | File? dest = null; | ||
39 | 120 | try { | ||
40 | 121 | string greeter_data_dir = Path.build_filename (Environment.get_variable ("XDG_GREETER_DATA_DIR"), "wallpaper"); | ||
41 | 122 | |||
42 | 123 | var folder = File.new_for_path (greeter_data_dir); | ||
43 | 124 | if (folder.query_exists ()) { | ||
44 | 125 | var enumerator = folder.enumerate_children ("standard::*", FileQueryInfoFlags.NOFOLLOW_SYMLINKS); | ||
45 | 126 | FileInfo? info = null; | ||
46 | 127 | while ((info = enumerator.next_file ()) != null) { | ||
47 | 128 | enumerator.get_child (info).@delete (); | ||
48 | 129 | } | ||
49 | 130 | } else { | ||
50 | 131 | folder.make_directory_with_parents (); | ||
51 | 132 | } | ||
52 | 133 | |||
53 | 134 | dest = File.new_for_path (Path.build_filename (greeter_data_dir, source.get_basename ())); | ||
54 | 135 | source.copy (dest, FileCopyFlags.OVERWRITE | FileCopyFlags.ALL_METADATA); | ||
55 | 136 | } catch (Error e) { | ||
56 | 137 | warning ("%s\n", e.message); | ||
57 | 138 | return null; | ||
58 | 110 | } | 139 | } |
59 | 111 | 140 | ||
61 | 112 | return true; | 141 | return dest; |
62 | 113 | } | 142 | } |
63 | 114 | 143 | ||
64 | 115 | public static int main (string[] args) { | 144 | public static int main (string[] args) { |
65 | @@ -129,17 +158,27 @@ | |||
66 | 129 | var files = new List<File> (); | 158 | var files = new List<File> (); |
67 | 130 | for (var i = 1; i < args.length; i++) { | 159 | for (var i = 1; i < args.length; i++) { |
68 | 131 | var file = File.new_for_path (args[i]); | 160 | var file = File.new_for_path (args[i]); |
69 | 132 | var localfile = File.new_for_path (get_local_bg_location () + file.get_basename ()); | ||
70 | 133 | 161 | ||
71 | 134 | if (file != null) { | 162 | if (file != null) { |
76 | 135 | if (copy_to_local_folder (file)) { | 163 | |
77 | 136 | files.append (localfile); | 164 | string path = file.get_path (); |
78 | 137 | } else { | 165 | File append_file = file; |
79 | 138 | files.append (file); | 166 | if (!path.has_prefix (SYSTEM_BACKGROUNDS_PATH)) { |
80 | 167 | var local_file = copy_for_library (file); | ||
81 | 168 | if (local_file != null) { | ||
82 | 169 | append_file = local_file; | ||
83 | 170 | } | ||
84 | 171 | } | ||
85 | 172 | |||
86 | 173 | files.append (append_file); | ||
87 | 174 | |||
88 | 175 | var greeter_file = copy_for_greeter (file); | ||
89 | 176 | if (greeter_file != null) { | ||
90 | 177 | path = greeter_file.get_path (); | ||
91 | 139 | } | 178 | } |
92 | 140 | 179 | ||
93 | 141 | try { | 180 | try { |
95 | 142 | accounts_service.set_background_file (file.get_path ()); | 181 | accounts_service.set_background_file (path); |
96 | 143 | } catch (Error e) { | 182 | } catch (Error e) { |
97 | 144 | warning ("%s\n", e.message); | 183 | warning ("%s\n", e.message); |
98 | 145 | } | 184 | } |
99 | 146 | 185 | ||
100 | === modified file 'src/Wallpaper.vala' | |||
101 | --- src/Wallpaper.vala 2015-10-29 20:50:16 +0000 | |||
102 | +++ src/Wallpaper.vala 2015-11-15 21:44:26 +0000 | |||
103 | @@ -96,7 +96,9 @@ | |||
104 | 96 | 96 | ||
105 | 97 | //name of the default-wallpaper-link that we can prevent loading it again | 97 | //name of the default-wallpaper-link that we can prevent loading it again |
106 | 98 | //(assumes that the defaultwallpaper is also in the system wallpaper directory) | 98 | //(assumes that the defaultwallpaper is also in the system wallpaper directory) |
108 | 99 | static string default_link = "file:///usr/share/backgrounds/elementaryos-default"; | 99 | static string default_link = "file://%s/elementaryos-default".printf (SYSTEM_BACKGROUNDS_PATH); |
109 | 100 | |||
110 | 101 | const string SYSTEM_BACKGROUNDS_PATH = "/usr/share/backgrounds"; | ||
111 | 100 | 102 | ||
112 | 101 | public Wallpaper (Switchboard.Plug _plug) { | 103 | public Wallpaper (Switchboard.Plug _plug) { |
113 | 102 | plug = _plug; | 104 | plug = _plug; |
114 | @@ -201,8 +203,23 @@ | |||
115 | 201 | */ | 203 | */ |
116 | 202 | try { | 204 | try { |
117 | 203 | var file = File.new_for_uri (current_wallpaper_path); | 205 | var file = File.new_for_uri (current_wallpaper_path); |
120 | 204 | 206 | string uri = file.get_uri (); | |
121 | 205 | accountsservice.set_background_file (file.get_path ()); | 207 | string path = file.get_path (); |
122 | 208 | |||
123 | 209 | if (!path.has_prefix (SYSTEM_BACKGROUNDS_PATH)) { | ||
124 | 210 | var localfile = copy_for_library (file); | ||
125 | 211 | if (localfile != null) { | ||
126 | 212 | uri = localfile.get_uri (); | ||
127 | 213 | } | ||
128 | 214 | } | ||
129 | 215 | |||
130 | 216 | var greeter_file = copy_for_greeter (file); | ||
131 | 217 | if (greeter_file != null) { | ||
132 | 218 | path = greeter_file.get_path (); | ||
133 | 219 | } | ||
134 | 220 | |||
135 | 221 | settings.set_string ("picture-uri", uri); | ||
136 | 222 | accountsservice.set_background_file (path); | ||
137 | 206 | } catch (Error e) { | 223 | } catch (Error e) { |
138 | 207 | warning (e.message); | 224 | warning (e.message); |
139 | 208 | } | 225 | } |
140 | @@ -213,7 +230,6 @@ | |||
141 | 213 | 230 | ||
142 | 214 | if (!(children is SolidColorContainer)) { | 231 | if (!(children is SolidColorContainer)) { |
143 | 215 | current_wallpaper_path = children.uri; | 232 | current_wallpaper_path = children.uri; |
144 | 216 | settings.set_string ("picture-uri", current_wallpaper_path); | ||
145 | 217 | update_accountsservice (); | 233 | update_accountsservice (); |
146 | 218 | 234 | ||
147 | 219 | if (active_wallpaper == solid_color) { | 235 | if (active_wallpaper == solid_color) { |
148 | @@ -296,8 +312,8 @@ | |||
149 | 296 | } else if (folder_combo.get_active () == 1) { | 312 | } else if (folder_combo.get_active () == 1) { |
150 | 297 | clean_wallpapers (); | 313 | clean_wallpapers (); |
151 | 298 | 314 | ||
154 | 299 | var system_uri = "file:///usr/share/backgrounds"; | 315 | var system_uri = "file://" + SYSTEM_BACKGROUNDS_PATH; |
155 | 300 | var user_uri = GLib.File.new_for_path (GLib.Environment.get_user_data_dir () + "/backgrounds").get_uri (); | 316 | var user_uri = GLib.File.new_for_path (get_local_bg_location ()).get_uri (); |
156 | 301 | 317 | ||
157 | 302 | load_wallpapers (system_uri, cancellable); | 318 | load_wallpapers (system_uri, cancellable); |
158 | 303 | load_wallpapers (user_uri, cancellable); | 319 | load_wallpapers (user_uri, cancellable); |
159 | @@ -447,6 +463,52 @@ | |||
160 | 447 | Cache.clear (); | 463 | Cache.clear (); |
161 | 448 | } | 464 | } |
162 | 449 | 465 | ||
163 | 466 | private string get_local_bg_location () { | ||
164 | 467 | return Path.build_filename (Environment.get_user_data_dir (), "backgrounds") + "/"; | ||
165 | 468 | } | ||
166 | 469 | |||
167 | 470 | private File? copy_for_library (File source) { | ||
168 | 471 | File? dest = null; | ||
169 | 472 | |||
170 | 473 | try { | ||
171 | 474 | dest = File.new_for_path (get_local_bg_location () + source.get_basename ()); | ||
172 | 475 | source.copy (dest, FileCopyFlags.OVERWRITE | FileCopyFlags.ALL_METADATA); | ||
173 | 476 | } catch (Error e) { | ||
174 | 477 | warning ("%s\n", e.message); | ||
175 | 478 | } | ||
176 | 479 | |||
177 | 480 | return dest; | ||
178 | 481 | } | ||
179 | 482 | |||
180 | 483 | private File? copy_for_greeter (File source) { | ||
181 | 484 | File? dest = null; | ||
182 | 485 | try { | ||
183 | 486 | string greeter_data_dir = Path.build_filename (Environment.get_variable ("XDG_GREETER_DATA_DIR"), "wallpaper"); | ||
184 | 487 | if (greeter_data_dir == "") { | ||
185 | 488 | greeter_data_dir = Path.build_filename ("/var/lib/lightdm-data/", Environment.get_user_name (), "wallpaper"); | ||
186 | 489 | } | ||
187 | 490 | |||
188 | 491 | var folder = File.new_for_path (greeter_data_dir); | ||
189 | 492 | if (folder.query_exists ()) { | ||
190 | 493 | var enumerator = folder.enumerate_children ("standard::*", FileQueryInfoFlags.NOFOLLOW_SYMLINKS); | ||
191 | 494 | FileInfo? info = null; | ||
192 | 495 | while ((info = enumerator.next_file ()) != null) { | ||
193 | 496 | enumerator.get_child (info).@delete (); | ||
194 | 497 | } | ||
195 | 498 | } else { | ||
196 | 499 | folder.make_directory_with_parents (); | ||
197 | 500 | } | ||
198 | 501 | |||
199 | 502 | dest = File.new_for_path (Path.build_filename (greeter_data_dir, source.get_basename ())); | ||
200 | 503 | source.copy (dest, FileCopyFlags.OVERWRITE | FileCopyFlags.ALL_METADATA); | ||
201 | 504 | } catch (Error e) { | ||
202 | 505 | warning ("%s\n", e.message); | ||
203 | 506 | return null; | ||
204 | 507 | } | ||
205 | 508 | |||
206 | 509 | return dest; | ||
207 | 510 | } | ||
208 | 511 | |||
209 | 450 | void on_drag_data_received (Widget widget, Gdk.DragContext ctx, int x, int y, SelectionData sel, uint information, uint timestamp) { | 512 | void on_drag_data_received (Widget widget, Gdk.DragContext ctx, int x, int y, SelectionData sel, uint information, uint timestamp) { |
210 | 451 | if (sel.get_length () > 0) { | 513 | if (sel.get_length () > 0) { |
211 | 452 | File file = File.new_for_uri (sel.get_uris ()[0]); | 514 | File file = File.new_for_uri (sel.get_uris ()[0]); |
212 | @@ -457,29 +519,14 @@ | |||
213 | 457 | return; | 519 | return; |
214 | 458 | } | 520 | } |
215 | 459 | 521 | ||
236 | 460 | 522 | string local_uri = file.get_uri (); | |
237 | 461 | string display_name = Filename.display_basename (file.get_path ()); | 523 | var dest = copy_for_library (file); |
238 | 462 | 524 | if (dest != null) { | |
239 | 463 | var dest_folder = File.new_for_path (Environment.get_user_data_dir () + "/backgrounds"); | 525 | local_uri = dest.get_uri (); |
240 | 464 | var dest = File.new_for_path (Environment.get_user_data_dir () + "/backgrounds/" + display_name); | 526 | } |
221 | 465 | if (!dest_folder.query_exists ()) { | ||
222 | 466 | try { | ||
223 | 467 | dest_folder.make_directory (); | ||
224 | 468 | } catch (Error e) { | ||
225 | 469 | warning ("Creating local wallpaper directory failed: %s", e.message); | ||
226 | 470 | } | ||
227 | 471 | } | ||
228 | 472 | |||
229 | 473 | try { | ||
230 | 474 | file.copy (dest, 0); | ||
231 | 475 | } catch (Error e) { | ||
232 | 476 | warning ("Copying wallpaper to local directory failed: %s", e.message); | ||
233 | 477 | } | ||
234 | 478 | |||
235 | 479 | string uri = dest.get_uri (); | ||
241 | 480 | 527 | ||
242 | 481 | // Add the wallpaper name and thumbnail to the IconView | 528 | // Add the wallpaper name and thumbnail to the IconView |
244 | 482 | var wallpaper = new WallpaperContainer (uri); | 529 | var wallpaper = new WallpaperContainer (local_uri); |
245 | 483 | wallpaper_view.add (wallpaper); | 530 | wallpaper_view.add (wallpaper); |
246 | 484 | wallpaper.show_all (); | 531 | wallpaper.show_all (); |
247 | 485 | 532 |
Hmm, I'd be against chmod'ing the user files.