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 688210 - Use a different image for the screen shield
Use a different image for the screen shield
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: lock-screen
3.7.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on: 677848 682427
Blocks: 696166
 
 
Reported: 2012-11-12 21:51 UTC by Allan Day
Modified: 2013-08-20 12:13 UTC
See Also:
GNOME target: ---
GNOME version: 3.7/3.8


Attachments
Compositor: remove the hidden group (1.88 KB, patch)
2012-12-17 19:32 UTC, Giovanni Campagna
accepted-commit_now Details | Review
MetaBackground: untie background rendering from MetaScreen (27.15 KB, patch)
2012-12-17 19:32 UTC, Giovanni Campagna
needs-work Details | Review
MetaWindowGroup: store aside the background actor (6.29 KB, patch)
2012-12-17 19:32 UTC, Giovanni Campagna
accepted-commit_now Details | Review
Remove deprecated Clutter symbols (7.56 KB, patch)
2012-12-17 19:47 UTC, Giovanni Campagna
rejected Details | Review
LayoutManager: Remove broken startup animation (2.84 KB, patch)
2012-12-17 19:48 UTC, Giovanni Campagna
reviewed Details | Review
Adapt for MetaBackgroundActor API changes (3.65 KB, patch)
2012-12-17 19:48 UTC, Giovanni Campagna
accepted-commit_now Details | Review
ScreenShield: use different settings for the lock screen background (2.07 KB, patch)
2012-12-17 19:48 UTC, Giovanni Campagna
accepted-commit_now Details | Review
ScreenShield: remove blur effect from the background (3.14 KB, patch)
2012-12-17 19:54 UTC, Giovanni Campagna
accepted-commit_now Details | Review
Background: allow passing the settings schema from outside (2.79 KB, patch)
2013-08-19 22:52 UTC, Giovanni Campagna
committed Details | Review
ScreenShield: use the screensaver background (1.13 KB, patch)
2013-08-19 22:53 UTC, Giovanni Campagna
committed Details | Review

Description Allan Day 2012-11-12 21:51:43 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.
Comment 1 Giovanni Campagna 2012-11-19 18:32:03 UTC
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.
Comment 2 Giovanni Campagna 2012-12-17 19:32:06 UTC
Created attachment 231766 [details] [review]
Compositor: remove the hidden group

It is unused and always empty.
Comment 3 Giovanni Campagna 2012-12-17 19:32:22 UTC
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.
Comment 4 Giovanni Campagna 2012-12-17 19:32:34 UTC
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...
Comment 5 Giovanni Campagna 2012-12-17 19:47:24 UTC
Created attachment 231770 [details] [review]
Remove deprecated Clutter symbols

This solves a number of compiler warnings.
Comment 6 Giovanni Campagna 2012-12-17 19:48:15 UTC
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.
Comment 7 Giovanni Campagna 2012-12-17 19:48:31 UTC
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.
Comment 8 Giovanni Campagna 2012-12-17 19:48:45 UTC
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.
Comment 9 Giovanni Campagna 2012-12-17 19:54:14 UTC
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.
Comment 10 Giovanni Campagna 2012-12-17 19:56:20 UTC
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.
Comment 11 Jasper St. Pierre (not reading bugmail) 2012-12-17 21:58:47 UTC
Review of attachment 231770 [details] [review]:

Please don't tell me you wrote this patch.
Comment 12 Jasper St. Pierre (not reading bugmail) 2012-12-17 22:00:32 UTC
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.
Comment 13 Jasper St. Pierre (not reading bugmail) 2012-12-17 22:00:59 UTC
Review of attachment 231766 [details] [review]:

Fine by me.
Comment 14 Jasper St. Pierre (not reading bugmail) 2012-12-17 22:28:24 UTC
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.
Comment 15 Jasper St. Pierre (not reading bugmail) 2012-12-17 22:30:36 UTC
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.
Comment 16 Jasper St. Pierre (not reading bugmail) 2012-12-17 22:31:28 UTC
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.
Comment 17 Jasper St. Pierre (not reading bugmail) 2012-12-17 22:31:52 UTC
Review of attachment 231772 [details] [review]:

OK.
Comment 18 Jasper St. Pierre (not reading bugmail) 2012-12-17 22:33:17 UTC
Review of attachment 231773 [details] [review]:

OK.
Comment 19 Jasper St. Pierre (not reading bugmail) 2012-12-17 22:34:27 UTC
Review of attachment 231774 [details] [review]:

Wouldn't it be possible to go to a ClutterDesaturationEffect now? :q:
Comment 20 Giovanni Campagna 2012-12-17 23:12:05 UTC
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.
Comment 21 Giovanni Campagna 2012-12-17 23:19:32 UTC
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.
Comment 22 Allan Day 2012-12-18 10:07:48 UTC
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.
Comment 23 Jasper St. Pierre (not reading bugmail) 2012-12-21 14:35:09 UTC
(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.
Comment 24 Giovanni Campagna 2012-12-21 17:10:30 UTC
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.
Comment 25 Jasper St. Pierre (not reading bugmail) 2013-01-23 04:45:58 UTC
Where do we stand on this bug? Am I supposed to be waiting for you, or are you supposed to be waiting for me?
Comment 26 Bastien Nocera 2013-01-23 09:44:38 UTC
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.
Comment 27 Giovanni Campagna 2013-01-23 15:09:08 UTC
(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)
Comment 28 Jasper St. Pierre (not reading bugmail) 2013-03-03 21:24:25 UTC
All of those bugs are closed now. Can we get something in?
Comment 29 Giovanni Campagna 2013-03-04 08:04:22 UTC
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.
Comment 30 Jakub Steiner 2013-03-19 17:08:30 UTC
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
Comment 31 Bastien Nocera 2013-03-19 18:15:09 UTC
(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?
Comment 32 Matthias Clasen 2013-06-10 02:31:18 UTC
The gcc bug is bug 696166
Comment 33 Giovanni Campagna 2013-08-19 22:52:51 UTC
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.
Comment 34 Giovanni Campagna 2013-08-19 22:53:11 UTC
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.
Comment 35 Giovanni Campagna 2013-08-19 22:53:53 UTC
Yay, the control center side is fixed, and the gnome-shell fix is really a two liner!
Comment 36 Bastien Nocera 2013-08-19 23:37:22 UTC
(In reply to comment #35)
> Yay, the control center side is fixed, and the gnome-shell fix is really a two
> liner!

Yay indeed :)
Comment 37 Ray Strode [halfline] 2013-08-20 12:06:27 UTC
Review of attachment 252313 [details] [review]:

looks fine. really glad to see this land
Comment 38 Ray Strode [halfline] 2013-08-20 12:06:51 UTC
Review of attachment 252314 [details] [review]:

+
Comment 39 Giovanni Campagna 2013-08-20 12:12:56 UTC
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