After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 653858 - add `above` and `resizeable` properties to MetaWindow
add `above` and `resizeable` properties to MetaWindow
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2011-07-02 14:02 UTC by Tim Cuthbertson
Modified: 2011-08-09 11:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
exposes these two properties (2.44 KB, patch)
2011-07-02 14:05 UTC, Tim Cuthbertson
none Details | Review
exposes these two properties (2) (3.25 KB, patch)
2011-07-03 01:54 UTC, Tim Cuthbertson
needs-work Details | Review
exposes these two properties (3) (4.92 KB, patch)
2011-07-10 00:53 UTC, Tim Cuthbertson
needs-work Details | Review
exposes these two properties (4) (4.94 KB, patch)
2011-07-16 08:29 UTC, Tim Cuthbertson
needs-work Details | Review
exposes these two properties (5) (4.95 KB, patch)
2011-07-29 10:57 UTC, Tim Cuthbertson
committed Details | Review

Description Tim Cuthbertson 2011-07-02 14:02:44 UTC
For the purpose of extensions that need to know whether the given window is
 - resizeable, and/or
 - always on top ("above")

Shellshape in particular (to help decide if the window can or should be tiled), but I presume there could be others.
Comment 1 Tim Cuthbertson 2011-07-02 14:05:34 UTC
Created attachment 191149 [details] [review]
exposes these two properties
Comment 2 Rui Matos 2011-07-02 20:29:07 UTC
(In reply to comment #1)
> Created an attachment (id=191149) [details] [review]
> exposes these two properties

You'd also need to sprinkle some g_object_notify() calls for this to be useful, no?
Comment 3 Tim Cuthbertson 2011-07-03 01:54:19 UTC
Created attachment 191172 [details] [review]
exposes these two properties (2)

Good point, I haven't really added properties before so I forgot this. Here's a patch that includes the notification calls as well.
Comment 4 Owen Taylor 2011-07-08 18:24:42 UTC
Review of attachment 191172 [details] [review]:

Commit message needs improvement; mutter is a library (and executable) that doesn't have extensions. And "extensions might want this" isn't really a motivation - motivation is what you might be able to do with it. Other than that, one problem with the notifications:

::: src/core/window.c
@@ +3750,3 @@
   meta_window_raise (window);
   set_net_wm_state (window);
+  g_object_notify (G_OBJECT (window), "above");

There are more places where wm_state_above is changed - see meta_window_client_message() - note that you can't just make that call make_above/unmake_above because we don't want the raise() calls there - especially for unmake_above(). (I'm not sure why they are there at all - maybe we just want all keybindings and window menu operations to raise the window?)
Comment 5 Tim Cuthbertson 2011-07-10 00:53:02 UTC
Created attachment 191606 [details] [review]
exposes these two properties (3)

Thanks, Owen. I've changed the commit message to hopefully make it clearer, but I'm not really sure how best to justify it in general - my specific case is so that my gnome-shell extension can determine whether is is useful to try and lay out the window or leave it floating (non-resizeable or always-on-top windows are both left alone when calculating a tiled window layout). I'm not seeing an obvious way to generalize that ;)

I've extracted a method meta_window_set_above to set the flag, which takes care of the notification whenever the flag changes, and used it in meta_window_make_above, meta_window_unmake_above and meta_window_client_message.
Comment 6 Owen Taylor 2011-07-13 14:27:34 UTC
Review of attachment 191606 [details] [review]:

Commit message looks better - it's informative enough. I might say
the same thing slightly different:

 MetaWindow: Add :resizable and :above properties

 Allow libmutter users to treat windows different based on these
 attributes and to watch for changes.

(Because actually, object properties are part of GObject since forever, and are not dependent on introspection to be useful.)

::: src/core/window.c
@@ +3752,3 @@
   meta_window_update_layer (window);
   meta_window_raise (window);
   set_net_wm_state (window);

You can move the update_layer() and set_net_wm_state() calls into set_above()

@@ +3767,3 @@
 
+static void
+meta_window_set_above (MetaWindow *window, gboolean new_value)

Though it's not 100% consistent, generally parameters in method definitions are broken into separate lines with aligned parameter names - see the rest of the file

@@ +3771,3 @@
+  gboolean old_value = window->wm_state_above;
+  window->wm_state_above = new_value;
+  if(old_value != new_value)

missing space after if;

old_value doesn't seem necessary here.

What we generally would do is something liek:

 new_value = new_value != FALSE;
 if (new_value == window->wm_state_above)
   return;

@@ +7770,3 @@
     set_allowed_actions_hint (window);
 
+  if(window->has_resize_func != old_has_resize_func)

Missing space after if
Comment 7 Tim Cuthbertson 2011-07-16 08:29:39 UTC
Created attachment 192078 [details] [review]
exposes these two properties (4)

Changed commit message & minor edits as suggested.
Comment 8 drago01 2011-07-16 11:09:44 UTC
Review of attachment 192078 [details] [review]:

::: src/core/window.c
@@ +3770,3 @@
+    return;
+  window->wm_state_above = new_value;
+  g_object_notify (G_OBJECT (window), "above");

Should use g_object_freeze/thaw_notify around that. (And other uses of g_object_notify)
Comment 9 Tim Cuthbertson 2011-07-17 02:01:01 UTC
Review of attachment 192078 [details] [review]:

::: src/core/window.c
@@ +3770,3 @@
+    return;
+  window->wm_state_above = new_value;
+{

I'm not familiar with freeze/thaw_notify. The docs say it freezes the notification queue until thaw is called, which sounds like it's useful when you change more than one property simultaneously (from a quick look, that seems to be the situation where freeze/thaw is used elsewhere in the file). Is it a multithreading issue? The actual property here is changed by the time the notification is sent.
Comment 10 Rui Matos 2011-07-20 21:57:05 UTC
Review of attachment 192078 [details] [review]:

::: src/core/window.c
@@ +72,3 @@
 static void     set_net_wm_state          (MetaWindow     *window);
+static void     meta_window_set_above     (MetaWindow     *window,
+                                           gboolean new_value);

Nit: arguments should be aligned.

@@ +3749,3 @@
   g_return_if_fail (!window->override_redirect);
 
+  meta_window_set_above(window, TRUE);

Nit: here and on other calls to this function you're missing a space before (.

@@ +3764,3 @@
+static void
+meta_window_set_above (MetaWindow *window,
+                       gboolean new_value)

Same alignment nit.

@@ +3770,3 @@
+    return;
+  window->wm_state_above = new_value;
+  g_object_notify (G_OBJECT (window), "above");

I don't think a freeze/thaw is needed here since only one property is changing.
Comment 11 Tim Cuthbertson 2011-07-29 10:57:17 UTC
Created attachment 192865 [details] [review]
exposes these two properties (5)

as before, with nits fixed (formatting)
Comment 12 Owen Taylor 2011-08-09 08:56:49 UTC
Review of attachment 192865 [details] [review]:

One tiny problem - I'll just fix this when pushing it. (Assuming that you don't have a gnome git account, let me know for future reference if you do)

::: src/core/window.c
@@ +3770,3 @@
+    return;
+  window->wm_state_above = new_value;
+  g_object_notify (G_OBJECT (window), "above");

the g_object_notify call should come as late as possible (at the end of this function) so when the signal is received the state of the window is internally consistent.
Comment 13 Tim Cuthbertson 2011-08-09 11:45:07 UTC
> One tiny problem - I'll just fix this when pushing it. (Assuming that you don't
have a gnome git account, let me know for future reference if you do)

No worries. And nope, I don't have a gnome git account.