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 658660 - New Brightness and Lock layout
New Brightness and Lock layout
Status: RESOLVED WONTFIX
Product: gnome-control-center
Classification: Core
Component: [obsolete] Screen
unspecified
Other All
: Normal normal
: ---
Assigned To: Control-Center Maintainers
Control-Center Maintainers
Depends on:
Blocks: 677113 677399
 
 
Reported: 2011-09-09 14:55 UTC by Bastien Nocera
Modified: 2013-01-16 15:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screen: New layout (26.07 KB, patch)
2011-09-09 14:55 UTC, Bastien Nocera
needs-work Details | Review
Brightness & Lock: update for the new mockups (40.03 KB, patch)
2012-06-08 21:05 UTC, Giovanni Campagna
needs-work Details | Review
Brightness: fix for gnome-settings-daemon changes (2.67 KB, patch)
2012-06-08 21:06 UTC, Giovanni Campagna
needs-work Details | Review
how it looks (28.66 KB, image/png)
2012-06-10 22:52 UTC, Matthias Clasen
  Details
configure: remove mention of gsettings-desktop-schemas in each panel (1.90 KB, patch)
2012-06-14 20:59 UTC, Giovanni Campagna
committed Details | Review
Brightness & Lock: update for the new mockups (38.74 KB, patch)
2012-06-14 21:35 UTC, Giovanni Campagna
none Details | Review
Brightness & Lock: update for the new mockups (39.30 KB, patch)
2012-06-14 21:48 UTC, Giovanni Campagna
needs-work Details | Review
Brightness & Lock: add Show Notifications setting (4.29 KB, patch)
2012-07-10 14:18 UTC, Giovanni Campagna
needs-work Details | Review
screenshot (16.20 KB, image/png)
2012-07-10 15:32 UTC, Bastien Nocera
  Details
another attempt (3.57 KB, patch)
2012-07-17 11:15 UTC, Matthias Clasen
committed Details | Review
how it looks (23.73 KB, image/png)
2012-07-17 11:16 UTC, Matthias Clasen
  Details

Description Bastien Nocera 2011-09-09 14:55:21 UTC
.
Comment 1 Bastien Nocera 2011-09-09 14:55:24 UTC
Created attachment 196117 [details] [review]
screen: New layout
Comment 2 Bastien Nocera 2011-09-09 14:59:10 UTC
Needs a bit of work. See:
https://github.com/gnome-design-team/gnome-mockups/blob/master/system-settings/screen/screen-hylke.png

for the updated mockups.
Comment 3 Allan Day 2012-05-31 16:07:19 UTC
We'll need to add an option for screen rotation to this panel if we want to fix bug 677113.

Hylke - do you fancy updating your design?
Comment 4 Allan Day 2012-06-04 15:38:07 UTC
I've done an updated design here - https://github.com/gnome-design-team/gnome-mockups/raw/master/system-settings/screen/screen-aday.png

This adds a screen rotation option (for bug 677113) as well as an option to disable notifications in the new lock screen. It'd be cool to get a design review from someone.
Comment 5 Giovanni Campagna 2012-06-08 21:05:43 UTC
Created attachment 215994 [details] [review]
Brightness & Lock: update for the new mockups

Switches are now placed on the right of the section headers,
there is a new combobox for changing the rotation of the primary
monitor and we expose the show notifications when locked setting.
Also, this commit removes some GtkBox usage in favor of GtkGrid.
Comment 6 Giovanni Campagna 2012-06-08 21:06:07 UTC
Created attachment 215995 [details] [review]
Brightness: fix for gnome-settings-daemon changes

Apparently, g-s-d changed its DBus interface, and that made all
external changes to brightness invisible to the panel. Update
accordingly.

(A small fix I found while testing the rest of the panel)
Comment 7 Matthias Clasen 2012-06-10 22:52:04 UTC
Created attachment 216076 [details]
how it looks

Note that the patch depends on a new gsettings key, org.gnome.desktop.screensaver.show-notifications.
Comment 8 Matthias Clasen 2012-06-10 23:06:54 UTC
One observation: the current panel makes the 'lock delay' combobox insensitive until automatic lock is switched on, the new panel seems to have lost that.
Comment 9 Matthias Clasen 2012-06-10 23:09:04 UTC
Another observation: with both the current panel and the new panel, using Fn keys to change brightness does not update the slider in an already open panel, so we end up with an inconsistent situation of dimmed panel, but slider at 100%. Restarting the panel makes the slider jump down.
Comment 10 Giovanni Campagna 2012-06-11 14:12:15 UTC
Oh, you're right about the other observation, it's in a separate patch I forgot to upload.
Comment 11 Giovanni Campagna 2012-06-11 14:13:41 UTC
No, actually it is attached, it's the "Brightness: fix for gnome-settings-daemon changes" one.
(I noticed some lag in the update, but couldn't track down the cause)
Comment 12 Matthias Clasen 2012-06-11 14:31:58 UTC
I had the second patch when I tested, but didn't get any updates, still.
Comment 13 Bastien Nocera 2012-06-12 12:51:43 UTC
Review of attachment 215994 [details] [review]:

::: configure.ac
@@ +88,3 @@
  gio-unix-2.0
+ gsettings-desktop-schemas >= $SCHEMAS_REQUIRED_VERSION
+ gnome-desktop-3.0 >= $GNOME_DESKTOP_REQUIRED_VERSION

Why add gnome-desktop-3.0 as a requirement to all the panels?

@@ +121,3 @@
                   libcanberra-gtk3 >= $CANBERRA_REQUIRED_VERSION
                   libpulse >= $PA_REQUIRED_VERSION
+                  libpulse-mainloop-glib >= $PA_REQUIRED_VERSION)

Removing the gsettings-desktop-schemas requirement is fine, but it should be done separately.

::: panels/screen/cc-screen-panel.c
@@ +493,3 @@
+}
+
+

Just need the one linefeed.

@@ +506,3 @@
+  priv = self->priv;
+
+  g_clear_object (&priv->rr_config);

I'd rather you didn't use RRConfig at all, and used GnomeRRCrtc instead, gathered from gnome_rr_screen_list_crtcs().
Use RRConfig in push_new_rotation() only.

@@ +550,3 @@
+
+  /* prevent reentrancy */
+  if (priv->setting_rotation)

How about checking for the status of the D-Bus call instead? Why not cancel the previous call, or apply the new setting if it's different?

@@ +553,3 @@
+    return;
+
+  primary = find_primary_output (priv->rr_config);

What happens if the display is cloned?

@@ +557,3 @@
+    return;
+
+  rotation = rotations[gtk_combo_box_get_active (widget)];

Dangerous, don't do that please.
Comment 14 Bastien Nocera 2012-06-12 13:00:18 UTC
Review of attachment 215995 [details] [review]:

There's a separate bug about that:
https://bugzilla.gnome.org/show_bug.cgi?id=670043

Could you move it there please?
Comment 15 Bastien Nocera 2012-06-12 13:05:03 UTC
(In reply to comment #14)
> Review of attachment 215995 [details] [review]:
> 
> There's a separate bug about that:
> https://bugzilla.gnome.org/show_bug.cgi?id=670043
> 
> Could you move it there please?

Make that bug 662117
Comment 16 Matthias Clasen 2012-06-12 13:06:05 UTC
One thing I noticed is that the rotation combo has the same keep-or-revert dialog we have in the display panel - there has been some discussion about avoiding that in most cases for the display panel. When we make changes there, we should do them here as well.
Comment 17 Giovanni Campagna 2012-06-14 20:59:18 UTC
Created attachment 216466 [details] [review]
configure: remove mention of gsettings-desktop-schemas in each panel

It is already in the common modules, no need to repeat it.
Comment 18 Giovanni Campagna 2012-06-14 21:35:34 UTC
Created attachment 216471 [details] [review]
Brightness & Lock: update for the new mockups

Switches are now placed on the right of the section headers,
there is a new combobox for changing the rotation of the primary
monitor and we expose the show notifications when locked setting.
Also, this commit removes some GtkBox usage in favor of GtkGrid.

Comments:
1) setting_rotation is there to prevent reentrancy when handling
   the XRandR signal (so we don't try to push the new rotation again),
   cancelling the DBus call is wrong there, and settings are already
   applied at that point
2) if the display is cloned, the panel recognizes it now and makes the
   rotation combobox insensitive (it doesn't make sense to change the
   rotation of many monitors at once)
Comment 19 Giovanni Campagna 2012-06-14 21:48:40 UTC
Created attachment 216472 [details] [review]
Brightness & Lock: update for the new mockups

Switches are now placed on the right of the section headers,
there is a new combobox for changing the rotation of the primary
monitor and we expose the show notifications when locked setting.
Also, this commit removes some GtkBox usage in favor of GtkGrid.

Updated to also bind the sensitivity of the lock combobox to the
lock-enabled setting.
Comment 20 Matthias Clasen 2012-06-20 01:10:44 UTC
Review of attachment 216466 [details] [review]:

This one looks fine to me
Comment 21 Matthias Clasen 2012-06-20 01:11:39 UTC
Review of attachment 216472 [details] [review]:

All of Bastien's comments have been addressed
Comment 22 Allan Day 2012-06-20 08:27:55 UTC
Jon & Jimmac - any comments?
Comment 23 Bastien Nocera 2012-06-21 18:31:27 UTC
Review of attachment 216472 [details] [review]:

This needs more design work unfortunately. Code is good, but this isn't ready to land.
Comment 24 Allan Day 2012-06-22 11:43:33 UTC
I got some feedback from Jon on IRC yesterday:

 * Horizontal separators aren't an established pattern in System Settings
 * Screen is monitor-specific and maybe not common enough to have in the screen panel
 * The indentation of the 'Automatic Lock Delay' option makes it hard to parse
 * There seemed to be general dissatisfaction with having rotate here. I'm not sure whether that reflects a dislike of the proposal in bug 677113 - I'd like more feedback there.

I've changed my mockup based on this feedback - https://github.com/gnome-design-team/gnome-mockups/raw/master/system-settings/screen/screen-aday.png

Jon - have you got any more feedback? (Giovanni - I'd wait until doing any more work on this.)
Comment 25 Matthias Clasen 2012-07-06 14:30:42 UTC
So, various issues were brought up with the redesign - can we just add the notification switch for now, so that this doesn't block the lock screen work ?
Comment 26 Giovanni Campagna 2012-07-10 14:18:29 UTC
Created attachment 218420 [details] [review]
Brightness & Lock: add Show Notifications setting

Add a switch to control the visibility of notifications when
the screen is locked.
Comment 27 Giovanni Campagna 2012-07-10 14:19:24 UTC
Ok, this was the easy patch, just adding the switch like originally planned.
The panel redesign, if needed, can then be done at a later time.
Comment 28 Bastien Nocera 2012-07-10 15:32:51 UTC
Created attachment 218433 [details]
screenshot

The label is lacking the colon the other labels have, and it looks very bizarre that they don't line up.
Comment 29 Matthias Clasen 2012-07-17 11:15:03 UTC
Created attachment 219007 [details] [review]
another attempt
Comment 30 Matthias Clasen 2012-07-17 11:16:36 UTC
Created attachment 219008 [details]
how it looks
Comment 31 Allan Day 2012-07-17 14:45:32 UTC
(In reply to comment #30)
> Created an attachment (id=219008) [details]
> how it looks

Looks fine for now. I'd suggest changing the label to 'Show notifications when locked' if it's going to be paired with a checkbox.
Comment 32 Hylke Bons 2012-07-17 14:46:46 UTC
Really? It still has all the problems it had...
Comment 33 Bastien Nocera 2012-07-17 14:50:49 UTC
(In reply to comment #32)
> Really? It still has all the problems it had...

That would be because this is just the "Show Notifications" setting, not the complete redesign. But we already said that in comment 24 and comment 25. Scroll back a wee bit when commenting next time.
Comment 34 Bastien Nocera 2012-07-17 14:52:07 UTC
Comment on attachment 219007 [details] [review]
another attempt

Committed with a commit message change, and the label change Allan mentioned in comment 31.
Comment 35 Hylke Bons 2012-07-17 14:53:28 UTC
(In reply to comment #33)
> (In reply to comment #32)
> > Really? It still has all the problems it had...
> 
> That would be because this is just the "Show Notifications" setting, not the
> complete redesign. But we already said that in comment 24 and comment 25.
> Scroll back a wee bit when commenting next time.

Update the bug title next time?
Comment 36 Allan Day 2012-07-17 14:54:39 UTC
(In reply to comment #32)
> Really? It still has all the problems it had...

We need this option to land the lock screen work, and it doesn't degrade the existing design. Hylke - if you have any more ideas for taking this panel forward, I'd love to see them. It's got me stumped...
Comment 37 Bastien Nocera 2012-07-17 15:04:15 UTC
(In reply to comment #35)
> (In reply to comment #33)
> > (In reply to comment #32)
> > > Really? It still has all the problems it had...
> > 
> > That would be because this is just the "Show Notifications" setting, not the
> > complete redesign. But we already said that in comment 24 and comment 25.
> > Scroll back a wee bit when commenting next time.
> 
> Update the bug title next time?

Nope, because we want to use this bug to discuss the redesign.
Comment 38 Bastien Nocera 2013-01-16 15:00:06 UTC
The screen panel will shortly be gone, replaced by the Privacy and Power panels.