GNOME Bugzilla – Bug 344521
_NET_MOVERESIZE_WINDOW support (EWMH)
Last modified: 2007-06-12 16:22:46 UTC
The XAtom is registered but the window manager doesn't respond to the message.
Created attachment 67102 [details] [review] Most obvious fix ;-) This is not so much a suggested fix as it is a cry for help :-) I'd like to try to address the issues in the FIXME: comments, but I fear I'll need some pointers to get started.
Basically, the second FIXME comment just means that the function should not be written from scratch; it should use as much of the same code that meta_window_configure_request() uses as possible. So, you'll just need to factor out the common code and have both meta_window_configure_request() and handle_net_moveresize_window() call it. I didn't write the first FIXME; you'll have to read over the spec to see if it's any clearer or bring it up on wm-spec-list@ if not.
Created attachment 67593 [details] [review] Stab in the dark :) I'm not sure about this one at all. This makes the two functions rather similar. The next step would be to factor things out. However, before I do that I'd like to know if I'm going in the right direction or not. Be ruthless!
Created attachment 67659 [details] [review] Next step
(In reply to comment #4) The fix seems to miss the connection between the atom and the function. I have to admit I'm at a total loss about how things fit together when it comes to atoms in metacity. Any and all help much appreciated.
Sorry for not getting to this during 2.15.x development. However, 2.17.x development has begun so let's get this in. Your patch looks pretty good overall. My review looks long, but I'm just verbose. ;-) The most important bits: - In order to get metacity to broadcast that it supports handle_net_moveresize_window, you need to modify metacity/src/screen.c:set_supported_hint(). I think this is what you were referring to in comment 5? - new_gravity can be 0 in meta_window_move_resize_request(); however 0 isn't a valid gravity for moving or resizing and you haven't manually set it to anything in that case. 0 should mean "use whatever gravity already exists", i.e. window->size_hints.win_gravity. - meta_window_get_gravity_position() assumes the gravity being used is window->size_hints.win_gravity. This is not true for the call from meta_window_move_resize_request(), so you'll need to modify meta_window_get_gravity_position() to accept a gravity flag. - In meta_window_move_resize_request() you set window->size_hints.win_gravity to the specified gravity. I assumed that was wrong at first, but it appears the spec did not actually specify whether that should be done. We need to ask on wm-spec-list (@g.o. instead of @f.o. for weird historical reasons) what should be done here. (Also note that this potentially affects the call to meta_window_move_resize_internal, as new_gravity is the correct thing to pass there) Random nitpicks: - Code like if(!window) should be if (!window) to match the style elsewhere in the code. - I'd prefer the name "value_mask" over "mode" (as you use in handle_net_move_resize_window) or "flags" (as you use in meta_window_move_resize_request). It's slightly more clear what the meaning is and is consistent with XConfigureRequestEvent, which this is designed to mimick. Making this change also prevents potential confusion in the difference between "flags" and "meta_flags" ;-) - I prefer argument lists like MetaWindow *window, guint value_mask, over MetaWindow *window, guint value_mask, Most of the code uses the former, and for consistency it'd be nice to keep the new code the same. - I'd prefer meta_window_configure_request to explicitly pass window->size_hints.win_gravity (rather than 0) for gravity, as it makes the code more self-documenting. Thanks for working on this! (Want to fix up net_restack_window similarly too once you're done with this?)
Created attachment 73717 [details] [review] Adressing parts of comment 6
(In reply to comment #6) Attached a new version of the patch. > - In order to get metacity to broadcast that it supports > handle_net_moveresize_window, you need to modify > metacity/src/screen.c:set_supported_hint(). I think this is what you were > referring to in comment 5? Not sure. I found that change in my local environment and now I can't remember whether I tested it or not :-) > - new_gravity can be 0 in meta_window_move_resize_request(); however 0 isn't > a valid gravity for moving or resizing and you haven't manually set it to > anything in that case. 0 should mean "use whatever gravity already > exists", i.e. window->size_hints.win_gravity. The patch does contain code that sets window->size_hints.win_gravity to new_gravity only if the latter is !=0. window->size_hints.win_gravity is always passed on to meta_window_move_resize_internal(). I could be missing something, but I think that has the effect you are talking about... > - meta_window_get_gravity_position() assumes the gravity being used is > window->size_hints.win_gravity. This is not true for the call from > meta_window_move_resize_request(), so you'll need to modify > meta_window_get_gravity_position() to accept a gravity flag. I'll upload a new patch shortly. > - In meta_window_move_resize_request() you set window->size_hints.win_gravity > to the specified gravity. I assumed that was wrong at first, but it > appears the spec did not actually specify whether that should be done. We > need to ask on wm-spec-list (@g.o. instead of @f.o. for weird historical > reasons) what should be done here. (Also note that this potentially > affects the call to meta_window_move_resize_internal, as new_gravity is the > correct thing to pass there) I'm not on the list. Would you mind posting to it, and CC me? > Random nitpicks: All done. I'm probably a bit of a pervert, but I actually like these sort of comments. It shows you care about the code... I miss getting that feeling at my day job! > Thanks for working on this! (Want to fix up net_restack_window similarly too > once you're done with this?) Hmm, don't know what you're talking about here, but WTH, why not?
Created attachment 73722 [details] [review] meta_window_get_gravity_position in comment 6 Attempting to also address the part on meta_window_get_gravity_position in comment 6.
Created attachment 74036 [details] Small test program that uses wnck_set_geometry
(In reply to comment #6) > - In order to get metacity to broadcast that it supports > handle_net_moveresize_window, you need to modify > metacity/src/screen.c:set_supported_hint(). I think this is what you were > referring to in comment 5? That doesn't seem to be enough. I wrote a small test program that works fine in OpenBox, but has no effect in my modified version of Metacity. It uses wnck_set_geometry() which translates to a _NET_MOVERESIZE_WINDOW "call". I made the change in src/screen.c: static int set_supported_hint (MetaScreen *screen) { #define N_SUPPORTED 60 Atom atoms[N_SUPPORTED]; ... atoms[59] = screen->display->atom_net_moveresize_window; ... } Searching through the source code a bit I found a few places that seemed to be related to dealing with this atom. I stuck a call to meta_warning() in a few places in src/display.c: static void handle_net_moveresize_window (MetaDisplay* display, XEvent *event) { ... meta_warning ("MT: %s\n", __FUNCTION__); window = meta_display_lookup_x_window (display, event->xclient.window); ... } static gboolean event_callback (XEvent *event, gpointer data) { ... screen = meta_display_screen_for_root (display, event->xproperty.window); meta_warning ("MT: %s, just before test\n", __FUNCTION__); if (screen != NULL) { if (event->xproperty.atom == display->atom_net_desktop_layout) meta_screen_update_workspace_layout (screen); else if (event->xproperty.atom == display->atom_net_desktop_names) meta_screen_update_workspace_names (screen); else if (event->xproperty.atom == display->atom_net_moveresize_window) { meta_warning ("MT: %s, calling handle_net_moveresize_window\n", __FUNCTION__); handle_net_moveresize_window (display, event); } ... } Out of these three meta_warning() only one is ever hit, the one just before the if(screen != NULL) statement. Any ideas on what might be going on here and how to fix it?
Comment on attachment 73722 [details] [review] meta_window_get_gravity_position in comment 6 >+#include "window.h" display.c already includes window.h, no need to do so again. :) >+ guint gravity, value_mask, source; elsewhere in the code, gravity always has type int. For consistency, it'd be nice to do the same here. Also, source should be of type MetaClientType. >+ else if (event->xproperty.atom == >+ display->atom_net_moveresize_window) >+ handle_net_moveresize_window (display, event); This is actually the problem with metacity never catching the "_NET_MOVERESIZE_WINDOW" events that you found. I'm not sure why I didn't notice this in earlier reviews. Anyway, this code is in the PropertyNotify handler, but these messages are ClientMessage events. So the initial code you were working off misled you. Note that the ClientMessage event handler just calls meta_window_client_message(). In meta_window_client_message() you can see how lots of similar messages, such as _NET_WM_MOVERESIZE or _NET_ACTIVE_WINDOW are handled. Basically, the handle_net_moveresize_window() code should be moved into meta_window_client_message(). >+ guint gravity, again, int please... >+ meta_window_get_gravity_position (window, >+ (guint)window->size_hints.win_gravity, that way you don't have to do casts like this (it is X that defined gravity to be an int rather than an unsigned int). :) There's a couple others; I'll let you find them. >- MetaMoveResizeFlags flags; >+ MetaMoveResizeFlags meta_flags; Why not just leave this as flags? >+ meta_window_get_gravity_position (window, >+ (new_gravity ? new_gravity : (guint)window->size_hints.win_gravity), >+ &x, &y); I don't like this. I'd rather that meta_window_move_resize_request() always be passed a valid gravity. Thus, the caller should check if it got a 0 and fix it up. >+ if (new_gravity) >+ window->size_hints.win_gravity = new_gravity; I looked at the KWin code; they don't do this, so let's not do it. :) > meta_window_move_resize_internal (window, >- flags, >+ meta_flags, > window->size_hints.win_gravity, This needs to be new_gravity instead of window->size_hints.win_gravity (because of the previous change). >+void meta_window_move_resize_request(MetaWindow *window, >+ guint flags, >+ guint new_gravity, >+ gint new_x, >+ gint new_y, >+ gint new_width, >+ gint new_height); Those variable names don't line up very well. ;) >-#define N_SUPPORTED 58 >+#define N_SUPPORTED 60 You're only adding one new atom, not two. > //atoms[58] = screen->display->atom_net_restack_window; >- //atoms[59] = screen->display->atom_net_moveresize_window; >- >+ atoms[59] = screen->display->atom_net_moveresize_window; >+ This leaves atoms[58] undefined and yet used, which is bad. I played around with your test program and a slightly modified version of your patch (to handled the PropertyNotify vs. ClientMessage thing), but things still didn't behave quite right. I'm not sure if it's due to another metacity bug where we don't send ConfigureNotify events when we need to (thus making it so that wnck_window_get_geometry() can't get the right answer) or something else. But hopefully the above answers will help you play with it and you'll figure it out. :) As always, thanks for your patience and thanks for working on this.
Sticky notes make metacity curse each time they are moved: Window manager warning: Received a _NET_WM_MOVERESIZE message for 0x1e00024 (07/10/06); these messages lack timestamps and therefore suck. Evil sticky notes :(.
Diego: That's not really a sticky notes bug; it's a bug in the EWMH spec. Either way, that's completely unrelated to this bug.
Created attachment 74376 [details] [review] Addressing comment 12 I believe this one addresses all the points in comment 12. Enjoy! :-)
> I played around with your test program and a slightly modified version of your > patch (to handled the PropertyNotify vs. ClientMessage thing), but things still > didn't behave quite right. I'm not sure if it's due to another metacity bug > where we don't send ConfigureNotify events when we need to (thus making it so > that wnck_window_get_geometry() can't get the right answer) or something else. > But hopefully the above answers will help you play with it and you'll figure it > out. :) Actually, the behaviour is what I expected. It is in fact (almost) the same behaviour as in OpenBox. (In OB the window actuall moves around a bit as well, at least in MetaCity it stays put. :-) It isn't what I want though. This relates to another bug I raised, #344379. AFAICS what happens is that libwnck reports the size of the "inner window" (client) while the call to resize affects the "outer window" (border). My memory is a little sketchy here, but if I'm reading the EWMH specification correctly there is no simple way (my definintion of simple anyway) to ask the WM for the position and size of the border window. You _can_ ask the WM for the size of a window (including the border I assume) but you need a window that can receive the X message. Maybe it's time to d-bus enable metacity?
(In reply to comment #16) Forget comment #16. It's bullocks and I really ought to start thinking before I write stuff :-) After some testing I came across the problem Elijah mentions in comment #12. It seems that changing either size or position works fine, however when changing both only size is actually changed. Initial testing makes me believe that the problem lies in meta_window_move_resize_internal() (or below).
(In reply to comment #17) > (In reply to comment #16) > > Forget comment #16. It's bullocks and I really ought to start thinking before I > write stuff :-) > > After some testing I came across the problem Elijah mentions in comment #12. It > seems that changing either size or position works fine, however when changing > both only size is actually changed. Initial testing makes me believe that the > problem lies in meta_window_move_resize_internal() (or below). The code in meta_window_move_resize_internal() isn't very straight forward. Two questions: 1. How do I turn on the printing of meta_topic? 2. Is there a way to run metacity in a debugger?
You can just change meta_topic to meta_warning if you want a few selected messages (though you need to remove the first argument when you do so). I often do that -- though you have to be careful to not turn on too much output or metacity freezes (it waits for its output to get to the terminal it is being run from, while the terminal blocks waiting for metacity on something). If you just want to turn them all on, look at metacity/HACKING and read the bit about log files. Advance warning: the output is HUGE. As far as running metacity under a debugger, you can do so if you run it from a virtual terminal or a different display (e.g. different machine) that metacity is not managing. I haven't found doing so very useful all that often.
Created attachment 74665 [details] [review] Hunch to fix move+resize problem (In reply to comment #19) Thanks for the hint. Replacing the meta_topic() with meta_warning() turned out to be usable. I have tried understanding meta_window_move_resize_internal(). Weighing in a 400+ lines it's a gorilla of a funtion and I doubt I get it. The problem I had was that a move+resize turned into a resize only. On a bit of a hunch I added an else statement (see "Hunch to fix move+resize problem"). This does in fact have to effect that a move+resize does both move and resize. However, prior to this change the movement of windows was limited by panels, with this change that isn't the case any longer. Any pointers on how to address that?
Created attachment 75331 [details] [review] Attempt at finalising it. I tried digging into meta_window_constrain(). That proved to be a bit of a head ache. Left me feeling "there's got to be an easier way"! Since I didn't receive any feedback on my "hunch" I'm assuming it's alright and I hereby submit the combined patch :-) (obvious attempt to fish for feedback, I know)
Created attachment 86080 [details] Modified testcase Here's a testcase that avoids the frame vs. client window position issue by using static gravity. (Requires a patched version of libwnck that defines WNCK_WINDOW_GRAVITY_STATIC, but you could just use the constant 10 in place of this enum value since that's what it's defined to in the EWMH)
Created attachment 86081 [details] [review] Patch updated to HEAD, tested, fixed, and verified it works I think bug #426519 may have been biting us before. Anyway, things seem to work well with this patch so I'm going to commit it in a minute.
Committed in trunk now. Oh, and by the way, you can find out the size of the frame by checking the _NET_FRAME_EXTENTS property. (try 'xprop | grep EXTENTS' from the command line; not sure if libwnck supports this yet or not)
I've tried the HEAD version of metacity, and wmctrl is now able to resize the windows, but the position parameters have no efect on them (I basically wrote a little shell script which uses wmctrl to place all windows in current desktop in a matrix-like view). Is this patch supposed to just resize or should it be also possible to move the windows? Here's the kind of commands I'm running: wmctrl -i -r 0x03c0308e -e 0,0,0,512,512 Thanks, Lluis
The program attached in comment 22 modifies position & size, and I verified that it worked before committing the metacity patch. You can take a look at it and the libwnck code in svn (which the program uses) to see what might be the issue.
Created attachment 89809 [details] Window reshaping (bash+wnctrl)
Created attachment 89810 [details] Window reshaping (python-wnck)
(In reply to comment #26) > The program attached in comment 22 modifies position & size, and I verified > that it worked before committing the metacity patch. You can take a look at it > and the libwnck code in svn (which the program uses) to see what might be the > issue. > I've tested it, and it's not working well (on an amd64-like box). The test program resizes the window (as wmctrl did), but is not moving it. After downloading from svn metacity and wnck, it not only is not moving the window, but it is not resizing it as expected: Current geometry: 277,91 724x386 Current geometry: 277,91 715x401 Current geometry: 277,91 706x416 Current geometry: 277,91 697x431 Current geometry: 277,91 688x446 I also recompiled from svn pygtk, as I tried using python-wnck, but the script has no visible outcome (it could be wrong, as I couldn't test it). I've attached both scripts (being the python one the most accurate): Attachment #89809 [details] Attachment #89810 [details]
Could you file a new bug report? We don't want it to get lost in this closed bug report. Thanks!
(In reply to comment #30) > Could you file a new bug report? We don't want it to get lost in this closed > bug report. Thanks! > Filed Bug #446789 Thanks for your wonderful work!