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.

Created attachment 222106 [details] [review]
layout: Don't unnecessarily go through Main for a property

We *are* Main.layoutManager, here.

Created attachment 222107 [details] [review]
layout: Turn off the startup animation when in a user session

Review of attachment 222106 [details] [review]:

Even if the rest doesn't land, this makes sense

Review of attachment 222105 [details] [review]:

Bah... Breaks git-blame for little...

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 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

Lets attract some designers, then...

Comment on attachment 222105 [details] [review]
sessionMode: Reindent

(Marking patch obsolete that got committed in bug 683156)

Created attachment 224254 [details] [review]
layout: Turn off the startup animation when in a user session

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)

*** Bug 684011 has been marked as a duplicate of this bug. ***

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.

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.

Created attachment 224425 [details] [review]
layout: Raise the panel back up when logging in

Let's try this instead.

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.

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.

*** Bug 684189 has been marked as a duplicate of this bug. ***

(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.

Created attachment 224699 [details] [review]
layout: Raise the panel back up when logging in

(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.

Our only other approach is to tell g-s-d to do something fancy.

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.

I don't think we'll get anything for 3.6 here - time to drop this off the blocker list ?

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.

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.)

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.

Presumably XAddToSaveSet / XSetCloseDownMode will work (though I haven't tried it, so who knows)

*** Bug 682538 has been marked as a duplicate of this bug. ***

*** Bug 685295 has been marked as a duplicate of this bug. ***

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)
:)

Yeah, this is the goal. The hard part is implementing it.

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.

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.

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)

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.

*** Bug 682428 has been marked as a duplicate of this bug. ***

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 on attachment 224423 [details] [review]
backgrounds: Add the noise texture

Rejecting this, the configured background must stay Adwaita because of the screenshield.

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.

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.

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.

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.

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.

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.

I'll just reattach all three since i got some merge conflicts anyway.

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.

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.

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.

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.

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

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".

Review of attachment 233195 [details] [review]:

OK.

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>

(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).

Not anymore. You removed the last instance of Main.uiGroup.set_skip_paint:

http://git.gnome.org/browse/gnome-shell/commit/?id=5e865f5bc4bb781da7918bdb9567779b5861d807

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 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.

Just a heads up, i haven't forgotten about this. I should have some patches soon.

(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.

(Posted the partial stack now, just in case)

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.

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.

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.

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 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 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)

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.

well actually let's do the gnome-shell half of the patches on this side, and the mutter half on that side.

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.

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>

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

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.

Created attachment 235997 [details] [review]
sessionMode: load internal session modes immediately

We need isGreeter and friends to work in early startup

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.

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.

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.

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.

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).

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.

Created attachment 236004 [details] [review]
loginDialog: add cross fade animation between states

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)

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.

Can we separate out the loginDialog and other changes from the background handling?

Review of attachment 235997 [details] [review]:

This won't work with external session modes.

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?

Review of attachment 236000 [details] [review]:

OK.

Review of attachment 236001 [details] [review]:

Commit message doesn't mention the fade-in.

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.

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.

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.

(In reply to comment #84)
> Can we separate out the loginDialog and other changes from the background
> handling?
Sure.

(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.

(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.

(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.

(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.

(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.

(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.

(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.

rather bug 694062 not bug 689304

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>

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.

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

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.

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.

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.

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...

(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

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.

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?

Review of attachment 236536 [details] [review]:

OK.

(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.

(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).

(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).

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

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.

(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.

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

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

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.

I still need to figure out a story en lieu of E4x

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

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

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.

*** Bug 694132 has been marked as a duplicate of this bug. ***

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

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.

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.

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

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.

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.

Review of attachment 236841 [details] [review]:

OK.

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

Bad rebase?

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

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.

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.

Review of attachment 236852 [details] [review]:

I'm happy with this.

(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.

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

Is this fixed?

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.

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

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.