Created attachment 29859 [details] [review]
Improve some weak spots in the EWMH support

Note that the flags in handle_net_move_resize window are slightly wrong.  I have
a version that fixes that.

Thanks for the patch.

What's _NET_RESTACK_WINDOW? I'm not seeing it in the spec, though I'm probably
blind. It doesn't appear different from XRaiseWindow()?

Two problems here:
+  unsigned int gravity = (event->xclient.data.l[0] & 
+                          (NorthWestGravity | NorthGravity | NorthEastGravity | 
+                           WestGravity | CenterGravity | EastGravity |
SouthWestGravity | 
+                           SouthGravity | SouthEastGravity | StaticGravity));

1) variable declared not at the top of the function, C89 won't compile
2) gravity is an enum, not flags, the values are just 1 2 3 4 5;
 you want "gravity = ...l[0] & ~0xff" or something like that
 (I *think* the spec says the low-order byte is the enum and the rest is 
  flags, but it's a little vague)

Don't think size_hints.win_gravity can be set by the _NET_MOVERESIZE_WINDOW
request, clients expect to be controlling that value. setting
size_hints.[x,y,w,h] is just a sort of weird hack because those values in 
the size hints happen to be unused, iirc. Doesn't extend to win_gravity.

Overall more code can probably be shared with meta_window_configure_request,
after all this is just a configure request with a dynamic gravity instead of 
using size_hints.win_gravity

CWX, CWY, etc. don't seem to correspond to bits 8-11 in _NET_MOVERESIZE_WINDOW,
also you're using CWX three times.

The only_resize thing is only needed in meta_window_configure_request() because
of the weird crack where gravity is only used if the configure request specifies
position; for _NET_MOVERESIZE shouldn't need this, there's no optimization
advantage afaik.

I'm a bit uncomfortable adding _NET_MOVERESIZE with nothing using it, not even
some random test program. How would we know it works?

set_net_wm_state(window); space before parens ;-)

Nice bugfixes adding USER_TIME to the list of supported hints, using the
timestamp from _NET_ACTIVE_WINDOW, etc.

Still pending of course, make demands_attention actually do something... though
I guess this could be entirely done in libwnck instead.

heh, I hadn't seen your comment about the flags.

Created attachment 29877 [details] [review]
Updated patch

OK here's a even more splendiferous version of this patch, and also includes
support for _NET_DESKTOP_GEOMETRY and _NET_DESKTOP_VIEWPORT hints, which brings
the number of unsupported EWMH hints in metacity to zero (though there are
still a few partially supported hints).

I've included a comment in the COMPLIANCE files and in the
handle_net_moveresize_window that this hint appears to have fatal endian
issues, but I think that the current version is about as correct as its gonna
get.

I initially wanted to reuse the code in meta_window_configure_request, but
decided that it wasn't practical without allocating my own Xevent and filling
it in with information from the message, but eventually decided that was too
ugly.  A big chunk of the code in there is for various client workarounds,
which I decided aren't needed in the EWMH message-handling code.

The _NET_RESTACK_WINDOW is in the wm-spec.xml XML file, right next to the
MOVERESIZE_WINDOW hint.  Note that the XML file in freedesktop CVS is much much
more up to date than the web site, which doesn't appear to have been updated in
a long time.

ping; I think it's important to get these fixes in fairly soon, especially for
DEMANDS_ATTENTION.

Created attachment 29957 [details]
Excerpt from libwnck/xutils.c (dev. version)

This is an excerpt from a libwnck hack that tries to implement a
moveresize_window function. I patched metacity with
http://bugzilla.gnome.org/attachment.cgi?id=29877&action=view and used the code
you'll find in this attachment to test the metacity patch, but without success.


Note: the libwnck hack implements how I *think* one is supposed to send a
_NET_MOVERESIZE_WINDOW message, but this is my very first attempt at
interacting with X so I could very well be wrong - I just looked at how the
other functions in libwnck/xutils.c were implemented. Please check the code to
see if there's something I've missed... (btw, is gravity really something you
should have to care about if you want to make an absolute move/resize?)

Thanks; do you have the test program you used for that?  It would be helpful in
debugging.

I'm not terribly worried about checking in a buggy version of this since it
won't break anything, and we can always fix it later :-).

Created attachment 29963 [details] [review]
The libwnck patch

Ok, here's the scenario:

I have developed a window tiler utility that's completely finished except from
code style refactoring (I'm an old Java developer..) and translation of
comments, plus a libwnck hack (that I don't know if works or not since it
relies on that metacity patch)

I have attached the libwnck hack, and a tarball with the *NOT release-ready*
gtiler code can be found at 

http://www.studenter.hb.se/~arch/files/gtiler.tar.gz

Contact me if you have any questions.

I went ahead and committed the uncontroversial parts of the patch, including
demands attention support, and the fix for the NET_ACTIVE_WINDOW, and adding the
viewport and geometry hints.

The restack window and the moveresize window support is #if 0'd out right now,
since I'm not really sure if adding that support would qualify as a bug fix or
not, though EWMH non-compliance could always be considered a bug I would think.

Why not enable the moveresize window support, if it's working? If you did, you
could mark all(..) moveresize enhancement bug requests as fixed as well (if
there are any). (if there are no enhancement requests for moveresize_window,
I'll add one right away anyhow :) )

What's more, I could start working towards finalizing gtiler (or
gnome-window-tiler, which I'll rename it to). I seriously think GNOME does need
a window tiler utility, especially since nautilus went spatial.

Does gtiler work for you, with these pathes applied (metacity and libwnck)?

You can easily enough enable the support by checkout out HEAD and removing #if
0/#endif pairs where appropriate.  The main reason is that I'm not sure that the
implementation of the hint is correct, and metacity is in feature freeze right
now in preparation for the upcoming Gnome 2.8 release.

Incidentally, if you want to implement window tiling as a patch for metacity, I
see no reason why this could not be added (for gnome 2.10).  I think that as a
set of keybindings at first, then perhaps even as a window menu entry.  Or
perhaps it should be implemented as part of libwnck, using the NET_MOVERESIZE hints?

Can you please attach the final exact patch you committed? Generally speaking
all committed patches should be attached to a bug, in their final form.

Created attachment 30128 [details] [review]
Final patch

Sorry, I thought it was clear; the previous patch was the final patch, but with
moveresize and restack #if 0'd out.  Here's an rdiff with the exact final
version of the patch to make it simpler.

>>Incidentally, if you want to implement window tiling as a patch for metacity, I
>>see no reason why this could not be added (for gnome 2.10).  I think that as a
>>set of keybindings at first, then perhaps even as a window menu entry.

That's my plan, yes. Actually, I think it could be nifty to reach the tiler from
both metacity (a window menu entry) and the workspace applet (right click on
applet workspace -> "Organize windows.."). Too bad I didn't start working on
this earlier though, it would have been cool to have in 2.8. Oh well.

Created attachment 30186 [details] [review]
The slightly modified libwnck patch

It seems as the callback to display.c#handle_net_moveresize_window fails to be
trigged by _NET_MOVERESIZE_WINDOW events sent to the server. Code for testing
this can be found in the (now slightly modified) libwnck patch, tentatively
used together with the gtiler utility
(http://www.studenter.hb.se/~arch/files/gtiler.tar.gz)

It looks like Rob's patch has been committed, so I'm marking it as such.  I'm
tempted to mark carniz's patch as needs-work (as in 'needs to be filed against
libwnck') but Rob and Havoc are more familiar with that part of the spec and I'm
afraid they'd be less likely to review it if were moved, so I'll leave it alone.  :)

Comment on attachment 30186 [details] [review]
The slightly modified libwnck patch

I usually try to avoid "gint" (I think libwnck avoids it anyhow, maybe I
remember wrongly)

Since the point of _NET_MOVERESIZE_WINDOW is to let you pass in a gravity, I
think we want to either have a gravity argument to wnck_window_set_geometry()
or use StaticGravity always. I don't see any value in using
WM_SIZE_HINTS.win_gravity, that just makes the behavior undefined.

I think Magnus did a bunch of work on the wnck_window_set_geometry, so I don't think there's anything left in this bug report that hasn't been taken care of.  I'll go ahead and close; shout if I missed something.