GNOME Bugzilla – Bug 653858
add `above` and `resizeable` properties to MetaWindow
Last modified: 2011-08-09 11:45:07 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.
Created attachment 191149 [details] [review] exposes these two properties
(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?
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.
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?)
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.
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
Created attachment 192078 [details] [review] exposes these two properties (4) Changed commit message & minor edits as suggested.
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)
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.
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.
Created attachment 192865 [details] [review] exposes these two properties (5) as before, with nits fixed (formatting)
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.
> 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.