Review of attachment 144547 [details] [review]:

Your commit message should be in the form:

 Subject
 <blank line>
 Body

> This will give us a central notification so we know when to loop through windows to find one demanding attention

Better to make the signal take the window as an argument.

::: src/core/display.c
@@ +130,3 @@
   OVERLAY_KEY,
   FOCUS_WINDOW,
+  DEMANDS_ATTENTION_WINDOW,

I guess this is in analogy to focus-window, but it still doesn't really read right for a signal name (focus-window is a bit dubious itself)

I think I'd call it 'window-demands-attention'

::: src/core/window.c
@@ +2689,3 @@
         g_object_notify (G_OBJECT (window), "demands-attention");
         g_object_thaw_notify(G_OBJECT (window));
+        meta_display_window_demands_attention(window->display);

I think once you have these three different things that you are doing, then this should be encapsulated into a function. I'd add a static mark_demands_attention() or something like that.

Created attachment 144733 [details] [review]
Updated patch with suggested changes.

Review of attachment 144733 [details] [review]:

Closer. Commit message still needs fixing to have separate subject line and body.

::: src/core/display.c
@@ +234,3 @@
+                  0,
+                  NULL, NULL,
+                  g_cclosure_marshal_VOID__VOID,

this needs to be VOID_OBJECT since you have single object-valued parameter

::: src/core/window.c
@@ +3528,3 @@
+{
+  g_object_notify (G_OBJECT (window), "demands-attention");
+  meta_display_window_demands_attention(window->display, window);

Missing space after function name

@@ +8821,3 @@
   set_net_wm_state (window);
 
+  signal_demands_attention (window); 

but you don't want to emit window-demands-attention if you are unsetting the demands attention hint, right?

If you make it mark_demands_attention(), then you can do:

 if (demands_attention)
   meta_display_window_demands_attention (...)

Created attachment 144794 [details] [review]
Hopefully the final patch

Comment on attachment 144794 [details] [review]
Hopefully the final patch

I was suggesting that mark_demands_attention() would take a boolean parameter and actually take care of setting window->wm_state_demands_attention. With this latest patch, you seem to have lost notification on ::demands-attention when unsetting the property.

I think I wasn't clear about subject vs. body in the commit message. Git commit are stored as freeform text, but they aren't really freeform. They have a structure they need to follow or the tools don't work right.

The structure is:

 A subject line
 < blank line>
 Explanatory body

It would be good to get such a signal in, but I just noticed the patch is not against the gnome repo, but against Jon's github repo, as we do not have a boolean MetaWindow::demands-attention property. I uploaded an updated patch for this to bug 588065.

Created attachment 146439 [details] [review]
Revised patch

I am attaching a reworked patch that applies on the top of the one for MetaWindow::demands-attention property that I attached to bug 588065; I have done away with the static function (it seems to just obscure the code; there are three occasions when the signal needs to be emitted, and they all slightly different); I also opted to emit the signal by name instead of adding and api point for this to MetaDisplay. See what you think :)

Review of attachment 146439 [details] [review]:

Looks good

Comment on attachment 146439 [details] [review]
Revised patch

Committed as 7579b691dfb7ce4f9e86325210de00dbdbd295eb.