Review of attachment 345173 [details] [review]:

couldn't fully review this yet because I don't have the g-s-d changes required but there's quite a few things I've noticed already so posting this for now

::: panels/display/cc-display-panel.c
@@ +89,3 @@
   GtkWidget *config_grid;
+  GtkWidget *natural_light_filter_label;
+  GtkWidget *natural_light_dialog;

CcNaturalLightDialog doesn't inherit from GtkWidget

fwiw, I think it's fine that you use GObject as its parent type since it's just used to encapsulate code and the real GtkWidget (the GtkWindow instance created by the builder) is never visible here

@@ +91,3 @@
+  GtkWidget *natural_light_dialog;
+
+  GtkBuilder *builder;

this variable isn't used, see below

@@ +2834,3 @@
     }
 
+  priv->builder = gtk_builder_new ();

there's no use of the GtkBuilder here in this file, please remove this

::: panels/display/cc-natural-light-dialog.c
@@ +34,3 @@
+  GDBusProxy          *proxy_color;
+  GDBusProxy          *proxy_color_props;
+  GCancellable        *cancellable;

this cancellable is never cancelled and unref'ed. should be done in _finalize()

@@ +63,3 @@
+
+static void
+cc_natural_light_widget_finalize (GObject *object)

typo? you mean cc_natural_light_*dialog*_finalize

@@ +73,3 @@
+  g_object_unref (priv->settings_clock);
+  if (priv->timer_id > 0)
+    g_source_remove (priv->timer_id);

missing the chaining up to the parent class' finalize

@@ +193,3 @@
+  gtk_widget_set_sensitive (widget, enabled && !automatic);
+  widget = GTK_WIDGET (gtk_builder_get_object (priv->builder, "button_to_pm"));
+  gtk_widget_set_sensitive (widget, enabled && !automatic);

isn't there a common parent widget to all these that you could use to set sensitivity in a single call?

@@ +253,3 @@
+  /* refresh UI */
+  gtk_widget_hide (priv->natural_light_widget);
+  gtk_widget_show (priv->natural_light_widget);

not the common idiom to cause gtk+ widgets to redraw, see comment on the widget code

@@ +574,3 @@
+  if (error != NULL)
+    {
+      g_critical ("Could not load interface file: %s", error->message);

nitpick: could be a g_error() to abort since we'll crash below if this happens

::: panels/display/cc-natural-light-widget.c
@@ +33,3 @@
+
+G_DEFINE_TYPE_WITH_PRIVATE (CcNaturalLightWidget, cc_natural_light_widget, GTK_TYPE_DRAWING_AREA);
+#define GET_PRIVATE(o) (cc_natural_light_widget_get_instance_private (o))

nitpick: since this isn't going to be inherited from, you could G_DECLARE_FINAL_TYPE in the header and then just G_DEFINE_TYPE here and you wouldn't need the GET_PRIVATE stuff and instead just stash your variables right into the object instance struct

@@ +55,3 @@
+{
+  CcNaturalLightWidgetPrivate *priv = GET_PRIVATE (nlwidget);
+  priv->now = now;

these three setter methods should call gtk_widget_queue_draw() to request a draw call from gtk+

@@ +133,3 @@
+{
+  cairo_surface_t *surface;
+  surface = cairo_image_surface_create_from_png (fn);

these surfaces should be loaded once and cached instead of reloading them on every draw call

Created attachment 345301 [details] [review]
patch for review v2

Okay, all issues fixed up, and I've changed the icons to be cached and loaded from a GResource rather than from a file. Thanks for the speedy review. I've not switched from DERIVABLE to a FINAL object type, I don't know if we'll want signals from the dialog to the manager in the future, and I don't think it's possible to add a signal vfunc to a FINAL object. Happy to be corrected :)

Created attachment 345350 [details] [review]
patch for review v3

Okay, made FINAL and fixed up a few other issues. Comments welcome. If you install gnome-settings-daemon from git master you can actually test this.

Review of attachment 345301 [details] [review]:

::: panels/display/Makefile.am
@@ +54,3 @@
 
+CLEANFILES = $(Desktop_in_files) $(desktop_DATA) $(BUILT_SOURCES)
+EXTRA_DIST = $(resource_files) display.gresource.xml icons/16x16/sunrise.png icons/16x16/sunset.png

aren't the icon files already included in $(resource_files) ?

::: panels/display/cc-natural-light-dialog.c
@@ +53,3 @@
+  CcNaturalLightDialogPrivate *priv = GET_PRIVATE (nldialog);
+  GtkWindow *window;
+  window = GTK_WINDOW (gtk_builder_get_object (priv->builder, "window_natural_light"));

GtkBuilder keeps a reference to all its widgets so by destroying the builder instance on finalize you get rid of that. But, for toplevels you need to destroy them explicitly because gtk+ keeps a reference on them. So, you should keep this pointer around and gtk_widget_destroy() it on finalize otherwise we're leaking the whole dialog.

::: panels/display/cc-natural-light-widget.c
@@ +107,3 @@
+  g_autoptr(GInputStream) stream = NULL;
+  stream = g_resource_open_stream (cc_display_get_resource (),
+                                   "/org/gnome/control-center/display/sunrise.png",

this should be the path variable

::: panels/display/display.ui
@@ +26,3 @@
+    <property name="page_increment">10</property>
+  </object>
+  <object class="GtkWindow" id="window_natural_light">

This should be switched to GtkDialog to be consistent with other dialogs for this panel. In particular, GtkDialog handles closing via Escape, has a Cancel button, etc.

Review of attachment 345350 [details] [review]:

the review on v2 still applies

Created attachment 345369 [details] [review]
patch for review v4

New patch, all review points addressed. Thanks!

Review of attachment 345369 [details] [review]:

code looks good, if the design got an ack go ahead and push

I'll just note that the main row that activates the dialog looks a bit odd when you have multiple monitors because of the "Arrange Combined Displays" button which stays between the monitor rows and this dialog's row.

(In reply to Rui Matos from comment #7)
> code looks good, if the design got an ack go ahead and push

Great, thanks for the speedy review.

> I'll just note that the main row that activates the dialog looks a bit odd
> when you have multiple monitors because of the "Arrange Combined Displays"
> button which stays between the monitor rows and this dialog's row.

Yup, this is suboptimal but a compromise put forward by Allan. When someone implements the new display panel design it'll look a lot less odd.