refactor window geometry code

[totaam]

Issue migrated from trac ticket # 990

component: server | priority: critical | resolution: fixed

2015-09-26 05:51:10: antoine created the issue

---

Split from #907.

There are too many places recording the same information, all with slightly different intents: the X11 window itself (and its parents..), the window model classes, the desktop manager, (and of course: the client)...
Also relevant: the ownership information, initial window position and size hints, multiple clients (#41), etc..

This is a blocker for a number of tickets: #846, #988, #881 - and probably many others.

[totaam]

2015-10-23 16:28:08: antoine changed status from new to assigned

[totaam]

2015-10-23 16:28:08: antoine commented

---

The basics:

• the geometry property of all window models claims to be current (border-corrected, relative to parent) coordinates (x, y, w, h) for the window, it is actually retrieved via:
• do_get_property_geometry returns the value of the _geometry attribute, if it is still set to None this method will first attempt to populate it by calling XGetWindowAttributes and use the first 5 attributes from the XWindowAttributes structure:

int x, y;			/* location of window */
int width, height;		/* width and height of window */
int border_width;		/* border width of window */

• do_xpra_configure_event is the handler for ConfigureNotify X11 events, in the base class implementation it will simply update the window geometry with the event data and then fire the "geometry" notify signal
• get_position and get_dimensions are just shorthand versions for accessing parts of the geometry

The real trickery starts in [/browser/xpra/trunk/src/xpra/x11/gtk2/models/window.py WindowModel]:

• _NET_MOVERESIZE_WINDOW messages cause the window geometry to get updated, after sanitizing the event data with the size-hints, we call XConfigureWindow followed by a ConfigureNotify
• maybe size-hints should be moved here - only used here and doesn't make sense for OR windows... (what about "strut" - does that make any sense for OR windows?)
• actual-size should be the same as size portion of the geometry?
• during initialization, we populate requested-position and requested-size from the data we get back from calling XGetGeometry
• do_xpra_configure_event is a little bit different and calls resize_corral_window which may or may not update actual-size and user-friendly-size and "geometry"
• do_child_configure_request_event is the handler for ConfigureRequest Events and will update the geometry using the event's attribute, for the attributes which aren't set it will fallback to using requested-position and requested-size (is that right??) - the actual updating is done via _update_client_geometry

And the real ugly parts:

• ownership_election probably deserves a entire wiki page to itself... it may reparent the window to the parking window, call _update_client_geometry, raise the corral window, etc..
• maybe_recalculate_geometry_for is similar: checks for ownership may call _update_client_geometry
• _update_client_geometry will call _do_update_client_geometry with values it gets from the owner (if there is one), or from requested-position and requested-size
• _do_update_client_geometry does geometry calculations (honour "size-hints", etc) and then does the configure + notify

The [/browser/xpra/trunk/src/xpra/x11/gtk2/wm.py WM] object:

• interacts with the root window for some global properties (workarea, virtual desktops, dpi, frame extents)
• does some of the proper window-manager selection business, ie: XSetSelectionOwner
• tells the X11 server we want to receive all the window, focus, bell and cursor events
• it also takes care of creating window models for all non-OR windows..
• it creates the world window and does some focus exchange with it..
• it handles some root window messages: showing desktop, frame extents (nothing important)
• do_child_configure_request_event does a configure + notify for windows that don't have a model yet

The [/browser/xpra/trunk/src/xpra/x11/gtk2/world_window.py WorldWindow] is just about GTK / X11 focus hell. Not very relevant here.

The [/browser/xpra/trunk/src/xpra/x11/server.py DesktopManager] is a hidden gtk widget which sits between out server class and the window models, deal with the ownership election business.
Each window is also recorded there using a (very confusingly named) model containing the following attributes:

• shown: for handling iconification
• geom: yet another copy of the geometry, initially set to the window's geometry, but this can be update when the client resizes a window, in which case we end up in maybe_recalculate_geometry_for
• resize_counter: used for arbitration and preventing resizing loops: when the application resizes itself in response to client resizing, or when an application resizes itself and the client later honours it (by which time the application may have resized itself again)

The [/browser/xpra/trunk/src/xpra/server/source.py Source] and [/browser/xpra/trunk/src/xpra/server/window/window_source.py WindowSource] try very hard not to deal with any geometry stuff, for 2 reasons:

• much cleaner design
• the encoding stage is not running in the UI thread, which makes it impossible to query GTK or X11
  The only call made is to get_dimensions so we know the size of the window, and get_image to get the window pixels.

The server is where some of the ugliest code lives:

• when registering a new window:

        _, _, w, h, _ = window.get_property("client-window").get_geometry()
        x, y, _, _, _ = window.corral_window.get_geometry()

• when retrieving the geometry of an existing window:

            if window.is_OR() or window.is_tray():
                x, y, w, h = window.get_property("geometry")[:4]
            else:
                x, y, w, h = self._desktop_manager.window_geometry(window)[:4]            

Maybe we should keep the desktop manager, but have one per client with some shared data structures? (for things like resize counter?) Maybe it should handle OR windows too? (and just delegate those calls so we don't have this ugly if/else mess)

Maybe start by exposing as much as we can via xpra info.

[totaam]

2015-10-26 11:32:03: antoine commented

---

More notes:

• r11016 moves size-hints to window model
• r11020 removes user-friendly-size (not used)
• r11017 removes get_position and r11031 removes actual-size, we now use the geometry attribute instead
• removing get_dimensions would be too hard for now - and as of r11031 it's implemented just once in core, cleanly
• actual-size should always be the same as the corral window size - the only external caller is the x11 server's _window_resized_signaled which could access the corral window if needed
• new / improved tests: r11023, r11028
• fixes: r11019, r11029, r11025, r11024 (scaling related, found during window move testing)
• debug logging added or improved: r11030, r11027, r11019, r11026, r11025, r11022, r11021
• requested-size and requested-position are set from the window's initial geometry (queried via XGetGeometry), we update it when receiving a configure request on the child window (when the application requests it), but not when we handle _NET_MOVERESIZE_WINDOW messages. They are only used for the geometry when: there is no owner, or when the configure request omits some values. This looks fine.
• the "ownership" stuff could be re-used for better multi-client support: the one that moved or focused the window last would become the owner, maybe_recalculate_geometry_for already handles most of this
• when we get a configure notify for the corral window, shouldn't we verify the position part of the geometry is correct? and update it if not?

Remaining tasks:

• remove border from geometry (unused)
• remove the do_get_property_geometry ugly accessor method, it should never be used if we keep the geometry up to date, and use updateprop to fire geometry notifications?
• add some logging to verify all geometry attributes make sense and are consistent with their intended purpose, test with OR windows, windows that trigger move, resize, etc
• remove the need to know which type of window you are dealing with to know its real position on screen: go through the desktop manager for all, and expose via xpra info

[totaam]

2015-10-27 15:36:33: antoine commented

---

More updates, see:

• r11035: doc update
• r11051: big, see commit message
• r11052: better debugging
• r11053: better configure request handling
• r11054: simplify code
• r11055 + r11056: turn "geometry" into a regular gobject property
• r11057: window message handling fixes

Note: this removes the border attribute from geometry, which we never really used anyway.

Things that will need checking:

• iconification / resizing loops, Windows is continous flashing in loop #790

[totaam]

2015-10-28 03:57:44: antoine changed status from assigned to new

[totaam]

2015-10-28 03:57:44: antoine changed owner from antoine to afarr

[totaam]

2015-10-28 03:57:44: antoine commented

---

Summary: the window geometry code is cleaner and more consistent. It is now handled as a regular property.

The current geometry is always accessible with:

$ xpra info | grep -e "window.*geometry="
window[1].client-geometry=(370, 132, 499, 316)
window[1].geometry=(370, 132, 499, 316)

(the client-geometry is for non-OR windows and shows where the window is mapped for this particular client - which should be the same as the server's geometry)

This the same geometry which is also used with xpra screenshot and with the sync-xvfb option (#988).

These changes need to be tested with most of the [/trac/browser/xpra/trunk/src/tests/xpra/test_apps window test apps] to make sure this has not caused any regressions.
Will follow up in #41, #846 and #881.

[totaam]

2015-11-04 00:40:51: maxmylyn changed owner from afarr to antoine

[totaam]

2015-11-04 00:40:51: maxmylyn commented

---

Tested a Fedora 21 trunk r11118 Server from a Fedora 20 trunk r11118 Client:

Played around with the python test scripts for a while

• All working as expected....or at least identical to running it locally

• a couple spawned a weird secondary panel in addition to the main window...but they did that running locally so I guess it was expected. They behaved nicely, it was just unexpected

• No noticeable regressions

---

Passing back to you for now. Pass it back to us (either afarr or myself) if you need some more testing, as always.

[totaam]

2015-11-13 11:26:39: antoine changed status from new to closed

[totaam]

2015-11-13 11:26:39: antoine set resolution to fixed

[totaam]

2015-11-13 11:26:39: antoine commented

---

Does not seem to have caused any major breakage after all, will follow up in the other geometry related tickets: #723, #772, #994?, #809, #881, #846, #968, #911

[totaam]

2016-02-15 14:09:08: antoine commented

---

Breakage found: #1120.

[totaam]

2016-03-16 05:39:01: antoine commented

---

See also #1121.