Created attachment 196117 [details] [review]
screen: New layout

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.

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?

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.

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.

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)

Created attachment 216076 [details]
how it looks

Note that the patch depends on a new gsettings key, org.gnome.desktop.screensaver.show-notifications.

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.

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.

Oh, you're right about the other observation, it's in a separate patch I forgot to upload.

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)

I had the second patch when I tested, but didn't get any updates, still.

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.

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?

(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

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.

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.

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)

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.

Review of attachment 216466 [details] [review]:

This one looks fine to me

Review of attachment 216472 [details] [review]:

All of Bastien's comments have been addressed

Jon & Jimmac - any comments?

Review of attachment 216472 [details] [review]:

This needs more design work unfortunately. Code is good, but this isn't ready to land.

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

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 ?

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.

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.

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.

Created attachment 219007 [details] [review]
another attempt

Created attachment 219008 [details]
how it looks

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

Really? It still has all the problems it had...

(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 on attachment 219007 [details] [review]
another attempt

Committed with a commit message change, and the label change Allan mentioned in comment 31.

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

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

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

The screen panel will shortly be gone, replaced by the Privacy and Power panels.