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 148364 - Enhanced EWHM compliance
Enhanced EWHM compliance
Status: RESOLVED FIXED
Product: metacity
Classification: Other
Component: general
2.8.x
Other Linux
: High normal
: ---
Assigned To: Metacity maintainers list
Metacity maintainers list
Depends on:
Blocks: 340691
 
 
Reported: 2004-07-24 20:31 UTC by Rob Adams
Modified: 2008-02-21 02:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Improve some weak spots in the EWMH support (13.32 KB, patch)
2004-07-24 20:32 UTC, Rob Adams
none Details | Review
Updated patch (16.24 KB, patch)
2004-07-25 17:37 UTC, Rob Adams
none Details | Review
Excerpt from libwnck/xutils.c (dev. version) (1.06 KB, text/plain)
2004-07-27 19:51 UTC, carniz
  Details
The libwnck patch (4.17 KB, patch)
2004-07-27 20:35 UTC, carniz
none Details | Review
Final patch (17.45 KB, patch)
2004-08-01 07:49 UTC, Rob Adams
committed Details | Review
The slightly modified libwnck patch (4.16 KB, patch)
2004-08-03 20:54 UTC, carniz
needs-work Details | Review

Description Rob Adams 2004-07-24 20:31:30 UTC
Support additional EWHM hints, including DEMANDS_ATTENTION which will be
required to make the user_time stuff not suck.
Comment 1 Rob Adams 2004-07-24 20:32:46 UTC
Created attachment 29859 [details] [review]
Improve some weak spots in the EWMH support
Comment 2 Rob Adams 2004-07-25 02:07:47 UTC
Note that the flags in handle_net_move_resize window are slightly wrong.  I have
a version that fixes that.
Comment 3 Havoc Pennington 2004-07-25 05:13:10 UTC
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.
Comment 4 Havoc Pennington 2004-07-25 05:14:15 UTC
heh, I hadn't seen your comment about the flags.
Comment 5 Rob Adams 2004-07-25 17:37:52 UTC
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.
Comment 6 Rob Adams 2004-07-27 17:09:41 UTC
ping; I think it's important to get these fixes in fairly soon, especially for
DEMANDS_ATTENTION.
Comment 7 carniz 2004-07-27 19:51:07 UTC
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?)
Comment 8 Rob Adams 2004-07-27 19:58:04 UTC
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 :-).
Comment 9 carniz 2004-07-27 20:35:19 UTC
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.
Comment 10 Rob Adams 2004-07-31 20:02:40 UTC
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.
Comment 11 carniz 2004-07-31 22:15:32 UTC
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)?
Comment 12 Rob Adams 2004-07-31 23:33:55 UTC
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?
Comment 13 Havoc Pennington 2004-08-01 03:28:46 UTC
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.
Comment 14 Rob Adams 2004-08-01 07:49:43 UTC
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.
Comment 15 carniz 2004-08-01 09:16:24 UTC
>>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.




Comment 16 carniz 2004-08-03 20:54:54 UTC
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)
Comment 17 Elijah Newren 2005-05-26 19:01:12 UTC
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 18 Havoc Pennington 2005-05-27 13:57:56 UTC
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.
Comment 19 Elijah Newren 2008-02-21 02:15:32 UTC
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.