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 682429 - gdm => session transition doesn't look very good (background / session side)
gdm => session transition doesn't look very good (background / session side)
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: login-screen
3.4.x
Other Linux
: Normal normal
: ---
Assigned To: Ray Strode [halfline]
gnome-shell-maint
: 682428 682538 684011 684189 685295 694132 (view as bug list)
Depends on: 682427
Blocks: 682428 694062
 
 
Reported: 2012-08-22 06:46 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2013-05-06 13:46 UTC
See Also:
GNOME target: 3.8
GNOME version: ---


Attachments
sessionMode: Reindent (5.91 KB, patch)
2012-08-22 07:42 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
layout: Don't unnecessarily go through Main for a property (815 bytes, patch)
2012-08-22 07:42 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
layout: Turn off the startup animation when in a user session (1.82 KB, patch)
2012-08-22 07:42 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
layout: Turn off the startup animation when in a user session (1.41 KB, patch)
2012-09-13 18:42 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
backgrounds: Add the noise texture (100.03 KB, patch)
2012-09-15 22:09 UTC, Jasper St. Pierre (not reading bugmail)
rejected Details | Review
data: Use a noise texture for the gdm user's background (2.79 KB, patch)
2012-09-15 22:09 UTC, Jasper St. Pierre (not reading bugmail)
rejected Details | Review
layout: Raise the panel back up when logging in (2.21 KB, patch)
2012-09-15 22:10 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
layout: Raise the panel back up when logging in (2.24 KB, patch)
2012-09-19 06:42 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
Main: move the stage hierarchy initialization to LayoutManager (9.27 KB, patch)
2012-12-24 15:14 UTC, Giovanni Campagna
none Details | Review
Util: add an accessor for the root background pixmap (3.93 KB, patch)
2012-12-24 15:14 UTC, Giovanni Campagna
none Details | Review
LayoutManager: implement new design for the startup animation (7.08 KB, patch)
2012-12-24 15:14 UTC, Giovanni Campagna
none Details | Review
Main: move the stage hierarchy initialization to LayoutManager (53.42 KB, patch)
2013-01-10 22:51 UTC, Ray Strode [halfline]
reviewed Details | Review
Util: add an accessor for getting still frame (15.02 KB, patch)
2013-01-10 22:51 UTC, Ray Strode [halfline]
needs-work Details | Review
LayoutManager: implement new design for the startup animation (27.92 KB, patch)
2013-01-10 22:51 UTC, Ray Strode [halfline]
reviewed Details | Review
Run the startup animation when we don't have much going on (2.06 KB, patch)
2013-01-11 23:36 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
Main: move the stage hierarchy initialization to LayoutManager (8.99 KB, patch)
2013-02-07 09:55 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
layout: Make the uiGroup a standard StWidget (3.13 KB, patch)
2013-02-07 09:55 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
Main: move the stage hierarchy initialization to LayoutManager (9.34 KB, patch)
2013-02-13 18:51 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
Main: move the stage hierarchy initialization to LayoutManager (51.40 KB, patch)
2013-02-14 05:55 UTC, Ray Strode [halfline]
committed Details | Review
Run the startup animation when we don't have much going on (37.50 KB, patch)
2013-02-14 05:57 UTC, Ray Strode [halfline]
none Details | Review
layout: rework background handling (149.25 KB, patch)
2013-02-14 05:57 UTC, Ray Strode [halfline]
none Details | Review
background: add slide show support (20.85 KB, patch)
2013-02-14 05:57 UTC, Ray Strode [halfline]
none Details | Review
sessionMode: load internal session modes immediately (2.70 KB, patch)
2013-02-14 05:57 UTC, Ray Strode [halfline]
rejected Details | Review
layout: don't bother hiding window group (37.90 KB, patch)
2013-02-14 05:57 UTC, Ray Strode [halfline]
none Details | Review
loginDialog: get rid of title label (28.69 KB, patch)
2013-02-14 05:57 UTC, Ray Strode [halfline]
needs-work Details | Review
loginDialog: drop animations (40.30 KB, patch)
2013-02-14 05:57 UTC, Ray Strode [halfline]
accepted-commit_now Details | Review
loginDialog: fade out before starting session (55.55 KB, patch)
2013-02-14 05:57 UTC, Ray Strode [halfline]
reviewed Details | Review
loginDialog: clone user avatar when doing verification (26.50 KB, patch)
2013-02-14 05:58 UTC, Ray Strode [halfline]
reviewed Details | Review
loginDialog: put "Not Listed?" button and user list in separate container (32.16 KB, patch)
2013-02-14 05:58 UTC, Ray Strode [halfline]
reviewed Details | Review
loginDialog: add cross fade animation between states (79.09 KB, patch)
2013-02-14 05:58 UTC, Ray Strode [halfline]
none Details | Review
Run the startup animation when we don't have much going on (8.11 KB, patch)
2013-02-18 04:54 UTC, Ray Strode [halfline]
committed Details | Review
main: defer startup until we know our session mode (2.77 KB, patch)
2013-02-18 04:54 UTC, Ray Strode [halfline]
committed Details | Review
layout: rework background handling (77.97 KB, patch)
2013-02-18 04:54 UTC, Ray Strode [halfline]
needs-work Details | Review
background: add slide show support (23.58 KB, patch)
2013-02-18 04:54 UTC, Ray Strode [halfline]
needs-work Details | Review
layout: don't bother hiding window group (3.27 KB, patch)
2013-02-18 04:54 UTC, Ray Strode [halfline]
committed Details | Review
layout: rework background handling (73.71 KB, patch)
2013-02-18 08:10 UTC, Ray Strode [halfline]
reviewed Details | Review
layout: rework background handling (78.51 KB, patch)
2013-02-18 16:34 UTC, Ray Strode [halfline]
none Details | Review
layout: rework background handling (78.49 KB, patch)
2013-02-18 16:38 UTC, Ray Strode [halfline]
none Details | Review
background: add slide show support (25.99 KB, patch)
2013-02-18 16:40 UTC, Ray Strode [halfline]
none Details | Review
layout: rework background handling (80.05 KB, patch)
2013-02-18 22:10 UTC, Ray Strode [halfline]
none Details | Review
background: add slide show support (20.71 KB, patch)
2013-02-18 22:10 UTC, Ray Strode [halfline]
none Details | Review
layout: rework background handling (80.05 KB, patch)
2013-02-18 23:42 UTC, Ray Strode [halfline]
none Details | Review
background: add slide show support (21.97 KB, patch)
2013-02-18 23:42 UTC, Ray Strode [halfline]
none Details | Review
background: add slide show support (21.92 KB, patch)
2013-02-18 23:54 UTC, Ray Strode [halfline]
none Details | Review
layout: rework background handling (79.12 KB, patch)
2013-02-19 21:16 UTC, Ray Strode [halfline]
reviewed Details | Review
background: add slide show support (21.64 KB, patch)
2013-02-19 21:16 UTC, Ray Strode [halfline]
accepted-commit_now Details | Review
layout: rework background handling (79.66 KB, patch)
2013-02-19 23:03 UTC, Ray Strode [halfline]
committed Details | Review
background: add slide show support (20.80 KB, patch)
2013-02-19 23:03 UTC, Ray Strode [halfline]
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2012-08-22 06:46:00 UTC
After we log the user in, we see the system configured background again. Finally, panels animate down from the top.

As a quick hack, we might be able to set the gdm's dconf database to have the noise texture for a desktop background, so going from gray to blue to user's background will be a little less apparent.

We should probably only animate the panel coming down from the top when starting up gnome-shell for the first time, choosing to fade it in instead if it's a user session.
Comment 1 Jasper St. Pierre (not reading bugmail) 2012-08-22 07:42:03 UTC
Created attachment 222105 [details] [review]
sessionMode: Reindent

Keep everything on the same level, so that the name of the session mode
doesn't affect the length of the line.
Comment 2 Jasper St. Pierre (not reading bugmail) 2012-08-22 07:42:06 UTC
Created attachment 222106 [details] [review]
layout: Don't unnecessarily go through Main for a property

We *are* Main.layoutManager, here.
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-08-22 07:42:10 UTC
Created attachment 222107 [details] [review]
layout: Turn off the startup animation when in a user session
Comment 4 Giovanni Campagna 2012-08-22 10:19:28 UTC
Review of attachment 222106 [details] [review]:

Even if the rest doesn't land, this makes sense
Comment 5 Giovanni Campagna 2012-08-22 10:20:40 UTC
Review of attachment 222105 [details] [review]:

Bah... Breaks git-blame for little...
Comment 6 Giovanni Campagna 2012-08-22 10:22:35 UTC
Review of attachment 222107 [details] [review]:

I think this needs designer input.
Also consider that at least here, there is quite some time before the greeter shell is killed and the new one starts, so it still makes sense for me to animate the panels.
As far as code is concerned, it's fine.
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-08-22 18:57:10 UTC
Comment on attachment 222106 [details] [review]
layout: Don't unnecessarily go through Main for a property

Attachment 222106 [details] pushed as 2154a22 - layout: Don't unnecessarily go through Main for a property
Comment 8 Matthias Clasen 2012-08-25 00:05:54 UTC
Lets attract some designers, then...
Comment 9 Florian Müllner 2012-09-13 16:59:46 UTC
Comment on attachment 222105 [details] [review]
sessionMode: Reindent

(Marking patch obsolete that got committed in bug 683156)
Comment 10 Jasper St. Pierre (not reading bugmail) 2012-09-13 18:42:36 UTC
Created attachment 224254 [details] [review]
layout: Turn off the startup animation when in a user session
Comment 11 Ray Strode [halfline] 2012-09-14 18:28:46 UTC
I tried this yesterday and wasn't very happy with it.  comment 6 seems right, we probably still want to animate the panels, it's kind of jarring to see them pop in.  I think what would look better than this, though, is dropping down the shield at greeter exit rather than stopping the animation at user start.

Ultimately, we want to get to something like:

http://jimmac.fedorapeople.org/gnome3/login.webm

(although see bug 684011 this animation about to be updated)
Comment 12 Ray Strode [halfline] 2012-09-14 18:29:59 UTC
*** Bug 684011 has been marked as a duplicate of this bug. ***
Comment 13 Jasper St. Pierre (not reading bugmail) 2012-09-15 22:09:49 UTC
Created attachment 224423 [details] [review]
backgrounds: Add the noise texture

This will be used by gdm to log in, and other random system services.
It isn't designed to be a user-selectable wallpaper, so don't add it
to the XML.
Comment 14 Jasper St. Pierre (not reading bugmail) 2012-09-15 22:09:59 UTC
Created attachment 224424 [details] [review]
data: Use a noise texture for the gdm user's background

This gives us a nice, clean image while we're logging in that matches
the gdm background.
Comment 15 Jasper St. Pierre (not reading bugmail) 2012-09-15 22:10:38 UTC
Created attachment 224425 [details] [review]
layout: Raise the panel back up when logging in

Let's try this instead.
Comment 16 Ray Strode [halfline] 2012-09-17 03:19:33 UTC
Review of attachment 224424 [details] [review]:

::: data/00-upstream-settings
@@ +12,3 @@
 [org/gnome/desktop/background]
 show-desktop-icons=false
+picture-uri=@backgrounddir@/Noise.png

So this means the screen shield (at the login screen) is going to have a noise texture background too, yea? Also, the fallback greeter will.

Not sure that's right.
Comment 17 Ray Strode [halfline] 2012-09-17 03:21:16 UTC
Review of attachment 224425 [details] [review]:

::: js/gdm/loginDialog.js
@@ +980,3 @@
+                this._greeter.call_start_session_when_ready_sync(serviceName, true, null);
+            }
+        ]);

you have to run the batch after creating it.
Comment 18 Giovanni Campagna 2012-09-17 13:25:42 UTC
*** Bug 684189 has been marked as a duplicate of this bug. ***
Comment 19 Jasper St. Pierre (not reading bugmail) 2012-09-19 03:24:31 UTC
(In reply to comment #16)
> Review of attachment 224424 [details] [review]:
> 
> ::: data/00-upstream-settings
> @@ +12,3 @@
>  [org/gnome/desktop/background]
>  show-desktop-icons=false
> +picture-uri=@backgrounddir@/Noise.png
> 
> So this means the screen shield (at the login screen) is going to have a noise
> texture background too, yea? Also, the fallback greeter will.
> 
> Not sure that's right.

Fallback greeter sounds "correct" to me. Didn't think of the shield situation, but it's better than having g-s-d put the default wallpaper on the screen for the rest of time IMO.
Comment 20 Jasper St. Pierre (not reading bugmail) 2012-09-19 06:42:22 UTC
Created attachment 224699 [details] [review]
layout: Raise the panel back up when logging in
Comment 21 Matthias Clasen 2012-09-19 10:25:40 UTC
(In reply to comment #19)

> 
> Fallback greeter sounds "correct" to me. Didn't think of the shield situation,
> but it's better than having g-s-d put the default wallpaper on the screen for
> the rest of time IMO.

No.
We can't have a gray shield on top of a gray lock dialog.
Comment 22 Jasper St. Pierre (not reading bugmail) 2012-09-19 11:22:27 UTC
Our only other approach is to tell g-s-d to do something fancy.
Comment 23 Ray Strode [halfline] 2012-09-19 18:38:13 UTC
how about:

1) start with the screen shield down briefly (which looks like the background so should transition well from g-s-d before the shell is started)

2) raise the screen shield

3) on exit lower the screen shield

4) then it should transition back to the g-s-d background okay when the shield disappears

5) repeat 1) for the user session.
Comment 24 Matthias Clasen 2012-09-21 13:08:40 UTC
I don't think we'll get anything for 3.6 here - time to drop this off the blocker list ?
Comment 25 Jakub Steiner 2012-09-26 12:43:52 UTC
Here's an updated mockup of a transition from the authorization to the loaded desktop. 

http://jimmac.fedorapeople.org/gnome3/login-gray-fadein.webm

The form fades out to the system grey texture, followed by the desktop zooming in from the center.
Comment 26 Ray Strode [halfline] 2012-09-28 14:39:00 UTC
so we could keep the cow mapped after the login screen exits while the desktop loads underneath, and then have gnome-shell from the user session wait until the SessionRunning signal to zoom itself in.

The downside is, if the user logs into a session that doesn't pick up the pieces they won't be able to see their session after it's loaded, so we'll probably need to make gdm tell the login dialog whether or not the session can support the feature (which means probably adding a new key to the X session desktop file.)
Comment 27 Giovanni Campagna 2012-10-26 15:19:20 UTC
How would you keep the COW mapped? Docs say that it is unmapped automatically after the last client using it disconnects from X.

What we could do instead is paint the COW on the root window (or _XROOTPMAP_ID) as the last step in the fading shell, and use that for the animation.
Comment 28 Ray Strode [halfline] 2012-10-26 17:36:57 UTC
Presumably XAddToSaveSet / XSetCloseDownMode will work (though I haven't tried it, so who knows)
Comment 29 Jean-François Fortin Tam 2012-10-31 00:34:56 UTC
*** Bug 682538 has been marked as a duplicate of this bug. ***
Comment 30 Ray Strode [halfline] 2012-11-01 19:31:59 UTC
*** Bug 685295 has been marked as a duplicate of this bug. ***
Comment 31 Diogo Campos 2012-11-08 01:31:09 UTC
Hey guys, I can give a suggestion about the animation of the login screen?

I believe she could behave * exactly * like the lock screen: ie, rolling up, and revealing the user's desktop.

It would be something like shown in this video...
http://jimmac.fedorapeople.org/gnome3/login.webm
...only without the user's desktop "from below and growing".

(I'm new here, and I think before posting this comment I clicked a wrong button, sorry)
:)
Comment 32 Jasper St. Pierre (not reading bugmail) 2012-11-08 01:43:43 UTC
Yeah, this is the goal. The hard part is implementing it.
Comment 33 Diogo Campos 2012-11-08 16:17:17 UTC
The goal really is *without* the "fade in"? If yes: nice!

The animation is done by Javascript / CSS? If yes: I would like to try to help in my spare time.
Comment 34 Jasper St. Pierre (not reading bugmail) 2012-11-08 16:58:04 UTC
If it was done by JavaScript/CSS, it would be easy. The issue is what happens when we're killing the gdm greeter, before the shell itself starts.
Comment 35 Diogo Campos 2012-11-08 18:23:16 UTC
Ok, I read all the bug again to try to understand it better, but I could not understand *exactly* what is the technical difficulty. So I'll try two variations of a suggestion:

First, a question: GDM can keep itself "alive" even after the user clicks the login button?

IF YES: GDM could "fade out" the login information and show a "spinner" while waiting Gnome Shell is ready. Then, when ready, Gnome Shell warn GDM to roll it up.

IF NOT: GDM could "fade out" the login information (until leave the screen all clean, with that gray) and up close. So, Gnome Shell appear immediately with that *same* gray screen (ie, nothing has changed to the user), "fade in" a "spinner" and, when ready, roll up the gray screen showing his "true face."

Note: in both methods, I includes loading the top panel, but would not be required.

What do you think? I understand something wrong?

(yes, I'm horrible at English, but wanted very much to participate and help. So, thanks for the attention)
Comment 36 Ray Strode [halfline] 2012-11-08 18:38:51 UTC
The login screen can't keep itself alive after the user clicks the login button, but we also don't want to fade to gray.   Instead we'd like it leave a dead, freeze frame of itself on screen and have gnome-shell from the users session pick up the peices once it's started, and finish the animation.
Comment 37 Giovanni Campagna 2012-12-24 15:09:17 UTC
*** Bug 682428 has been marked as a duplicate of this bug. ***
Comment 38 Giovanni Campagna 2012-12-24 15:11:48 UTC
I started to work on this, and got to a somehow acceptable state. The major problem is that the animation is sluggish because there is a lot of IO happening at startup.
Comment 39 Giovanni Campagna 2012-12-24 15:12:44 UTC
Comment on attachment 224423 [details] [review]
backgrounds: Add the noise texture

Rejecting this, the configured background must stay Adwaita because of the screenshield.
Comment 40 Giovanni Campagna 2012-12-24 15:13:18 UTC
Comment on attachment 224424 [details] [review]
data: Use a noise texture for the gdm user's background

Rejecting this, the configured background must stay Adwaita because of the
screenshield.
Comment 41 Giovanni Campagna 2012-12-24 15:14:14 UTC
Created attachment 232196 [details] [review]
Main: move the stage hierarchy initialization to LayoutManager

It is cleaner to concentrate all layout and chrome management
in one place, instead of having it scattered around the codebase.
Comment 42 Giovanni Campagna 2012-12-24 15:14:27 UTC
Created attachment 232197 [details] [review]
Util: add an accessor for the root background pixmap

The background of the root window is accessible as the _XROOTPMAP_ID
property on the root window. Given that the root window is always
fully covered by the Composite Overlay Window, we use that as temporary
storage while the COW is not mapped, that is, before the GDM greeter
is started and between killing the greeter and starting GDM from the
session.
Comment 43 Giovanni Campagna 2012-12-24 15:14:35 UTC
Created attachment 232198 [details] [review]
LayoutManager: implement new design for the startup animation

The design calls for two different startup animations, in the GDM
and in the normal session case.
In both cases, we implement fading from the previous situation
by acquiring the root background pixmap and turning it into a TFP
texture, which is then animated and blended by Clutter as normal.
Comment 44 Giovanni Campagna 2012-12-24 16:35:00 UTC
I also tried to implement the save to _XROOTPMAP_ID part, but I found many different strategies and I'm not sure which one to follow:

- we can push an offscreen framebuffer targetting the pixmap and force Clutter to do a paint cycle
- we can do a glReadPixels into CPU memory and then submit the result as a pixmap with XPutImage (or equivalent)
- we can do a glReadPixels into a buffer object and use that to fill a TFP texture with glTexSubImage2D
- we can do a glBlitFramebuffer binding the COW as the read buffer and a TFP texture as the write buffer
- we can bind the COW to a texture and use that to render on the pixmap (turned into an FBO)
- we can copy the current contents of the COW into pixmap using XCopyArea (or XRender)

Caveats:
- Clutter doesn't like being forced to paint, and pushing a framebuffer around a paint messes with nested FBO redirection and picks.
- a glReadPixels into CPU memory sucks from a performance perspective, and should be avoided at all costs
- glTexSubImage2D is not guaranteed to work
- glBlitFramebuffer is not supported by all drivers, and not wrapped by cogl (also, it might require flipping one of the axis)
- I did not check this, but docs say that the COW cannot be redirected, and only pixmaps can be bound to textures
- XCopyArea requires setting up legacy X11 state (such as GC), which is not worth the code and complexity

If noone has a better idea, I'll implement the XCopyArea path, using cairo to wrap the actual graphic calls.
Comment 45 Ray Strode [halfline] 2013-01-10 22:48:00 UTC
I'm having fun playing with these patches.  it looks pretty slick.  I think we should just ignore the root window property entirely, and instead get our initial frame from a screenshot.

I'll attach that. 

Then, I think we could drop meta-background-actor and implement gnome-bg-like functionality in javascript.
Comment 46 Ray Strode [halfline] 2013-01-10 22:51:25 UTC
I'll just reattach all three since i got some merge conflicts anyway.
Comment 47 Ray Strode [halfline] 2013-01-10 22:51:52 UTC
Created attachment 233194 [details] [review]
Main: move the stage hierarchy initialization to LayoutManager

It is cleaner to concentrate all layout and chrome management
in one place, instead of having it scattered around the codebase.
Comment 48 Ray Strode [halfline] 2013-01-10 22:51:55 UTC
Created attachment 233195 [details] [review]
Util: add an accessor for getting still frame

This gives us the state of the screen right when
gnome-shell is starting, so we can use it for
transition purposes.
Comment 49 Ray Strode [halfline] 2013-01-10 22:51:59 UTC
Created attachment 233196 [details] [review]
LayoutManager: implement new design for the startup animation

The design calls for two different startup animations, in the GDM
and in the normal session case.
In both cases, we implement fading from the previous situation
by acquiring the root background pixmap and turning it into a TFP
texture, which is then animated and blended by Clutter as normal.
Comment 50 Ray Strode [halfline] 2013-01-11 23:00:52 UTC
So i've started on moving gnome-bg here:

http://git.gnome.org/browse/gnome-shell/commit/?h=wip/background-rework&id=509afe66939d98b563eb3dcc46649f9d732709c1

Still has a ways to go. I'll keep hacking on it.
Comment 51 Jasper St. Pierre (not reading bugmail) 2013-01-11 23:23:00 UTC
Review of attachment 233196 [details] [review]:

I'd really like to reduce the amount of "isGreeter"s we have all over. Eventually, I'd prefer it if the gdm codebase, was as I've said before, a separate process that doesn't involve mutter, but just uses the parts of the shell that we'd like it to -- the panel and panel items, the message tray, and a modal dialog.

::: js/ui/layout.js
@@ +167,2 @@
         this.trayBox = new St.Widget({ name: 'trayBox',
+                                       layout_manager: new Clutter.BinLayout() });

Indentation change; bad.

::: js/ui/main.js
@@ +83,2 @@
 function _sessionUpdated() {
+    layoutManager.prepareStartupAnimation()

missing semi
Comment 52 Jasper St. Pierre (not reading bugmail) 2013-01-11 23:26:29 UTC
Review of attachment 233194 [details] [review]:

::: js/ui/layout.js
@@ +121,3 @@
+        this.uiGroup.connect('allocate',
+                        function (actor, box, flags) {
+                            let children = actor.get_children();

This has some funky indentation. It also seems like it's a good time to have a custom layout manager instead of this mess.

::: js/ui/main.js
@@ +127,3 @@
+    layoutManager = new Layout.LayoutManager();
+    // For backward compatibility
+    uiGroup = layoutManager.uiGroup;

This is not just "backwards compatibility". It's "we don't rewrite half the shell codebase".
Comment 53 Jasper St. Pierre (not reading bugmail) 2013-01-11 23:27:15 UTC
Review of attachment 233195 [details] [review]:

OK.
Comment 54 Jasper St. Pierre (not reading bugmail) 2013-01-11 23:36:37 UTC
Created attachment 233263 [details] [review]
Run the startup animation when we don't have much going on

Starting the startup animation when we don't have that much IO
makes it a lot more visible.

Based on a patch by Giovanni Campagna <gcampagna@src.gnome.org>
Comment 55 Giovanni Campagna 2013-01-12 14:48:03 UTC
(In reply to comment #50)
> So i've started on moving gnome-bg here:
> 
> http://git.gnome.org/browse/gnome-shell/commit/?h=wip/background-rework&id=509afe66939d98b563eb3dcc46649f9d732709c1
> 
> Still has a ways to go. I'll keep hacking on it.

I saw your branch. There are two problems with handling the background in the shell/JS, instead of mutter:
- The texture is not shared, so you have to load and paint it twice, once for the desktop and once for the overview.
- MetaBackgroundActor is capable of skipping a repaint when it is covered by windows or panels. This optimization relies on MetaWindowGroup being able to talk to MetaBackgroundActor, which it can't if the actor is a JS thing.

I believe these issues are enough to make unadvisable to move GnomeBG in JS. If we really want to replace GnomeBG, we should implement that in Mutter (where we also have access to Cogl, which we don't in JS).

Another question with your rebase: you implemented the initial frame with a CopyArea from the root window. Does that work when there are windows on the root (say, autostart applications)?

(In reply to comment #52)
> Review of attachment 233194 [details] [review]:
> 
> ::: js/ui/layout.js
> @@ +121,3 @@
> +        this.uiGroup.connect('allocate',
> +                        function (actor, box, flags) {
> +                            let children = actor.get_children();
> 
> This has some funky indentation. It also seems like it's a good time to have a
> custom layout manager instead of this mess.

We need set_skip_paint() (you told me this, the last time).
Comment 56 Jasper St. Pierre (not reading bugmail) 2013-01-12 16:31:30 UTC
Not anymore. You removed the last instance of Main.uiGroup.set_skip_paint:

http://git.gnome.org/browse/gnome-shell/commit/?id=5e865f5bc4bb781da7918bdb9567779b5861d807
Comment 57 Ray Strode [halfline] 2013-01-14 15:14:57 UTC
Hi,

> - The texture is not shared, so you have to load and paint it twice, once for
> the desktop and once for the overview.
We could do some improvements to StTextureCache to help with this, I think, but...

> - MetaBackgroundActor is capable of skipping a repaint when it is covered by
> windows or panels. This optimization relies on MetaWindowGroup being able to
> talk to MetaBackgroundActor, which it can't if the actor is a JS thing.
This is a really good point.  So we have 3 options I guess:

1) keep MetaBackgroundActor in mutter, and move gnome-bg over to mutter
2) keep MetaBackgroundActor in mutter, but set its content from gnome-shell
3) drop MetaBackgroundActor, forward the region up to gnome-shell and do clipping at paint node time.

I think i'm sort of favoring doing the slideshow logic and such from gnome-shell just because it seems more like a compositor feature than a window manager feature.  So i'm sort of in the 2) camp right now.

> Another question with your rebase: you implemented the initial frame with a
> CopyArea from the root window. Does that work when there are windows on the
> root (say, autostart applications)?
Autostarted apps don't get run until afterward.  Right now GDM is doing the same CopyArea thing before the login screen loads anyway, so we can drop it from the GDM side.  Anyway, at user login, conceptually we aren't trying to get the background for our initial animation frame, we're trying to seamlessly make our initial animation frame be the same as the final animation frame of the login screen, so screen shot really is conceptually closer to right.
Comment 58 Ray Strode [halfline] 2013-01-14 15:37:42 UTC
Comment on attachment 233195 [details] [review]
Util: add an accessor for getting still frame

This should probably be per-monitor instead of per-screen so we aren't creating huge pixmaps that are more likely to run into texture size limits.  Also, should probably be in mutter following earlier discussion.
Comment 59 Ray Strode [halfline] 2013-02-04 20:24:42 UTC
Just a heads up, i haven't forgotten about this. I should have some patches soon.
Comment 60 Giovanni Campagna 2013-02-04 21:50:26 UTC
(In reply to comment #59)
> Just a heads up, i haven't forgotten about this. I should have some patches
> soon.

Ah, cause I was starting to rework on this, to integrate GnomeBG in mutter.
I wanted to push a wip/ branch and then send a mail to gnome-shell-list, because this work has implications beyond just this bug, and it's getting confusing.
Comment 61 Giovanni Campagna 2013-02-04 23:13:01 UTC
(Posted the partial stack now, just in case)
Comment 62 Jasper St. Pierre (not reading bugmail) 2013-02-07 09:55:13 UTC
Created attachment 235356 [details] [review]
Main: move the stage hierarchy initialization to LayoutManager

It is cleaner to concentrate all layout and chrome management
in one place, instead of having it scattered around the codebase.
Comment 63 Jasper St. Pierre (not reading bugmail) 2013-02-07 09:55:18 UTC
Created attachment 235357 [details] [review]
layout: Make the uiGroup a standard StWidget

Now that we do not use set_skip_paint, it's possible to do this.
Comment 64 Jasper St. Pierre (not reading bugmail) 2013-02-13 18:51:20 UTC
Created attachment 235942 [details] [review]
Main: move the stage hierarchy initialization to LayoutManager

It is cleaner to concentrate all layout and chrome management
in one place, instead of having it scattered around the codebase.
Comment 65 drago01 2013-02-13 21:08:20 UTC
Review of attachment 235942 [details] [review]:

Looks good, just fix one nitpick regarding that comment.

::: js/ui/main.js
@@ +119,2 @@
     layoutManager = new Layout.LayoutManager();
+    // For backward compatibility

I'd remove this comment or replace it with something else ... this implies that we have grown a stable api that we trying to not break. Maybe "for convenient access" or something like that.
Comment 66 Ray Strode [halfline] 2013-02-13 21:09:02 UTC
Comment on attachment 235357 [details] [review]
layout: Make the uiGroup a standard StWidget

(Jasper said this patch doesn't work yet for him on irc)
Comment 67 Ray Strode [halfline] 2013-02-14 02:50:49 UTC
Comment on attachment 224699 [details] [review]
layout: Raise the panel back up when logging in

(marking obsolete because the mockups leave the panel in place and instead grow the new session from the center of the screen)
Comment 68 Ray Strode [halfline] 2013-02-14 05:39:50 UTC
I know this bug is logically separate form bug 682427, but it's going to be easier if we just handle both issues at the same time over there. 

Marking this bug as depending on that one.  I'll post the patchset there.
Comment 69 Ray Strode [halfline] 2013-02-14 05:50:02 UTC
well actually let's do the gnome-shell half of the patches on this side, and the mutter half on that side.
Comment 70 Ray Strode [halfline] 2013-02-14 05:55:31 UTC
Created attachment 235993 [details] [review]
Main: move the stage hierarchy initialization to LayoutManager

It is cleaner to concentrate all layout and chrome management
in one place, instead of having it scattered around the codebase.
Comment 71 Ray Strode [halfline] 2013-02-14 05:57:17 UTC
Created attachment 235994 [details] [review]
Run the startup animation when we don't have much going on

Starting the startup animation when we don't have that much IO
makes it a lot more visible.

Based on a patch by Giovanni Campagna <gcampagna@src.gnome.org>
Comment 72 Ray Strode [halfline] 2013-02-14 05:57:23 UTC
Created attachment 235995 [details] [review]
layout: rework background handling

This commit updates the code to use mutter's new background
api, and changes the shell's startup animation to be closer
to the mockups.

Based on initial work by Giovanni Campagna
Comment 73 Ray Strode [halfline] 2013-02-14 05:57:30 UTC
Created attachment 235996 [details] [review]
background: add slide show support

gnome-desktop's background drawing code supports an
XML format for presenting backgrounds based on time of day,
monitor geometry, etc. Now that we don't use gnome-desktop for drawing the
background, we need to implement that support ourselves to maintain
feature parity.

This commit implements that.
Comment 74 Ray Strode [halfline] 2013-02-14 05:57:35 UTC
Created attachment 235997 [details] [review]
sessionMode: load internal session modes immediately

We need isGreeter and friends to work in early startup
Comment 75 Ray Strode [halfline] 2013-02-14 05:57:40 UTC
Created attachment 235998 [details] [review]
layout: don't bother hiding window group

Right now we hide the window group if we're a greeter.
We did this to avoid showing the background. These days
we don't add the background in the first place, so we
can drop this code.
Comment 76 Ray Strode [halfline] 2013-02-14 05:57:48 UTC
Created attachment 235999 [details] [review]
loginDialog: get rid of title label

the login screen currently says "Sign In" at the top of it.

The latest mock ups don't have that.

This commit drops it.
Comment 77 Ray Strode [halfline] 2013-02-14 05:57:53 UTC
Created attachment 236000 [details] [review]
loginDialog: drop animations

The latest mockups don't animate between states by
resizing actors. Instead, crossfades are employed.

This commit strips out many of the existing animations
as a first step toward implementing the new ones.
Comment 78 Ray Strode [halfline] 2013-02-14 05:57:59 UTC
Created attachment 236001 [details] [review]
loginDialog: fade out before starting session

Right now we very abruptly kill the login screen
and start the users session without any transition
out.

This commit introduces a fade out of the dialog and
panels.
Comment 79 Ray Strode [halfline] 2013-02-14 05:58:04 UTC
Created attachment 236002 [details] [review]
loginDialog: clone user avatar when doing verification

Right now, when a user item is clicked we remove all other users from
the list and position the item in the appropriate place on screen.

Ultimately, we're going to want to crossfade from the fully populated
list to the user prompt.  Since we're going to need to show the user
avatar in two different positions we can't simply move it.

This commit separates the avatar shown at user verification
into a different actor (a clone).
Comment 80 Ray Strode [halfline] 2013-02-14 05:58:09 UTC
Created attachment 236003 [details] [review]
loginDialog: put "Not Listed?" button and user list in separate container

The user list and the "Not Listed?" button get shown and hidden at the
same time, so we can simplify the code by putting them in a new
subcontainer.

This commit creates a userSelectionBox container that both actors get
put in, and changes all the code that shows and hides these actors to
show and hide userSelectionBox instead.
Comment 81 Ray Strode [halfline] 2013-02-14 05:58:22 UTC
Created attachment 236004 [details] [review]
loginDialog: add cross fade animation between states
Comment 82 Ray Strode [halfline] 2013-02-14 06:13:16 UTC
Comment on attachment 235993 [details] [review]
Main: move the stage hierarchy initialization to LayoutManager

Attachment 235993 [details] pushed as 33dde63 - Main: move the stage hierarchy initialization to LayoutManager

(pushing this one since it's already been looked at above)
Comment 83 Ray Strode [halfline] 2013-02-14 06:16:50 UTC
Review of attachment 235994 [details] [review]:

::: js/ui/main.js
@@ +121,3 @@
+    // Various parts of the shell refer to Main.uiGroup instead of
+    // the newer layoutManager.uiGroup. This line keeps those parts
+    // of code working until they get updated.

woops this was meant to be squashed into the previous patch (which i corrected before i pushed it).  ignore this hunk here.
Comment 84 Jasper St. Pierre (not reading bugmail) 2013-02-14 06:27:37 UTC
Can we separate out the loginDialog and other changes from the background handling?
Comment 85 Jasper St. Pierre (not reading bugmail) 2013-02-14 06:28:28 UTC
Review of attachment 235997 [details] [review]:

This won't work with external session modes.
Comment 86 Jasper St. Pierre (not reading bugmail) 2013-02-14 06:29:21 UTC
Review of attachment 235999 [details] [review]:

::: js/gdm/loginDialog.js
@@ +1238,3 @@
                      },
 
                      new Batch.ConcurrentBatch(this, [this._fadeOutTitleLabel,

You don't seem to remove this?

@@ +1252,3 @@
         let tasks = [this._hidePrompt,
 
                      new Batch.ConcurrentBatch(this, [this._fadeInTitleLabel,

You don't seem to remove this?
Comment 87 Jasper St. Pierre (not reading bugmail) 2013-02-14 06:29:51 UTC
Review of attachment 236000 [details] [review]:

OK.
Comment 88 Jasper St. Pierre (not reading bugmail) 2013-02-14 06:30:45 UTC
Review of attachment 236001 [details] [review]:

Commit message doesn't mention the fade-in.
Comment 89 Jasper St. Pierre (not reading bugmail) 2013-02-14 06:32:22 UTC
Review of attachment 236002 [details] [review]:

I'd prefer it if you just created a new UserListItem. ClutterClone has lots of issues, and we should use it sparingly.
Comment 90 Jasper St. Pierre (not reading bugmail) 2013-02-14 06:33:44 UTC
Review of attachment 236003 [details] [review]:

::: js/gdm/loginDialog.js
@@ +1046,3 @@
     _hideUserListAndLogIn: function() {
+        this._userSelectionBox.hide();
+        this._userList.updateStyle(false);

I think it makes sense to have a setExpanded that sets the visibility of the userSelectionBox in one place.
Comment 91 Jasper St. Pierre (not reading bugmail) 2013-02-14 06:35:32 UTC
Review of attachment 236004 [details] [review]:

My browser hung and froze while Splinter was loading this patch, so I couldn't review it. Whatever settings you are using for git diff aren't suited towards CSS it seems.
Comment 92 Ray Strode [halfline] 2013-02-14 15:27:28 UTC
(In reply to comment #84)
> Can we separate out the loginDialog and other changes from the background
> handling?
Sure.
Comment 93 Ray Strode [halfline] 2013-02-14 15:33:28 UTC
(In reply to comment #86)
> Review of attachment 235999 [details] [review]:
> 
> ::: js/gdm/loginDialog.js
> @@ +1238,3 @@
>                       },
> 
>                       new Batch.ConcurrentBatch(this, [this._fadeOutTitleLabel,
> 
> You don't seem to remove this?
> 
> @@ +1252,3 @@
>          let tasks = [this._hidePrompt,
> 
>                       new Batch.ConcurrentBatch(this, [this._fadeInTitleLabel,
> 
> You don't seem to remove this?

yea, i remove it in the next patch.  will fix.
Comment 94 Ray Strode [halfline] 2013-02-14 15:34:46 UTC
(In reply to comment #89)
> Review of attachment 236002 [details] [review]:
> 
> I'd prefer it if you just created a new UserListItem. ClutterClone has lots of
> issues, and we should use it sparingly.

Sure. Actually, I wonder if it would make sense to just use the widget that's used in the unlock screen, which is already neatly separated.
Comment 95 Ray Strode [halfline] 2013-02-14 15:36:29 UTC
(In reply to comment #90)
> Review of attachment 236003 [details] [review]:
> I think it makes sense to have a setExpanded that sets the visibility of the
> userSelectionBox in one place.

sure.
Comment 96 Ray Strode [halfline] 2013-02-14 15:39:03 UTC
(In reply to comment #91)
> Review of attachment 236004 [details] [review]:
> 
> My browser hung and froze while Splinter was loading this patch, so I couldn't
> review it. Whatever settings you are using for git diff aren't suited towards
> CSS it seems.

Indeed :-) i'll switch from -W to -U 30 or something.  My main motivation for doing things this way (in general, not specifically for this bug) is to provide enough context that a casual reviewer doesn't have to dive into the source to provide useful feedback.
Comment 97 Jasper St. Pierre (not reading bugmail) 2013-02-18 01:20:21 UTC
(In reply to comment #96)
> Indeed :-) i'll switch from -W to -U 30 or something.  My main motivation for
> doing things this way (in general, not specifically for this bug) is to provide
> enough context that a casual reviewer doesn't have to dive into the source to
> provide useful feedback.

Note that I actually did dive into the source several times for this review, even with all this context, because the stuff I want to refer to are usually functions in a separate file.
Comment 98 Ray Strode [halfline] 2013-02-18 04:31:03 UTC
(In reply to comment #97)
> Note that I actually did dive into the source several times for this review,
> even with all this context, because the stuff I want to refer to are usually
> functions in a separate file.
Sure, and that's reasonable.  I just do it to make things easier for drive-by reviewers who are looking through the patch in the web browser, but aren't necessarily going to do a full review / test it out.
Comment 99 Ray Strode [halfline] 2013-02-18 04:35:50 UTC
(In reply to comment #84)
> Can we separate out the loginDialog and other changes from the background
> handling?

I've cloned this bug as bug 689304.  I'll move the loginDialog changes there and keep the background / user-session side stuff here.
Comment 100 Ray Strode [halfline] 2013-02-18 04:36:42 UTC
rather bug 694062 not bug 689304
Comment 101 Ray Strode [halfline] 2013-02-18 04:54:25 UTC
Created attachment 236532 [details] [review]
Run the startup animation when we don't have much going on

Starting the startup animation when we don't have that much IO
makes it a lot more visible.

Based on a patch by Giovanni Campagna <gcampagna@src.gnome.org>
Comment 102 Ray Strode [halfline] 2013-02-18 04:54:29 UTC
Created attachment 236533 [details] [review]
main: defer startup until we know our session mode

commit 92083eaf76fc7a5c2ecdd182896583ab5026ddf0 made
session mode loading an asynchronous operation.

Aspects of the session mode aren't known immediately at
start up. For instance, sessionMode.isGreeter returns
false for greeter sessions until the asynchronous
operation completes.

This commit defers start up processing until the session
mode is fully known.
Comment 103 Ray Strode [halfline] 2013-02-18 04:54:34 UTC
Created attachment 236534 [details] [review]
layout: rework background handling

This commit updates the code to use mutter's new background
api, and changes the shell's startup animation to be closer
to the mockups.

Based on initial work by Giovanni Campagna
Comment 104 Ray Strode [halfline] 2013-02-18 04:54:39 UTC
Created attachment 236535 [details] [review]
background: add slide show support

gnome-desktop's background drawing code supports an
XML format for presenting backgrounds based on time of day,
monitor geometry, etc. Now that we don't use gnome-desktop for drawing the
background, we need to implement that support ourselves to maintain
feature parity.

This commit implements that.
Comment 105 Ray Strode [halfline] 2013-02-18 04:54:43 UTC
Created attachment 236536 [details] [review]
layout: don't bother hiding window group

Right now we hide the window group if we're a greeter.
We did this to avoid showing the background. These days
we don't add the background in the first place, so we
can drop this code.
Comment 106 Jasper St. Pierre (not reading bugmail) 2013-02-18 05:02:21 UTC
Review of attachment 236532 [details] [review]:

So, this is my patch, which is a stripped down version of Giovanni's. I'm assuming that by attaching it into your stack, you're fine with it. Thus, marking as ACN.
Comment 107 Jasper St. Pierre (not reading bugmail) 2013-02-18 05:04:13 UTC
Review of attachment 236533 [details] [review]:

OK.

::: js/ui/main.js
@@ +123,3 @@
     tracker.connect('startup-sequence-changed', _queueCheckWorkspaces);
 
     _loadDefaultStylesheet();

I'd prefer if this was paired with the 'updated' signal above, even if it wasn't before...
Comment 108 Ray Strode [halfline] 2013-02-18 05:34:38 UTC
(In reply to comment #106)
> Review of attachment 236532 [details] [review]:
> 
> So, this is my patch, which is a stripped down version of Giovanni's. I'm
> assuming that by attaching it into your stack, you're fine with it. Thus,
> marking as ACN.

yea
Comment 109 Jasper St. Pierre (not reading bugmail) 2013-02-18 05:49:09 UTC
Review of attachment 236535 [details] [review]:

I'd prefer if you copied the C code from gnome-bg to do this, using GMarkup and everything. I'd be much more comfortable than reimplementing this in JS.

Using the nickname "slide" makes it sound like you're doing a slide transition with things sliding left and right, not a cross-fade slideshow.

::: js/ui/background.js
@@ +625,3 @@
+                if (elements[i].name() == 'size' &&
+                    elements[i].@width &&
+                    elements[i].@height &&

E4X has been removed upstream, and there's plans for new tarballs for JS *today* that have it removed. We should not be adding new code that depends on E4X.
Comment 110 Jasper St. Pierre (not reading bugmail) 2013-02-18 06:12:52 UTC
Review of attachment 236534 [details] [review]:

My initial fears were that this is a lot of code to handle, and it's scary, especially late in the release cycle. After reading it a bit, parts of I really like, and parts of it I don't. I'm not overly happy with the overall approach. Unorganized thoughts below:

I think the split between mutter and gnome-shell is *very* iffy. Why is there a large MetaBackground thing in mutter, but the thing that builds a giant wrapper and generally drives it in gnome-shell? Having this code in C and in mutter would make me happier, and gnome-shell drove it elsewhere where it needed to (overview, screen shield, etc.). I mean, mutter already uses some enums from gsettings directly. It seems that parts of the MetaBackground stuff is 

This also looks like a lot of code was ripped out and moved around at the last minute. I've spotted some dead code.

I don't really like the API for this -- when the background emits a signal, you need to destroy and recreate it. It would be nice if you could just create an actor that handles itself and responds to reloading appropriately. Reloading and re-adding actors seems like it would be ripe for stacking issues.

::: js/ui/background.js
@@ +1,3 @@
+// -*- mode: js; js-indent-level: 4; indent-tabs-mode: nil -*-
+
+const Cairo = imports.cairo;

Unused?

@@ +3,3 @@
+const Cairo = imports.cairo;
+const Clutter = imports.gi.Clutter;
+const GdkPixbuf = imports.gi.GdkPixbuf;

Unused?

@@ +8,3 @@
+const GLib = imports.gi.GLib;
+const Lang = imports.lang;
+const Mainloop = imports.mainloop;

Unused?

@@ +10,3 @@
+const Mainloop = imports.mainloop;
+const Meta = imports.gi.Meta;
+const Shell = imports.gi.Shell;

Unused?

@@ +12,3 @@
+const Shell = imports.gi.Shell;
+const Signals = imports.signals;
+const St = imports.gi.St;

Unused?

@@ +14,3 @@
+const St = imports.gi.St;
+
+const Layout = imports.ui.layout;

Unused?

@@ +15,3 @@
+
+const Layout = imports.ui.layout;
+const Main = imports.ui.main

Unused?

@@ +18,3 @@
+const Params = imports.misc.params;
+const Tweener = imports.ui.tweener;
+const Util = imports.misc.util;

Unused?

@@ +37,3 @@
+
+    _init: function(layoutManager) {
+       this._mats = [];

You don't ever define what a "mat" is. There's already a thing called a "CoglMaterial", so this is a bit confusing to see. I would rename this to "pattern" along with a comment like "// solid color or gradient"

@@ +40,3 @@
+       this._images = [];
+       this._fileMonitors = {};
+       this._layoutManager = layoutManager;

You don't seem to use this?

@@ +107,3 @@
+                                           }
+
+                                           GLib.source_remove(signalId);

Should be monitor.disconnect, or am I wrong?

@@ +180,3 @@
+                                                      content.load_file_finish(result);
+
+                                                      this._monitorFile(params.filename);

Can we have the MetaBackground monitor files instead?

@@ +210,3 @@
+
+        this._settings = new Gio.Settings({ schema: BACKGROUND_SCHEMA });
+        this._layoutManager = params.layoutManager;

Seems to be unused as well.

@@ +231,3 @@
+
+    destroy: function() {
+        if (this._slideUpdateTimeoutId) {

Bad rebase?

@@ +259,3 @@
+                this._cache.removeImageContent(this._image.content);
+
+            this._image..destroy();

???

Was this tested?

@@ +294,3 @@
+                                                  shadingType: shadingType });
+
+        this._mat = new Meta.BackgroundActor(global.screen, this._monitorIndex);

JS constructor syntax isn't like this.

@@ +313,3 @@
+    },
+
+    _addActorFromContent: function(content) {

Only used in _addImage.

@@ +315,3 @@
+    _addActorFromContent: function(content) {
+        let actor = new Meta.BackgroundActor(global.screen,
+                                             this._monitorIndex);

JS constructor syntax isn't like this.

@@ +322,3 @@
+    },
+
+    _addImage: function(content, filename) {

Bad function name.

@@ +357,3 @@
+        this._cache = getBackgroundCache(this._layoutManager);
+
+        this._loadMat(this._cache);

Why are we loading the mat if we have an image? To display something until the image loads?

@@ +366,3 @@
+
+        let uri = this._settings.get_string(PICTURE_URI_KEY);
+        let filename = Gio.File.new_for_uri(uri).get_path();

This seems weird to me. It's a URI for a reason. Are we not allowed to use http:// or load images over smb:// ? :)

(I wonder if we could/should use g_filename_to_uri instead, to avoid building a GFile object)

@@ +374,3 @@
+        if (!this.actor.visible) {
+            this.actor.opacity = 0;
+            this.actor.show();

I'm confused about the state management here. It seems that opacity and visibility should be handled by users of Background itself, so I'm not sure providing this utility is for the best.

::: js/ui/screenShield.js
@@ +59,3 @@
     return 'texel += texture2D (sampler, tex_coord.st + pixel_step * ' +
         'vec2 (' + offx + ',' + offy + '));\n'
 }

This can be removed as well.

::: src/Makefile.am
@@ +289,2 @@
 Shell-0.1.gir: libgnome-shell.la St-1.0.gir
+Shell_0_1_gir_INCLUDES = Clutter-1.0 ClutterX11-1.0 Meta-3.0 TelepathyGLib-0.12 TelepathyLogger-0.2 Soup-2.4 GMenu-3.0 NetworkManager-1.0 NMClient-1.0 xlib-2.0

???

::: src/shell-util.c
@@ +11,3 @@
 #include <gdk-pixbuf/gdk-pixbuf.h>
+#include <gdk/gdkx.h>
+#include <X11/Xatom.h>

???

What are these includes for?

::: src/shell-util.h
@@ +8,3 @@
 #include <libsoup/soup.h>
 #include <gdk-pixbuf/gdk-pixbuf.h>
+#include <gdk/gdkx.h>

???

What are these includes for?
Comment 111 Jasper St. Pierre (not reading bugmail) 2013-02-18 06:13:16 UTC
Review of attachment 236536 [details] [review]:

OK.
Comment 112 Ray Strode [halfline] 2013-02-18 07:19:41 UTC
(In reply to comment #110)

> I think the split between mutter and gnome-shell is *very* iffy. Why is there a
> large MetaBackground thing in mutter, but the thing that builds a giant wrapper
> and generally drives it in gnome-shell?
MetaBackground is in mutter so that we avoid drawing under windows using the meta-window-group machinery.

We talked about this on irc, but just to jot it down here... The wrapper thing is necessary because backgrounds have two parts 1) a mat (pattern) and 2) an image sitting on top. The mat may show through if the image isn't stretched to cover the screen.

The reason the two are managed in the compositor is because they are being composed together.  It's doing what a compositor is conceptually supposed to do. It's where it "fits" better.

> This also looks like a lot of code was ripped out and moved around at the last
> minute. I've spotted some dead code.
yea, those mistakes were embarassing :-)  Most of them were transient fallout in the intermediate patch from my splitting the "slide show" part from the rest.

> I don't really like the API for this -- when the background emits a signal, you
> need to destroy and recreate it. It would be nice if you could just create an
> actor that handles itself and responds to reloading appropriately. Reloading
> and re-adding actors seems like it would be ripe for stacking issues.
That gets rid of a lot of the advantages of doing this. One of the main point advantages is we get actors in the scene that the compositor can choose the  effects for.  If the actor manages the transition itself, then the thing using the actor doesn't get a say in how to transition from one to the next.

> @@ +357,3 @@
> +        this._cache = getBackgroundCache(this._layoutManager);
> +
> +        this._loadMat(this._cache);
> 
> Why are we loading the mat if we have an image? To display something until the
> image loads?

To display something around the part of the image that doesn't fill the screen.
(if relevant)

> +        let uri = this._settings.get_string(PICTURE_URI_KEY);
> +        let filename = Gio.File.new_for_uri(uri).get_path();
> 
> This seems weird to me. It's a URI for a reason. Are we not allowed to use
> http:// or load images over smb:// ? :)
Nope, that doesn't work.  

> (I wonder if we could/should use g_filename_to_uri instead, to avoid building a
> GFile object)
you mean g_filename_from_uri i guess? sure. though we use GFile everywhere else in the shell code.

> I'm confused about the state management here. It seems that opacity and
> visibility should be handled by users of Background itself, so I'm not sure
> providing this utility is for the best.
Okay i'll move it to the callers. I put fadeIn there because fadeOut was there, and I put fadeOut there because it's called in a couple places. but it's not that much code, so it doesn't really matter.
Comment 113 Bastien Nocera 2013-02-18 07:40:14 UTC
(In reply to comment #109)
> Review of attachment 236535 [details] [review]:
> 
> I'd prefer if you copied the C code from gnome-bg to do this, using GMarkup and
> everything. I'd be much more comfortable than reimplementing this in JS.

Yes, please, then we can trim GnomeBg to only do the things that it needs to (we can probably remove the unused transitions code for example).
Comment 114 Jasper St. Pierre (not reading bugmail) 2013-02-18 08:02:51 UTC
(In reply to comment #112)
> We talked about this on irc, but just to jot it down here... The wrapper thing
> is necessary because backgrounds have two parts 1) a mat (pattern) and 2) an
> image sitting on top. The mat may show through if the image isn't stretched to
> cover the screen.

Right, I didn't realize we had that option. I've never seen it in practice.

> The reason the two are managed in the compositor is because they are being
> composed together.  It's doing what a compositor is conceptually supposed to
> do. It's where it "fits" better.

I think where we disagree is what part of things is "a compositor". I'd say that mutter and gnome-shell combine to make one compositor, and the module split is simply because of historical reasons. And Ray here thinks that gnome-shell is the true compositor.

> That gets rid of a lot of the advantages of doing this. One of the main point
> advantages is we get actors in the scene that the compositor can choose the 
> effects for.  If the actor manages the transition itself, then the thing using
> the actor doesn't get a say in how to transition from one to the next.

If something wants special handling, we should make it easier to modify or extend Background. Background is already something that handles slideshows itself, attaches itself to settings, etc. so making it half-immutable, half-self-managed seems inconsistent. I'd also say that Background is not an actor that you "drive", it's a convenient way to have an efficient way to render "the background" as if you were a standard background actor. You do "new Background.Background()", and that's it.

And when we *do* want to add transitions, we'll have to add the same transitions everywhere that uses Background (overview.js, screenShield.js, workspaceThumbnail.js).
Comment 115 Ray Strode [halfline] 2013-02-18 08:10:34 UTC
Created attachment 236558 [details] [review]
layout: rework background handling

This commit updates the code to use mutter's new background
api, and changes the shell's startup animation to be closer
to the mockups.

Based on initial work by Giovanni Campagna
Comment 116 Jasper St. Pierre (not reading bugmail) 2013-02-18 09:14:43 UTC
Review of attachment 236558 [details] [review]:

I think the new startup animations should be split out into another patch, but it's manageable as is if it's too difficult.

::: js/ui/background.js
@@ +9,3 @@
+const Signals = imports.signals;
+
+const Main = imports.ui.main

Still unused?

@@ +353,3 @@
+    set saturation(saturation) {
+        this._saturation = saturation;
+

either put whitespace elsewhere or remove this one

@@ +366,3 @@
+
+    set brightness(factor) {
+        this._brightness = factor;

either use "factor" elsewhere or use "brightness" here

@@ +392,3 @@
+    Name: 'StillFrame',
+
+    _init: function(monitor) {

monitorIndex for consistency

::: js/ui/layout.js
@@ +576,3 @@
+        for (let i = 0; i < this.monitors.length; i++) {
+
+            let monitor = this.monitors[i];

whitespace

@@ +610,2 @@
+    _startupAnimationGreeter: function() {
+         this._freezeUpdateRegions();

Perhaps the freezeUpdateRegions should go in startupAnimation?

::: js/ui/main.js
@@ +71,2 @@
 function _sessionUpdated() {
+    layoutManager.prepareStartupAnimation()

Bad rebase?

::: js/ui/overview.js
@@ +25,2 @@
 // Time for initial animation going into Overview mode
+const ANIMATION_TIME = .25;

???

@@ +200,3 @@
+        background.actor.set_position(monitor.x, monitor.y);
+        background.actor.set_size(monitor.width, monitor.height);
+        background.actor.lower_bottom();

I can see stacking issues here.

@@ +219,3 @@
+            }
+        }
+        this._backgrounds = {};

As monitors is a dense array, I think this._backgrounds should be one too.

@@ +233,3 @@
+        let keys = Object.keys(this._backgrounds);
+        for (let i = 0; i < keys.length; i++) {
+            this._backgrounds[keys[i]].actor.hide();

Wouldn't it be great if we had some way to Group actors together?

::: js/ui/screenShield.js
@@ +525,3 @@
 
+    _updateBackground: function() {
+        let background = new Background.Background({ monitorIndex: Main.layoutManager.primaryIndex,

So, uh, you do know the shield covers all monitors, right? I know there's bugs about it, but we probably shouldn't make it worse.

@@ +527,3 @@
+        let background = new Background.Background({ monitorIndex: Main.layoutManager.primaryIndex,
+                                                     effects: Meta.BackgroundEffects.BLUR | Meta.BackgroundEffects.DESATURATE });
+        background.actor.add_constraint(new Layout.MonitorConstraint({primary: true }));

bad whitespace

@@ +534,3 @@
+        let signalId = background.connect('changed',
+                                          Lang.bind(this, function() {
+                                              background.disconnect(signalId);

OK, cool, so when the shield is up and something changes (some cron job updates ~/Pictures/NewYork.jpg or something) we don't actually animate or anything, we just shove in the new image.
Comment 117 Ray Strode [halfline] 2013-02-18 16:33:40 UTC
(In reply to comment #116)

> ::: js/ui/screenShield.js
> @@ +525,3 @@
> 
> +    _updateBackground: function() {
> +        let background = new Background.Background({ monitorIndex:
> Main.layoutManager.primaryIndex,
> 
> So, uh, you do know the shield covers all monitors, right? I know there's bugs
> about it, but we probably shouldn't make it worse
Well, it's actually a litte better feel than before since we don't see giant spanning shield of doom (bug 688309) but i'll throw a background on each head so we can keep the same look as now.
Comment 118 Ray Strode [halfline] 2013-02-18 16:34:28 UTC
Created attachment 236623 [details] [review]
layout: rework background handling

This commit updates the code to use mutter's new background
api, and changes the shell's startup animation to be closer
to the mockups.

Based on initial work by Giovanni Campagna
Comment 119 Ray Strode [halfline] 2013-02-18 16:38:12 UTC
Created attachment 236625 [details] [review]
layout: rework background handling

This commit updates the code to use mutter's new background
api, and changes the shell's startup animation to be closer
to the mockups.

Based on initial work by Giovanni Campagna
Comment 120 Ray Strode [halfline] 2013-02-18 16:40:37 UTC
Created attachment 236626 [details] [review]
background: add slide show support

gnome-desktop's background drawing code supports an
XML format for presenting backgrounds based on time of day,
monitor geometry, etc. Now that we don't use gnome-desktop for drawing the
background, we need to implement that support ourselves to maintain
feature parity.

This commit implements that.
Comment 121 Ray Strode [halfline] 2013-02-18 16:41:13 UTC
I still need to figure out a story en lieu of E4x
Comment 122 Ray Strode [halfline] 2013-02-18 20:38:33 UTC
Attachment 236532 [details] pushed as 1950a67 - Run the startup animation when we don't have much going on
Attachment 236533 [details] pushed as 65303d0 - main: defer startup until we know our session mode
Comment 123 Ray Strode [halfline] 2013-02-18 22:10:51 UTC
Created attachment 236671 [details] [review]
layout: rework background handling

This commit updates the code to use mutter's new background
api, and changes the shell's startup animation to be closer
to the mockups.

Based on initial work by Giovanni Campagna
Comment 124 Ray Strode [halfline] 2013-02-18 22:10:56 UTC
Created attachment 236672 [details] [review]
background: add slide show support

gnome-desktop's background drawing code supports an
XML format for presenting backgrounds based on time of day,
monitor geometry, etc. Now that we don't use gnome-desktop for drawing the
background, we need to implement that support ourselves to maintain
feature parity.

This commit implements that.
Comment 125 Giovanni Campagna 2013-02-18 23:10:43 UTC
*** Bug 694132 has been marked as a duplicate of this bug. ***
Comment 126 Ray Strode [halfline] 2013-02-18 23:42:18 UTC
Created attachment 236686 [details] [review]
layout: rework background handling

This commit updates the code to use mutter's new background
api, and changes the shell's startup animation to be closer
to the mockups.

Based on initial work by Giovanni Campagna
Comment 127 Ray Strode [halfline] 2013-02-18 23:42:26 UTC
Created attachment 236687 [details] [review]
background: add slide show support

gnome-desktop's background drawing code supports an
XML format for presenting backgrounds based on time of day,
monitor geometry, etc. Now that we don't use gnome-desktop for drawing the
background, we need to implement that support ourselves to maintain
feature parity.

This commit implements that.
Comment 128 Ray Strode [halfline] 2013-02-18 23:54:06 UTC
Created attachment 236688 [details] [review]
background: add slide show support

gnome-desktop's background drawing code supports an
XML format for presenting backgrounds based on time of day,
monitor geometry, etc. Now that we don't use gnome-desktop for drawing the
background, we need to implement that support ourselves to maintain
feature parity.

This commit implements that.
Comment 129 Ray Strode [halfline] 2013-02-19 21:16:07 UTC
Created attachment 236840 [details] [review]
layout: rework background handling

This commit updates the code to use mutter's new background
api, and changes the shell's startup animation to be closer
to the mockups.

Based on initial work by Giovanni Campagna
Comment 130 Ray Strode [halfline] 2013-02-19 21:16:18 UTC
Created attachment 236841 [details] [review]
background: add slide show support

gnome-desktop's background drawing code supports an
XML format for presenting backgrounds based on time of day,
monitor geometry, etc. Now that we don't use gnome-desktop for drawing the
background, we need to implement that support ourselves to maintain
feature parity.

This commit implements that.
Comment 131 Jasper St. Pierre (not reading bugmail) 2013-02-19 21:25:35 UTC
Review of attachment 236840 [details] [review]:

Getting very close... ScreenShield should use the manager too.

::: js/ui/background.js
@@ +414,3 @@
+
+const Manager = new Lang.Class({
+    Name: 'Manager',

This should be 'BackgroundManager'

@@ +422,3 @@
+                                        effects: Meta.BackgroundEffects.NONE });
+
+        this._container = params.container;

You should provide an explicit destroy() function that disconnects the background signals that the WorkspaceThumbnail / ScreenShield will call when it's destroyed, to prevent the Backgrounds from sticking around.

::: js/ui/overview.js
@@ +179,3 @@
+
+        for (let i = 0; i < Main.layoutManager.monitors.length; i++) {
+            let bgManager = new Background.Manager({ container: this._backgroundGroup,

It seems like this could get GC'd at any time. I'd want to root it.

I was also imagining that it would have a BackgroundGroup instead of being passed in a BackgroundGroup, which I think makes slightly more sense, but if we don't want to do that, OK.
Comment 132 Jasper St. Pierre (not reading bugmail) 2013-02-19 21:26:44 UTC
Review of attachment 236841 [details] [review]:

OK.

::: js/ui/background.js
@@ +221,3 @@
 Signals.addSignalMethods(BackgroundCache.prototype);
 
+function getBackgroundCache(layoutManager) {

Bad rebase?
Comment 133 Ray Strode [halfline] 2013-02-19 23:03:39 UTC
Created attachment 236852 [details] [review]
layout: rework background handling

This commit updates the code to use mutter's new background
api, and changes the shell's startup animation to be closer
to the mockups.

Based on initial work by Giovanni Campagna
Comment 134 Ray Strode [halfline] 2013-02-19 23:03:44 UTC
Created attachment 236853 [details] [review]
background: add slide show support

gnome-desktop's background drawing code supports an
XML format for presenting backgrounds based on time of day,
monitor geometry, etc. Now that we don't use gnome-desktop for drawing the
background, we need to implement that support ourselves to maintain
feature parity.

This commit implements that.
Comment 135 Jasper St. Pierre (not reading bugmail) 2013-02-19 23:10:33 UTC
Review of attachment 236853 [details] [review]:

Almost.

::: js/ui/background.js
@@ +248,3 @@
+        // contains a single image for static backgrounds and
+        // two images (from and to) for slide shows
+        this._images = {};

Hm, again, this contains numeric keys, so using an array makes more sense.
Comment 136 Jasper St. Pierre (not reading bugmail) 2013-02-19 23:13:13 UTC
Review of attachment 236852 [details] [review]:

I'm happy with this.
Comment 137 Ray Strode [halfline] 2013-02-19 23:23:10 UTC
(In reply to comment #135)
> ::: js/ui/background.js
> @@ +248,3 @@
> +        // contains a single image for static backgrounds and
> +        // two images (from and to) for slide shows
> +        this._images = {};
> 
> Hm, again, this contains numeric keys, so using an array makes more sense.
The images are load asynchronously, but always need to be placed at a specific location in the array, since the order matters.  A list isn't really right for this.

If it were indexed by order images loaded instead of by monitor then a list would be right.
Comment 138 Ray Strode [halfline] 2013-02-19 23:41:07 UTC
Attachment 236536 [details] pushed as ba4780c - layout: don't bother hiding window group
Attachment 236852 [details] pushed as 3c8325f - layout: rework background handling
Attachment 236853 [details] pushed as b1a6940 - background: add slide show support
Comment 139 Jasper St. Pierre (not reading bugmail) 2013-02-20 19:08:03 UTC
Is this fixed?
Comment 140 Jakub Steiner 2013-04-17 17:03:21 UTC
Reopening to address the zoom transition.

We have discussed this with ryan and garrett extensively during LGM and we've
come up with a transition that avoids the jarring zoom and keeps the system
layer from being composited over the desktop.

I've mocked it up and provided a small infographic about the different layers
in the following video:

http://www.youtube.com/watch?v=Dcr0ke2UeaI

The slide happens only on the primary display. Secondary displays continue to
fade like they do now, to avoid the complexity of the physical layout.
Comment 141 Jakub Steiner 2013-04-19 14:33:22 UTC
Further discussion with Allan pointed out it feels more like the session pushes the auth dialog away, breaking the spacial z-order relation anyway. Thus, we come full circle to the initial transition, where the session is uncovered by the curtain both the curtain and the auth dialog. When using authoentication, the curtain stops at the top and joins the auth dialog layer to fully uncover the session at the end.

http://www.youtube.com/watch?v=QRLGtLhv7kY
Comment 142 Jakub Steiner 2013-05-06 13:46:10 UTC
Discussed the issue further and with the enlarged intial size for the scale transition, the major issue of the vertigo inducing zoom has actually been addressed. We have bigger fish to fry than circling around this.