Created attachment 139845 [details] [review]
Patch to expose window properties via gobject

Thanks John. I agree with making these into GObject properties, some notes on the patch:

* All the properties that we expose should come with notifications,

* Notification on umaximize seems missing,

* The notifications should be generated when we are done with processing a particular change. For example, in reload_net_wm_state() the g_object_notify() should come after the meta_window_recalc_window_type() call which should be preceded with g_object_freeze() notify:

   g_object_freeze_notify (window);
   meta_window_recalc_window_type (window);
   if (demands_attention_changed)
     g_object_notify (window, "demands-attention");
   g_object_thaw_notify (window);

This avoids folk being notified of change to the object while it is in an inconsistent state.

okay thanks.  I think that will fix the inconsistencies I was seeing.  I will rework the patch against the changes just pushed to mutter HEAD and re-submit it.

Ping on this patch; Jon, you think you'll have a chance to update?

Created attachment 144546 [details] [review]
rebased and updated patch

Sorry about the delay this should fix most the outstanding issues.

Comment on attachment 144546 [details] [review]
rebased and updated patch


>+void
>+meta_window_updated_wm_property (MetaWindow *window,
>+                                 const gchar *property_name,
>+                                 gboolean requires_recalc)
>+{
>+  if (property_name != NULL)
>+    g_object_freeze_notify(G_OBJECT (window));
>+
>+  if (requires_recalc)
>+    recalc_window_features (window);
>+
>+  set_net_wm_state (window);

So we always call set_net_wm_state here, but:

>@@ -2789,6 +2962,10 @@ meta_window_minimize (MetaWindow  *window)
>                       "Minimizing window %s which doesn't have the focus\n",
>                       window->desc);
>         }
>+    
>+      meta_window_updated_wm_property (window,
>+                                       "minimized",
>+                                       FALSE);
>     }
> }

This case did not synchronously set the state before; minimize (for some reason I don't know, perhaps avoiding recursion) actually queues an idle to process the minimize later.  So I'm not sure it's entirely safe to unconditionally call set_net_wm_state here.  

I'd just change this to a simple g_object_notify (window, "minimized");


> 
>@@ -2806,6 +2983,10 @@ meta_window_unminimize (MetaWindow  *window)
>       meta_window_foreach_transient (window,
>                                      queue_calc_showing_func,
>                                      NULL);
>+      
>+      meta_window_updated_wm_property (window,
>+                                       "minimized",
>+                                       FALSE);
>     }

Ditto here.

> /**
>+The following section are functions for getting properties
>+of a MetaWindow.  I have moved them here and defined them
>+in window-private.h so they will not be included in 
>+gobject-introspection. 

Well, if we have public accessors already, I don't see the harm in making them private now for gobject-introspection.  Moving them to -private may also break other C consumers who were using the API before.

It would be nice actually to have an introspection annotation like (getter minimized) so that a binding could make the choice how to handle C getters versus property getters.

Created attachment 144628 [details] [review]
Minimize changes reverted.

What would we like to do for the accessors?

Didn't really look through the patch in detail (assume that Colin did that), but onething I saw when I was looking at the other patch was:

+        g_object_freeze_notify(G_OBJECT (window));
         window->wm_state_demands_attention = TRUE;
+        g_object_notify (G_OBJECT (window), "demands-attention");
+        g_object_thaw_notify(G_OBJECT (window));

There's no point in doing a freeze/thaw when you are only notifying one property.

(In reply to comment #8)
> What would we like to do for the accessors?

Don't touch the accessors, just leave them.

Created attachment 144725 [details] [review]
Newest Version

Review of attachment 144725 [details] [review]:

Generally looks pretty good. I checked the notifications thoroughly, and you are missing some, not below. Couple of the properties should not be exported, also noted. And I made some suggestions about improving property descriptions. Portion of the patch that moves getters needs to be omitted. COmmit message needs to be subject\n\nbody.

::: src/core/window-props.c
@@ +648,3 @@
       else if (value->v.atom_list.atoms[i] == window->display->atom__NET_WM_STATE_DEMANDS_ATTENTION)
+        {
+          if (!window->wm_state_demands_attention)

Code above has:

  window->wm_state_demands_attention = FALSE;

so this won't work as expected. On the other hand, apps aren't allowed to change _NET_WM_STATE unless the window is withdrawn, so you don't need notification here at all.

::: src/core/window.c
@@ +141,3 @@
+  PROP_MAXIMIZED,
+  PROP_MAXIMIZED_HORIZONTALLY,
+  PROP_MAXIMIZED_VERTICALLY,

You don't have notification for these two properties. I don't think these need to be present, but if they are exported, they need to be notified (note that notification doesn't have to be precise - it's OK to notify a property when it isn't actually changing.)

@@ +145,3 @@
   PROP_WINDOW_TYPE,
+  PROP_DEMANDS_ATTENTION,
+  PROP_HAS_FOCUS,

You are missing notification for "has-focus". Yes, screen has a focus-window signal but if a property is present it needs to be individually notified as well.

@@ +146,3 @@
+  PROP_DEMANDS_ATTENTION,
+  PROP_HAS_FOCUS,
+  PROP_ROLE,

Missing notification for 'role'

@@ +147,3 @@
+  PROP_HAS_FOCUS,
+  PROP_ROLE,
+  PROP_WM_CLASS,

Missing notification for wm-class - changing your WM_CLASS on the fly strange is dubiously valid, but Metacity supports it, so should notify.

@@ +148,3 @@
+  PROP_ROLE,
+  PROP_WM_CLASS,
+  PROP_IS_ON_ALL_WORKSPACES,

Missing notification for on-all-workspaces. (window_stick_impl(), window_unstick_impl)

@@ +152,3 @@
+  PROP_DESCRIPTION,
+  PROP_USER_TIME,
+  PROP_BORDER_ONLY

Missing notification for border-only

@@ +382,3 @@
+                                   g_param_spec_boolean ("demands-attention",
+                                                         "Demands Attention",
+                                                         "Whether window demands attention",

In a lot of these, there isn't a lot more to say, but if the description was *always* a restatement of the nick, then there would be no point in it. It's generally good to try and expand just a bit. Here maybe "Whether the _NET_WM_DEMANDS_ATTENTION state is set for the window"

@@ +390,3 @@
+                                   g_param_spec_boolean ("has-focus",
+                                                         "Has Focus",
+                                                         "Whether window has focus",

"has keyboard focus", perhaps.

@@ +397,3 @@
+                                   g_param_spec_string ("role",
+                                                         "Role",
+                                                         "The role of the window",

"The WM_WINDOW_ROLE the application specified for the window"

@@ +403,3 @@
+                                   PROP_WM_CLASS,
+                                   g_param_spec_string ("wm-class",
+                                                         "WindowManager Class",

The name should be only a reformatting of the ID, so WM Class

@@ +404,3 @@
+                                   g_param_spec_string ("wm-class",
+                                                         "WindowManager Class",
+                                                         "The class of the window",

I think I'd say,"Resource class of window from WM_CLASS property". class is a pretty generic word.

@@ +410,3 @@
+                                   PROP_IS_ON_ALL_WORKSPACES,
+                                   g_param_spec_boolean ("is-on-all-workspaces",
+                                                         "Is On All Workspaces",

Property should be "on-all-workspaces" not "is-on-all-workspaces". meta_window_is_on_all_workspaces() is like meta_window_get_on_all_workspaces() - the 'is' is just a helper word indicating a getter. [This doesn't apply to 'has-focus', because a 'focus' property wouldn't sound like a boolean. And 'has' is a bit less of 'getter' word than 'is'.]

@@ +418,3 @@
+                                   g_param_spec_boolean ("hidden",
+                                                         "Hidden",
+                                                         "Whether window is hidden",

Don't expose this, meta_window_is_hidden() is exporting an implementation detail of "live hidden windows" - "is stacked at the bottom of the stack because it should be unmapped but can't be unmapped"

@@ +423,3 @@
+  g_object_class_install_property (object_class,
+                                   PROP_DESCRIPTION,
+                                   g_param_spec_string ("description",

Don't expose this - the description is a debugging facility... it's a description of a window that's useful in debug printf output.

@@ +441,3 @@
+                                   g_param_spec_boolean ("border-only",
+                                                         "Border only",
+                                                         "Whether window on has a border",

'only', not 'on' - maybe "Window only has a border, not full decorations" - not sure this one is worth exporting.. I haven't seen an app use this in years, and I don't think GTK+ has any way of setting the property.

@@ +8973,3 @@
+The following section are functions for getting properties
+of a MetaWindow.  I have moved them here and defined them
+in window-private.h so they will not be included in

Please delete this section of the patch - while it might be cleaner to group these functions together, it's not worth noise in the patch and in the VCS history; plus you've deleted doc comments.

Created attachment 153670 [details] [review]
[MetaWindow] Expose maximized state as properties

Patch exposing maximized_horizontally and maximized_vertically - sorry for
the conflict, but doing a separate patch seemed like a much quicker option
than cleaning up the existing one.

Review of attachment 153670 [details] [review]:

Looks good to commit

Comment on attachment 153670 [details] [review]
[MetaWindow] Expose maximized state as properties

Attachment 153670 [details] pushed as d21da56 - [MetaWindow] Expose maximized state as properties