GNOME Bugzilla – Bug 688210
Use a different image for the screen shield
Last modified: 2013-08-20 12:13:08 UTC
I think this has been in the lock screen designs since for ever. Using a different background image in the lock screen would distinguish it from the session, and would give people a bit more control over how they present themselves to others (since the lock screen wallpaper is more public than the session one). It might also be interesting to larger scale deployments or companies, who might want to lock down the lock screen wallpaper.
Yes, this is something I had in mind to do from the first day. It is blocked on bug 682427 to draw the background in mutter instead of gnome-settings-daemon.
Created attachment 231766 [details] [review] Compositor: remove the hidden group It is unused and always empty.
Created attachment 231767 [details] [review] MetaBackground: untie background rendering from MetaScreen Rather than associating background textures with a MetaScreen, associate them with GnomeBG objects. This allows to have different actors that render different backgrounds, without losing the ability to share textures.
Created attachment 231768 [details] [review] MetaWindowGroup: store aside the background actor This allows to retrieve the background actor from the window group (which is useful for a plugin), and to replace some of the expensive GType checks with a pointer comparison. Not all of them because sadly we allow plugins to put random stuff in the window_group...
Created attachment 231770 [details] [review] Remove deprecated Clutter symbols This solves a number of compiler warnings.
Created attachment 231771 [details] [review] LayoutManager: Remove broken startup animation It doesn't make sense to animate blindly a MetaBackgroundActor, given that it shows the content of _XROOTPMAP_ID, so if gnome-settings-daemon is fast we're animating the configured background, not the plymouth screen. And anyway it would be animated on top of the standard MetaBackgroundActor... It makes even less sense now that mutter renders the background on its own (and blocks the first paint cycle until the background image is ready) We need to do something better here, but for now, remove this.
Created attachment 231772 [details] [review] Adapt for MetaBackgroundActor API changes MetaBackgroundActor can now be constructed as a standard GObject, and accepts a GnomeBG settings object, which we retrieve from the default one to share textures.
Created attachment 231773 [details] [review] ScreenShield: use different settings for the lock screen background The design calls for the ability to set a different background in the lock screen, to differentiate it from the normal desktop.
Created attachment 231774 [details] [review] ScreenShield: remove blur effect from the background It's badly implemented, slow and actually not needed, now that we have a different image. Let's keep desaturation instead.
This addresses the gnome-shell side of things, and will give you a nice desaturated Adwaita background. The configuration part is still missing, and TBH I haven't seen designs for that - not even which panel it should live in, not that Brightness&Lock is going away. Before testing, take a look at the dependency tree. You'll need patches for mutter, gnome-desktop and gsettings-desktop-schemas.
Review of attachment 231770 [details] [review]: Please don't tell me you wrote this patch.
Review of attachment 231770 [details] [review]: OK, so this is subtly wrong. I already wrote https://bugzilla.gnome.org/show_bug.cgi?id=678917 And there's various issues with the port there. For instance, try changing workspaces using this patch. It will emit a bunch of warnings.
Review of attachment 231766 [details] [review]: Fine by me.
Review of attachment 231767 [details] [review]: Not a full review. ::: src/compositor/meta-background-actor.c @@ +128,3 @@ +static GnomeBG * +meta_background_get_default_settings (void) "meta_background_get_default_gnome_bg" @@ +142,2 @@ + /* Just to keep settings alive */ + g_object_set_data_full (G_OBJECT (bg), "g-settings", You want a weak ref, not this. @@ +334,3 @@ + g_clear_pointer (&priv->single_pipeline, cogl_object_unref); + g_clear_pointer (&priv->crossfade_pipeline, cogl_object_unref); Indentation changes. @@ +818,2 @@ + if (width == background->texture_width && height == background->texture_height) + priv->wrap_mode = COGL_MATERIAL_WRAP_MODE_CLAMP_TO_EDGE; I don't get this at all. Why does the wrap mode make a difference if the two sizes match? ::: src/core/screen.c @@ +239,3 @@ G_SIGNAL_RUN_LAST, G_STRUCT_OFFSET (MetaScreenClass, monitors_changed), + NULL, NULL, NULL, Completely unrelated indentation patch.
Review of attachment 231768 [details] [review]: OK. The property stuff seems irrelevant, but I assume the shell will use it in the future. ::: src/compositor/meta-window-group.c @@ +278,3 @@ continue; + if (l->data != window_group->background && Moving the background check first should prevent this. @@ +331,3 @@ for (l = children; l; l = l->next) { + if (l->data != window_group->background && Moving the background check first should prevent this. @@ +351,3 @@ + MetaBackgroundActor *actor) +{ + self->background = actor; We need to remove the old background actor if there is one.
Review of attachment 231771 [details] [review]: We could go back to having a raw _XROOTPMAP_ID actor. There's a commit that changed it, which you can revert.
Review of attachment 231772 [details] [review]: OK.
Review of attachment 231773 [details] [review]: OK.
Review of attachment 231774 [details] [review]: Wouldn't it be possible to go to a ClutterDesaturationEffect now? :q:
source(In reply to comment #14) > Review of attachment 231767 [details] [review]: > > Not a full review. > > ::: src/compositor/meta-background-actor.c > @@ +128,3 @@ > > +static GnomeBG * > +meta_background_get_default_settings (void) > > "meta_background_get_default_gnome_bg" Horrible name, if you want my opinion. > @@ +142,2 @@ > + /* Just to keep settings alive */ > + g_object_set_data_full (G_OBJECT (bg), "g-settings", > > You want a weak ref, not this. What's the problem with this? I want the GnomeBG object to keep alive the GSettings, so that the signal connection is not disposed. A weak ref on GnomeBG to unref the GSettings would do the same, that's true, but as far as performance is concerned it's not slower, and this way the code is more understandable. > @@ +334,3 @@ > > + g_clear_pointer (&priv->single_pipeline, cogl_object_unref); > + g_clear_pointer (&priv->crossfade_pipeline, cogl_object_unref); > > Indentation changes. > > @@ +818,2 @@ > + if (width == background->texture_width && height == > background->texture_height) > + priv->wrap_mode = COGL_MATERIAL_WRAP_MODE_CLAMP_TO_EDGE; > > I don't get this at all. Why does the wrap mode make a difference if the two > sizes match? Because if the two sizes match, you're rendering a quad that's sized like the whole texture. And because of bilinear filtering, the last pixel is interpolated with the first on the opposite side and becomes wrong. (In reply to comment #19) > Review of attachment 231774 [details] [review]: > > Wouldn't it be possible to go to a ClutterDesaturationEffect now? :q: No. A ClutterDesaturationEffect is a ClutterOffscreenEffect, so it renders the background texture into a temporary texture and desaturates that, which is inefficient and brings no advantage to shading the background texture directly. (In reply to comment #16) > Review of attachment 231771 [details] [review]: > > We could go back to having a raw _XROOTPMAP_ID actor. There's a commit that > changed it, which you can revert. Yes, that was my idea, but anyway we need to do something fancier for 3.8, and that's bug 682428.
source(In reply to comment #12) > Review of attachment 231770 [details] [review]: > > OK, so this is subtly wrong. I already wrote > > https://bugzilla.gnome.org/show_bug.cgi?id=678917 > > And there's various issues with the port there. For instance, try changing > workspaces using this patch. It will emit a bunch of warnings. Ooof, I see them now. Well, ok, I can live without it.
To be clear: the lock screen background should be configurable through the system settings background panel. This contains a button to change the background for the session. There will need to be another button for the lock screen background.
(In reply to comment #20) > Horrible name, if you want my opinion. So, "get_default_settings" returns a not-GSettings? > > @@ +142,2 @@ > and this way the code is more understandable. I don't think so. Usually you use set_data when you want to fetch the settings object again, and a weak ref when you just want to tie the two together.
source(In reply to comment #23) > (In reply to comment #20) > > Horrible name, if you want my opinion. > > So, "get_default_settings" returns a not-GSettings? Well, the property is also called settings, but is of type GnomeBG. After all, GnomeBG is nothing more than a bag of cached GSettings. > > > @@ +142,2 @@ > > and this way the code is more understandable. > > I don't think so. Usually you use set_data when you want to fetch the settings > object again, and a weak ref when you just want to tie the two together. The way I understand it, here we want the GnomeBG object to keep a reference to GSettings. In a GC world, that means a using a reserved slot and tracing. Here we have reference counting instead, but I don't see why it should be conceptually different. A weak ref is used when you want to be notified of the destruction of something, not when you want something to keep alive you.
Where do we stand on this bug? Am I supposed to be waiting for you, or are you supposed to be waiting for me?
It might be waiting on me (and bug 677848). I'd rather we had cleaned up the picture options, but I guess it's something we can postpone if it's blocking all this work.
(In reply to comment #25) > Where do we stand on this bug? Am I supposed to be waiting for you, or are you > supposed to be waiting for me? I think it's first of all waiting for Ray and bug 682427 (but see bug 682429 for the discussion around GnomeBG/MetaBackground)
All of those bugs are closed now. Can we get something in?
Last time this was mentioned, we decided that it is too late to have this in 3.8, because we're missing the required configuration UI.
The configuration would essentially remain the same. An additional button would be added for the lock screen with a distinct top bar and date/time overlay: https://raw.github.com/gnome-design-team/gnome-mockups/master/system-settings/wallpaper/with-lock-screen.png
(In reply to comment #30) > The configuration would essentially remain the same. An additional button would > be added for the lock screen with a distinct top bar and date/time overlay: > > https://raw.github.com/gnome-design-team/gnome-mockups/master/system-settings/wallpaper/with-lock-screen.png Can you please file a separate bug against g-c-c for that?
The gcc bug is bug 696166
Created attachment 252313 [details] [review] Background: allow passing the settings schema from outside This way it is possible to use different settings for different backgrounds.
Created attachment 252314 [details] [review] ScreenShield: use the screensaver background Now that that's configurable in the control center, we should use the appropriate background here.
Yay, the control center side is fixed, and the gnome-shell fix is really a two liner!
(In reply to comment #35) > Yay, the control center side is fixed, and the gnome-shell fix is really a two > liner! Yay indeed :)
Review of attachment 252313 [details] [review]: looks fine. really glad to see this land
Review of attachment 252314 [details] [review]: +
Attachment 252313 [details] pushed as 4413f81 - Background: allow passing the settings schema from outside Attachment 252314 [details] pushed as fdc0832 - ScreenShield: use the screensaver background