GNOME Bugzilla – Bug 682428
plymouth => gdm transition doesn't look very good
Last modified: 2013-02-18 04:34:10 UTC
After plymouth exits and Xorg/gdm starts, you see the standard configured window background just appear out of nowhere. After that, we fade to the system noise texture background. On F18, this is very visible: dark gray, to light blue stripes, to dark gray again. After plymouth exits, it's supposed to leave the contents on the screen. gdm used to (probably still does) save a giant pixmap in _XROOTPMAP_ID for the root window. We should be able to create a TFP texture for that, and fade in our contents on top of that.
Created attachment 222108 [details] [review] layout: Add a fake root pixmap actor at startup, and fade it out This provides us with a smooth transition between plymouth and gdm. Completely untested, but this is the sort of thing I was talking about.
Created attachment 222109 [details] [review] gdm: Don't fade in the log in dialog Instead, directly show it. This too.
Created attachment 222115 [details] [review] layout: Add a fake root pixmap actor at startup, and fade it out This provides us with a smooth transition between plymouth and gdm. Still untested, but doesn't crash the shell anymore. I'd appreciate testing, but I understand that you guys have better things to do at this point.
Created attachment 222116 [details] [review] gdm: Don't fade in the log in dialog Instead, directly show it.
we should turn off g-s-d's crossfade behavior as well I suppose.
*** Bug 682148 has been marked as a duplicate of this bug. ***
Review of attachment 222115 [details] [review]: Can't test it either but generally looks fine some comments below. ::: src/shell-global.c @@ +1758,3 @@ + * supporting raw xlib types. + * + * Returns: (transfer none): A #ClutterActor that represents the _XROOTPMAP_ID Why none? @@ +1775,3 @@ + + if (XGetWindowProperty (global->xdisplay, + xroot_window, Can just use DefaultRootWindow (global->xdisplay) instead. @@ +1791,3 @@ + if (root_pixmap_id != None) + { + g_print ("%ld\n", root_pixmap_id); Remove.
Created attachment 224255 [details] [review] layout: Add a fake root pixmap actor at startup, and fade it out This provides us with a smooth transition between plymouth and gdm.
Created attachment 224256 [details] [review] gdm: Don't fade in the log in dialog Instead, directly show it.
Review of attachment 224255 [details] [review]: Looks good still can't test fine to commit if it works.
Review of attachment 224256 [details] [review]: OK.
Created attachment 224357 [details] [review] gdm: Don't fade in the log in dialog Instead, directly show it.
Comment on attachment 224256 [details] [review] gdm: Don't fade in the log in dialog (ignore my churn on this bug)
Comment on attachment 224256 [details] [review] gdm: Don't fade in the log in dialog Attachment 224256 [details] pushed as cc67440 - gdm: Don't fade in the log in dialog Review on the plymouth patch would be appreciated.
(In reply to comment #7) > Review of attachment 222115 [details] [review]: > > Can't test it either but generally looks fine some comments below. > > ::: src/shell-global.c > @@ +1758,3 @@ > + * supporting raw xlib types. > + * > + * Returns: (transfer none): A #ClutterActor that represents the _XROOTPMAP_ID > > Why none? Because it's the correct annotation. There's an alias, (transfer floating), that might clear things up. Changed. Also, I had a chance to test this. It works, but I'm doing things slightly differently now.
Created attachment 224703 [details] [review] layout: Add a fake root pixmap actor at startup, and fade it out This provides us with a smooth transition between plymouth and gdm.
To clarify, I just ran the transition when restarting the shell, and it worked. I have not tested with real Plymouth, nor have I tested with multiple monitor support.
(In reply to comment #15) > (In reply to comment #7) > > Review of attachment 222115 [details] [review] [details]: > > > > Can't test it either but generally looks fine some comments below. > > > > ::: src/shell-global.c > > @@ +1758,3 @@ > > + * supporting raw xlib types. > > + * > > + * Returns: (transfer none): A #ClutterActor that represents the _XROOTPMAP_ID > > > > Why none? > > Because it's the correct annotation. There's an alias, (transfer floating), > that might clear things up. Changed. clutter_x11_texture_pixmap_new_with_pixmap does: ----------- ClutterActor *actor; actor = g_object_new (CLUTTER_X11_TYPE_TEXTURE_PIXMAP, "pixmap", pixmap, NULL); return actor; ----------- And you are returning that so it ought to be (transfer full) no?
ClutterActor is a GInitiallyUnowned. (transfer full) is defined as "transferring the reference", but since the object is floating, there's no reference to transfer.
Review of attachment 224703 [details] [review]: OK fine then.
Attachment 224703 [details] pushed as 5e12e5f - layout: Add a fake root pixmap actor at startup, and fade it out
I can still see the issue described in the bug description using gnome-shell-3.6.0-1.fc18.x86_64 gdm-3.6.0-1.fc18.x86_64
Still an issue, please re-open. See video: http://doom.co.il/f18-vm-boot.webm from bug #682148: >the problem is gdm itself (or gnome-settings-deamon?) draws the default background on the screen before gdm draws the gray background.
Ouch. Can we disable the background plugin in gnome-settings-daemon in the gdm user? The only issue with that is the lack of proper wallpaper in the shield. I'm not sure how to act with that.
Created attachment 227368 [details] [review] layout: Use a MetaBackgroundActor, not a custom ClutterX11TexturePixmap While looking at how the plymouth implementation was built, I was so short-sighted and focused on the string "_XROOTPMAP_ID" that I didn't realize it was the name of the standard background on the root window. Remove our own implementation, and switch to using a standard mutter MetaBackgroundActor. --- This doesn't fix anything, it's just a minor cleanup.
Review of attachment 227368 [details] [review]: Makes sense.
Comment on attachment 227368 [details] [review] layout: Use a MetaBackgroundActor, not a custom ClutterX11TexturePixmap Attachment 227368 [details] pushed as 85728f0 - layout: Use a MetaBackgroundActor, not a custom ClutterX11TexturePixmap
well we shouldn't need the background plugin to draw in non-fallback cases at all really right? It's not whether or not we're in the gdm session, it's whether or not we're using gnome-shell.
We still need it for the screen shield, don't we?
(In reply to comment #29) > well we shouldn't need the background plugin to draw in non-fallback cases at > all really right? It's not whether or not we're in the gdm session, it's > whether or not we're using gnome-shell. Unless we want to implement cross-fade and everything with GnomeBg in the shell session, we still need it.
right, we definitely want to do that
My point is right now, background handling is very rube goldberg, and we should probably just do it directly in shell for the non fallback case.
Ok, it's time to consolidate this. gnome-settings-daemon's background is (will be) obsoleted by bug 682427, and the plymouth=>gdm animation can be covered in bug 682429. *** This bug has been marked as a duplicate of bug 682429 ***