Created attachment 105903 [details]
Code sample exposing the bug

Sample case as promised.

Output from Compiz session:
** (wnck-test-align_windows:7064): DEBUG: Putting 'WinWrangler' at (0,0) @ 600x300
** (wnck-test-align_windows:7064): DEBUG: Putting 'mikkel@delight: ~/Projects/WindowExpander' at (0,300) @ 600x300
** (wnck-test-align_windows:7064): DEBUG: Found 'WinWrangler' at (-5,25) @ 610x330
** (wnck-test-align_windows:7064): DEBUG: Found 'mikkel@delight: ~/Projects/WindowExpander' at (-5,275) @ 607x324

Output from Metacity (trunk) session:
** (wnck-test-align_windows:7076): DEBUG: Putting 'WinWrangler' at (0,0) @ 600x300
** (wnck-test-align_windows:7076): DEBUG: Putting 'mikkel@delight: ~/Projects/WindowExpander' at (0,300) @ 600x300
** (wnck-test-align_windows:7076): DEBUG: Found 'WinWrangler' at (0,25) @ 610x330
** (wnck-test-align_windows:7076): DEBUG: Found 'mikkel@delight: ~/Projects/WindowExpander' at (0,300) @ 607x324

Created attachment 106340 [details]
Minimal test case

Here's a really minimal test case.

Please, when making test cases, make them minimal, it helps :-)

So, here's the output of the new test case:

Printing initial geometry
=========================
'Terminal' at (7,148) @ 917x574 (without decorations)
'Terminal' at (3,126) @ 925x600 (including decorations)

Changing geometry
=================
Set 'Terminal' to (3,126) @ 925x600 (including decorations)

Now, geometry changes...
========================
'Terminal' at (7,148) @ 925x589 (without decorations)
'Terminal' at (3,126) @ 933x615 (including decorations)

Looks weird. There's definitely something going on, and probably because of the decorations.

The new width, eg, is the old width + the width of decorations. I can't explain the new height, though.

I have a doubt now. Reading the EWMH spec, the only thing I can see is this: "Rationale: Using a _NET_MOVERESIZE_WINDOW message with StaticGravity allows Pagers to exactly position and resize a window including its decorations without knowing the size of the decorations." So I'm not sure _NET_MOVERESIZE_WINDOW should include the decorations...

That being said, the position of the window (x, y) does not change, which means the top-left corner is still fine, including decorations. I vote for a wm bug.

Let's have metacity people look at this.

Created attachment 106426 [details]
Output of wnck-test.c, and relevant metacity debug log

Your friendly neighbourhood metacity people confirm this bug.  Attached is the relevant part of the debug log; I'll work on this and see what comes up.

Crap, sorry guys.  This bug is my fault...and it's a bug in the libwnck API.  I didn't review the patches in bug 351055 nor read Vincent's comments closely enough.  In particular bug 351055 comment 4:

  "It seems to me that most people would understand wnck_window_set_geometry()
  and wnck_window_get_geometry() to be about the "same" geometry, so this API
  meaning change makes sense to me. Forgetting to update the doc is bad!"

I paid attention to the API change Vincent was making with wnck_window_get_geometry(), which seemed fine due to the fact that I ignored or overlooked the above connection Vincent was trying to make.  wnck_window_set_geometry() uses _NET_MOVERESIZE_WINDOW which should NOT be relative to the frame.

So, currently, the bug is in libwnck's documentation and in wnckprop (and, if one wants the correlation Vincent noted, then in the names of the functions themselves).  In particular, if we regard this as just a documentation bug, then the documentation of wnck_window_set_geometry() should be changed from
 
 * Note that the new size and position apply to @window with its frame added
 * by the window manager. Therefore, using wnck_window_set_geometry() with
 * the values returned by wnck_window_get_geometry() should be a no-op, while
 * using wnck_window_set_geometry() with the values returned by
 * wnck_window_get_client_window_geometry() should reduce the size of @window
 * and move it.

to

 * Note that the new size and position apply to @window without its frame added
 * by the window manager. Therefore, using wnck_window_set_geometry() with
 * the values returned by wnck_window_get_client_window_geometry() AND
 * WNCK_WINDOW_GRAVITY_STATIC should be a no-op, while using
 * wnck_window_set_geometry() with the values returned by
 * wnck_window_get_geometry() should increase the size of @window and move it.


The reason for this is the following:
  * _NET_MOVERESIZE_WINDOW is meant as an extension to ConfigureRequest events.
  * _NET_MOVERESIZE_WINDOW has the same meanings for flags as in
    ConfigureRequest events, but it allows a gravity to be specified.
  * Before the EWMH, clients had no way of knowing the frame's size, and were
    only knowledgeable about their actual window size, so it made no sense to
    have configure requests be relative to the frame.
  * Thus, by compatibility, _NET_MOVERESIZE_WINDOW messages are NOT relative to
    the frame.

Or, as stated by the EWMH rationale for _NET_MOVERESIZE_WINDOW:
    Rationale: Using a _NET_MOVERESIZE_WINDOW message with StaticGravity
    allows Pagers to exactly position and resize a window including its 
    decorations without knowing the size of the decorations.

Oh, nice :-)

What about this:

 + fix wnck_window_set_geometry() to take into account the decorations
 + add wnck_window_set_client_window_geometry(), which would keep the current wnck_window_set_geometry() behavior

It'd probably be a good thing to clarify the EWMH too, since I got confused partly because of it.

Created attachment 106458 [details] [review]
Patch for 2.23

With this patch I can now manually tile my windows with wnckprop.

I have not tested whether get/set_client_window_geom is actually a noop.

(In reply to comment #6)
> What about this:
> 
>  + fix wnck_window_set_geometry() to take into account the decorations
>  + add wnck_window_set_client_window_geometry(), which would keep the current
> wnck_window_set_geometry() behavior

Seems like a good way to fix this.

> It'd probably be a good thing to clarify the EWMH too, since I got confused
> partly because of it.

Yeah, Thomas was noting that it was hard to figure out what the EWMH wanted here as well.

Mikkel: Your patch looks good to me, but looking at the code without testing it is how I messed us up last time.  :-)  I'd like to see someone test it and make sure that things that should be no-ops really are; after that, I'm happy with the patch going in (unless Vincent objects...)  I gotta run right now; I'll test it tonight if no one else beats me to it...

I don't think we want to add the new API now, we'll just add it for 2.23. Let's just fix the broken function for now.

Feel free to update the patch for this. I'll try to look at this tonight.

Created attachment 106507 [details] [review]
Patch (for 2.22) only updating set_geometry to take frames into account

Patch updated to not cause API changes. I think this should do what is described in the documentation (ie respect window borders).

I've committed the 2.22 patch.

The 2.23 patch has a small problem: wnck_window_set_geometry() doesn't have the g_return_if_fail() anymore, but it should.

Created attachment 106517 [details] [review]
Small fix to patch on 2.22

I did not calculate the frame induced offset corretly for the top frame in my last patch. Sorry.

I don't understand. I tested the patch and didn't see this... Anyway, please commit.

Patch was committed.

We'll add the missing function when we'll branch for 2.22.

I'm still experiencing problems with this under fedora's 2.24.  My understanding of this bug is that the patch was applied?  If so, something is still wrong.

[jas@talby wnckTest]$ uname -a
Linux talby 2.6.27.4-68.fc10.x86_64 #1 SMP Thu Oct 30 00:25:13 EDT 2008 x86_64 x86_64 x86_64 GNU/Linux
[jas@talby wnckTest]$ rpm -q libwnck
libwnck-2.24.1-1.fc10.x86_64
[jas@talby wnckTest]$ ./wnck-test 
Printing initial geometry
=========================
'screen' at (13,134) @ 1166x695 (without decorations)
'screen' at (10,113) @ 1172x719 (including decorations)

Changing geometry
=================
Set 'screen' to (10,113) @ 1172x719 (including decorations)

Now, geometry changes...
========================
'screen' at (16,155) @ 1166x695 (without decorations)
'screen' at (13,134) @ 1172x719 (including decorations)

(In reply to comment #16)
> I'm still experiencing problems with this under fedora's 2.24.  My
> understanding of this bug is that the patch was applied?  If so, something is
> still wrong.

compiz or metacity?

I'm running metacity: metacity-2.24.0-2.fc10.x86_64

In case it helps, here is a little python program that illustrates the issue.

It prints:

Requested: 1, 50, 550, 250
Got      : 4, 71, 548, 246

#!/usr/bin/env python

import pygtk
pygtk.require('2.0')
import gtk
import gobject
import wnck

class WindowPlacer:

    def printGeometry(self, screen):
      # Compare new geometry with requested geometry
      window=screen.get_active_window()
      geom=window.get_geometry()
      print "Got      : " + repr(geom[0]) + ", " + repr(geom[1]) + ", " + repr(geom[2]) + ", " + repr(geom[3])

      # Quit the main loop
      gtk.main_quit()

    def setGeometry(self, screen):
      window=screen.get_active_window()

      # Indicate that we want to change all geometry params
      flags=wnck.WINDOW_CHANGE_X  | wnck.WINDOW_CHANGE_Y | wnck.WINDOW_CHANGE_WIDTH | wnck.WINDOW_CHANGE_HEIGHT

      # Set the geometry, flush pending events, and force an update
      left=1
      top=50
      width=550
      height=250
      window.set_geometry(wnck.WINDOW_GRAVITY_NORTHWEST, flags, left, top, width, height)
      print "Requested: " + repr(left) + ", " + repr(top) + ", " + repr(width) + ", " + repr(height)

      # Quit the main loop
      gtk.main_quit()
    
    def main(self):
      screen=wnck.screen_get_default()
      gobject.idle_add(self.setGeometry, screen)
      gtk.main()
      screen.force_update()
      gobject.idle_add(self.printGeometry, screen)
      gtk.main()

# This is the entry point if we are invoked on the command-line
if __name__ == "__main__":
  windowPlacer = WindowPlacer()
  windowPlacer.main()

From metacity:

gboolean
meta_window_configure_request (MetaWindow *window,
                               XEvent     *event)
{
  /* Note that x, y is the corner of the window border,
   * and width, height is the size of the window inside
   * its border, but that we always deny border requests
   * and give windows a border of 0. But we save the
   * requested border here.
   */
  if (event->xconfigurerequest.value_mask & CWBorderWidth)
    window->border_width = event->xconfigurerequest.border_width;

So, I'm lost: is this a metacity bug or a libwnck bug? It seems, according to this metacity comment that we shouldn't do this in libwnck:

  x += window->priv->left_frame;
  y += window->priv->top_frame;

Thomas, Elijah, any idea?

Vincent, thanks for investigating this.  Just some background from a user perspective.

I've been wanting a window tiling/placement utility for years, and in frustration, decided to write one myself.  This bug obviously impacts such a solution - a developer cannot use libwnck to place windows!  Could you suggest another approach in the interim?  Perhaps I should be using another API?  I am new to gnome programming, and have been bumbling around with gtk.gdk APIs with no luck so far (probably out of ignorance).

Thanks again, Jason.

Created attachment 122771 [details]
Tweak of Jason's Python script

I think one needs to use STATIC gravity for automated window placement. Also I think that the multiple main loops might have been playing tricks, at least it appeared to do so for my small hackery just now.

The attached code works for (TM). The changes compared to the original is that I placed everything in idle handlers, use only one main loop, and use STATIC gravity.

Jason, have you checked out WinWrangler? http://www.grillbar.org/wordpress/?p=306

WinWrangler is working fine here... Except for not being able to place terminal windows correctly under Metacity (because they can not be resized smoothly, only in factors determined by the height and width of the font). This is of course a bug in WW.

s/works for/works for me/

Thanks Mikkel,

Looks like the STATIC gravity was indeed the trick.  Your variant is working for me too now (but I need to run it a second time before printGeometry reports the new placement of the window - I'm guessing this is the same main loop trickery).

[jas@talby place]$ ./place.py
Requested: 1, 50, 550, 250
Got      : 171, 73, 998, 851
[jas@talby place]$ ./place.py 
Requested: 1, 50, 550, 250
Got      : 1, 50, 548, 246

Thanks again - will report back here if I encounter problems.  Thanks also for the tip about WinWrangler - will check it out.

(In reply to comment #22)
> I think one needs to use STATIC gravity for automated window placement.

Hrm. I'd need to put myself in libwnck-mode again, but this sounds like we either need to fix the doc to mention this or that wnck_window_set_geometry() doesn't work as expected for most gravities (or maybe metacity is broken).

I'd say it's not just a doc bug: I believe the gravity is only useful when the window is resized, and this is not the case with the test case -- so the gravity should not be relevant here.

I seem to still have a problem with wnck_window_set_geometry()

I'm on Debian 7 with Openbox. In my python script I use 
window.set_geometry(10,255,0,0,w,h)
where w and h are width and heigth of my screen. (I ried with different gravities in place of 10 but the effect is the same.)

If window happens to be undecorated, the above works as expected, i.e. upper-left corner is at (0,0). However, if window is decorated then the upper-left corner after the operation is at (0, heigth of decoration) instead of (0,0).