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 351055 - libwnck does not yet support _NET_FRAME_EXTENTS
libwnck does not yet support _NET_FRAME_EXTENTS
Status: RESOLVED FIXED
Product: libwnck
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: libwnck maintainers
libwnck maintainers
: 344379 (view as bug list)
Depends on:
Blocks: 90134
 
 
Reported: 2006-08-12 16:27 UTC by Elijah Newren
Modified: 2007-06-10 16:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add _NET_FRAME_EXTENTS support (7.28 KB, patch)
2007-06-09 22:35 UTC, Vincent Untz
none Details | Review
Add _NET_FRAME_EXTENTS support, version 2 (10.58 KB, patch)
2007-06-10 08:37 UTC, Vincent Untz
none Details | Review
Add _NET_FRAME_EXTENTS support, version 3 (10.70 KB, patch)
2007-06-10 09:08 UTC, Vincent Untz
committed Details | Review

Description Elijah Newren 2006-08-12 16:27:10 UTC
See bug 339371/bug 90134 for a specific example of where supporting _NET_FRAME_EXTENTS would be useful, though I'm guessing there are many more.  I'm really surprised that we didn't already support this, and even more surprised that no bugs were filed about it.
Comment 1 Vincent Untz 2007-06-09 22:35:59 UTC
Created attachment 89665 [details] [review]
Add _NET_FRAME_EXTENTS support

I quickly test the patch by looking at the windows in the pager, and it seems to work.

Elijah: I don't think I'm missing something, but can you verify?
Comment 2 Vincent Untz 2007-06-09 22:37:12 UTC
*** Bug 344379 has been marked as a duplicate of this bug. ***
Comment 3 Elijah Newren 2007-06-10 02:25:05 UTC
Vincent, the crazy madman patching machine.  I don't know how you fix so many things so quickly, but it's pretty awesome.  :-)

Most of the patch looks fine to me; I just saw one part that I wanted to comment on:

@@ -1972,9 +1980,9 @@ wnck_window_get_geometry (WnckWindow *wi
   if (yp)
     *yp = window->priv->y;
   if (widthp)
-    *widthp = window->priv->width;
+    *widthp = window->priv->width + window->priv->left_frame + window->priv->right_frame;
   if (heightp)
-    *heightp = window->priv->height;
+    *heightp = window->priv->height + window->priv->top_frame + window->priv->bottom_frame;
 }


Are window->priv->x and window->priv->y the location of the client window's upper left corner, or the frame's upper left corner?  From the documentation of wnck_window_get_geometry, I assume it's the former.  If so, then it would only make sense that modifying the width and height to account for the frame ought to be accompanied by changes to the x and y coordinates to account for it too.

Also, this is effectively an API change; some apps may be depending on the previous meaning of wnck_window_get_geometry.  I'm fine with changing it anyway, but we do need to update the documentation of the function, and we probably want to provide a wnck_window_get_client_window_geometry function (possibly with a better name) that returns what the old function used to.  Those could be useful, for example, in configure requests to the window manager.
Comment 4 Vincent Untz 2007-06-10 08:36:02 UTC
(In reply to comment #3)
> Are window->priv->x and window->priv->y the location of the client window's
> upper left corner, or the frame's upper left corner?  From the documentation of
> wnck_window_get_geometry, I assume it's the former.  If so, then it would only
> make sense that modifying the width and height to account for the frame ought
> to be accompanied by changes to the x and y coordinates to account for it too.

Ah, good catch.

> Also, this is effectively an API change; some apps may be depending on the
> previous meaning of wnck_window_get_geometry.  I'm fine with changing it
> anyway, but we do need to update the documentation of the function, and we
> probably want to provide a wnck_window_get_client_window_geometry function
> (possibly with a better name) that returns what the old function used to. 
> Those could be useful, for example, in configure requests to the window
> manager.

Yes, I was wondering what was the best way to handle this. 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 was thinking that a new function to get the window geometry could be useful too, but I was just not sure.
Comment 5 Vincent Untz 2007-06-10 08:37:16 UTC
Created attachment 89683 [details] [review]
Add _NET_FRAME_EXTENTS support, version 2

Fix x and y, add wnck_window_get_client_window_geometry() and update the docs.
Comment 6 Vincent Untz 2007-06-10 09:08:30 UTC
Created attachment 89684 [details] [review]
Add _NET_FRAME_EXTENTS support, version 3

Makes it more robust in case the window manager is being a bad player .
Comment 7 Elijah Newren 2007-06-10 14:57:10 UTC
Don't know why I didn't catch this earlier, but doesn't the is_in_viewport function have the same problem, namely that the x & y coordinates should be modified by frame size if the width and height are?

@@ -2128,8 +2185,8 @@ wnck_window_is_in_viewport (WnckWindow  
 
   window_rect.x = window->priv->x + viewport_rect.x;
   window_rect.y = window->priv->y + viewport_rect.y;
-  window_rect.width = window->priv->width;
-  window_rect.height = window->priv->height;
+  window_rect.width = window->priv->width + window->priv->left_frame + window->priv->right_frame;
+  window_rect.height = window->priv->height + window->priv->top_frame + window->priv->bottom_frame;


Also, as far as documentation goes for wnck_window_get_geometry:
  * Gets the size and position of @window, as last received
  * in a ConfigureNotify event (i.e. this call does not round-trip
  * to the server, just gets the last size we were notified of).
  * The X and Y coordinates are relative to the root window.
  *
  * The returned values take into account the frame that is added
  * by the window manager around windows. If you need to know the
  * actual size of @window, use wnck_window_get_client_window_geometry().
contradicts itself.  I know what you mean, but I'm afraid it might cause confusion because the size and position as last received in a ConfigureNotify event is not what we're returning.  We're modifying it.  Perhaps change to something like

  * Gets the size and position of @window, including decorations.  This
  * function uses the information last received in a ConfigureNotify
  * event and adjusts it according to the size of the frame that is
  * added by the window manager (this call does not round-trip to the
  * server, it just gets the last sizes that we were notified of).  The
  * X and Y coordinates are relative to the root window.
  *
  * If you need to know the actual size of @window ignoring the frame
  * added by the window manager, use
  * wnck_window_get_client_window_geometry().

Other than those two nits; it looks good to me.
Comment 8 Vincent Untz 2007-06-10 16:22:30 UTC
Fixed the two issues. Thanks!