Created attachment 346370 [details] [review]
source-dialog: update calendar image color

After editing the color of a calendar its image is not
updated as well. Fix this by updating the image when the
edit is finished.

Review of attachment 346370 [details] [review]:

The code and the commit message are over the top, congratulations! I'd like to propose, however, another approach to the problem.

Instead of storing the edited row, I believe it'll be better if the patch connects to ESourceSelectable's 'notify::color', and update the row's icon when that happens. This is safer and if someone changes the color outside Calendar (e.g. Evolution), we'll still be able to detect that and update accordingly.

What do you think? Thanks so much for the patch, it's looking great. Let me know if I can help with something.

Created attachment 346981 [details] [review]
source-dialog: update calendar image color

After editing the color of a calendar its image is not
updated as well. Fix this by updating the image when the
edit is finished.

Review of attachment 346981 [details] [review]:

Almost there! Thanks again for your work on this issue.

::: src/gcal-source-dialog.c
@@ +565,3 @@
+source_color_changed (GObject    *source,
+                      GParamSpec *pspec,
+                      gpointer    user_data)

Since g_signal_connect() doesn't check for function signatures, instead of 'gpointer user_data', you can let this parameter be 'GtkImage *icon' directly :)

@@ +569,3 @@
+  ESourceSelectable *extension;
+  GdkRGBA out_color;
+  GdkPixbuf *pixbuf;

Nitpick: in Calendar, I try to sort the variables according to the type length. Here, it'd be like:

ESourceSelectable*;
GdkPixbuf*
GdkRGBA

@@ +570,3 @@
+  GdkRGBA out_color;
+  GdkPixbuf *pixbuf;
+  extension = E_SOURCE_SELECTABLE (source);

Nitpick: please jump a line before 'extension = ...'

@@ +572,3 @@
+  extension = E_SOURCE_SELECTABLE (source);
+
+  if (gdk_rgba_parse (&out_color, e_source_selectable_get_color (extension)))

I prefer an early return here. Something like:

if (!gdk_rgba_parse (...))
  {
     g_warning (...);
     return;
  }

@@ +574,3 @@
+  if (gdk_rgba_parse (&out_color, e_source_selectable_get_color (extension)))
+    {
+      e_source_selectable_set_color (extension, gdk_rgba_to_string (&out_color));

You don't have to call this. source_color_changed() is called when e_source_selectable_set_color() changes the color.

@@ +638,3 @@
+  extension = E_SOURCE_SELECTABLE (e_source_get_extension (source, E_SOURCE_EXTENSION_CALENDAR));
+  g_signal_connect (extension, "notify::color", G_CALLBACK (source_color_changed),
+                    icon);

I know that surrounding code is like that, but could you format this g_signal_connect(...) call to one line?

I have fixed all, but before update the path, I have a question. What do you mean by not callind source_color_change function here:  g_signal_connect (extension, "notify::color", G_CALLBACK (source_color_changed),
+                    icon);

(In reply to Valentin Negoescu from comment #5)
> I have fixed all, but before update the path, I have a question. What do you
> mean by not callind source_color_change function here:  g_signal_connect
> (extension, "notify::color", G_CALLBACK (source_color_changed),
> +                    icon);

The logic here is:

 1. Someone calls e_source_selectable_set_color()
 2. The property 'color' changes
 3. The signal 'notify::color' is emited
 4. We receive the signal and source_color_change() is called
 5. source_color_change() sets the 'color' property again
 6. We can end in an infinite signal/callback loop!

Make sense?

Created attachment 347279 [details] [review]
source-dialog: update calendar image color

After editing the color of a calendar its image is not
updated as well. Fix this by updating the image when the
edit is finished.

Review of attachment 347279 [details] [review]:

Looks very good to me. Thanks!

Thanks for the patch!

Attachment 347279 [details] pushed as 7aa4cef - source-dialog: update calendar image color

Thanks so much for your help. Is anything that I can help you with?

Hey, sorry for the delay.

There are plenty of bugs to be fixed! Feel free to pick any one of them, and work on it. I'm available on IRC if you need any help