After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 741453 - Uses the screensaver background in place of the regular one
Uses the screensaver background in place of the regular one
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: background
3.14.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2014-12-12 18:47 UTC by Josselin Mouette
Modified: 2015-09-08 16:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix the race condition (2.49 KB, patch)
2014-12-12 19:31 UTC, Josselin Mouette
needs-work Details | Review
Patch for GNOME 3.14 (3.08 KB, patch)
2014-12-12 20:18 UTC, Josselin Mouette
none Details | Review
Patch for master (3.51 KB, patch)
2014-12-12 20:19 UTC, Josselin Mouette
needs-work Details | Review
Fix indentation (1.23 KB, patch)
2014-12-12 21:28 UTC, Josselin Mouette
accepted-commit_now Details | Review
Patch for master (2.96 KB, patch)
2014-12-12 21:28 UTC, Josselin Mouette
needs-work Details | Review
Patch for GNOME 3.14 (3.10 KB, patch)
2014-12-12 21:29 UTC, Josselin Mouette
accepted-commit_now Details | Review

Description Josselin Mouette 2014-12-12 18:47:05 UTC
If the screensaver background (org.gnome.desktop.screensaver → picture-uri) is set to a XML file, it is used in place of the regular background.

If the screensaver background is set to a regular file, the bug doesn’t happen.

I’m guessing some caching mechanism is doing it wrong.
Comment 1 Josselin Mouette 2014-12-12 19:04:27 UTC
Correction: the bug only happens if *both* backgrounds are XML.

Note that changing them after the shell startup will get correct results, but at the next restart of the shell the bug shows up again.
Comment 2 Josselin Mouette 2014-12-12 19:31:01 UTC
Created attachment 292627 [details] [review]
Fix the race condition
Comment 3 Ray Strode [halfline] 2014-12-12 19:40:21 UTC
Review of attachment 292627 [details] [review]:

::: js/ui/background.js
@@ +156,3 @@
                                         onLoaded: null });
 
+        if (this._animations[params.filename]) {

Not sure keying this off filename is a good idea.  It means the animations list will grow over time without bound. I think I would instead key it off the schema name in the same way backgroundSources are handled.  It does mean the xml file will get parsed twice if both screensaver and normal background are set to the same value, but i don't think that's that big of a deal.
Comment 4 Josselin Mouette 2014-12-12 20:18:52 UTC
Created attachment 292631 [details] [review]
Patch for GNOME 3.14

Here you go.
Comment 5 Josselin Mouette 2014-12-12 20:19:21 UTC
Created attachment 292632 [details] [review]
Patch for master
Comment 6 Ray Strode [halfline] 2014-12-12 20:25:20 UTC
Review of attachment 292632 [details] [review]:

commit message still says "based on filename"

::: js/ui/background.js
@@ +166,3 @@
                                         onLoaded: null });
 
+        if (this._animations[params.settingsSchema] && _fileEqual0(this._animations[params.settingsSchema].filename, params.file)) {

Animation object calls it "file" not "filename"

@@ -404,3 @@
-    _loadAnimation: function(file) {
-        this._cache.getAnimation({ file: file,
-                                         onLoaded: Lang.bind(this, function(animation) {

spacing is weird here before you touched it.  mind doing a prerequisite commit that fixes the spacing first?
Comment 7 Josselin Mouette 2014-12-12 21:28:19 UTC
Created attachment 292637 [details] [review]
Fix indentation
Comment 8 Josselin Mouette 2014-12-12 21:28:37 UTC
Created attachment 292638 [details] [review]
Patch for master
Comment 9 Josselin Mouette 2014-12-12 21:29:25 UTC
Created attachment 292639 [details] [review]
Patch for GNOME 3.14

And voilà.
Comment 10 Ray Strode [halfline] 2014-12-15 21:53:29 UTC
Review of attachment 292637 [details] [review]:

cool
Comment 11 Ray Strode [halfline] 2014-12-15 21:55:27 UTC
Review of attachment 292638 [details] [review]:

seems right (assuming it's tested)
Comment 12 Ray Strode [halfline] 2014-12-15 22:05:17 UTC
Review of attachment 292639 [details] [review]:

::: js/ui/background.js
@@ +397,1 @@
                                              onLoaded: Lang.bind(this, function(animation) {

would prefer if you pushed the fix indentation patch too.
Comment 13 Josselin Mouette 2014-12-15 22:21:12 UTC
(In reply to comment #11)
> seems right (assuming it's tested)

I’m running 3.14 so I can’t vouch for master.

(In reply to comment #12)
> would prefer if you pushed the fix indentation patch too.

Will do.
Comment 14 darkxst 2015-02-08 23:50:44 UTC
Review of attachment 292638 [details] [review]:

::: js/ui/background.js
@@ +165,2 @@
         params = Params.parse(params, { file: null,
                                         onLoaded: null });

You are missing the default for settingsSchema here
Comment 15 Michael Biebl 2015-09-07 04:56:21 UTC
I pushed both patches to master including darkxt's comment in #14

Feel free to close the bug report now.
Comment 16 Michael Biebl 2015-09-07 04:56:58 UTC
indentation fix: 785c90f4b85478b690de71c4448f640a27425c41
background race fix: 8a4c86263362c0b2d2f87b21b8f783ccc5690bf8