[Spice-devel] [PATCH vdagent-win] vdagent: fix cursor position sync on multimon rhbz#757819

[Alon Levy]

On Wed, Feb 01, 2012 at 10:48:29AM +0200, Arnon Gilboa wrote:
> Thanks Alon, adding these lines to the comment:
> 
> In VDAgent::handle_mouse_event(), mouse event is generated by
> scaling the received (x,y, display_id) to normalized absolute
> coordinates (0..0xffff) on the entire virtual desktop which contains
> all the displays. Keeping negative display coordinates, as received
> from the client, was mistakenly handled and generated wrong
> (sometimes even negative) coordinates, see:
> _input.mi.dx = (mode->get_pos_x() + _mouse_x) * 0xffff /
> _desktop_layout->get_total_width();
> _input.mi.dy = (mode->get_pos_y() + _mouse_y) * 0xffff /
> _desktop_layout->get_total_height();
> 
> Is it clear now?

That wasn't the point. I wasn't talking about what goes wrong with the
computation, I was talking about what sequence of events leads to the
client sending monitors locations that are negatively offset to the origin.

Anyway, if you already wrote it it wouldn't hurt to be in the commit
message I guess :)

> 
> Alon Levy wrote:
> >On Mon, Jan 23, 2012 at 12:59:22PM +0200, Arnon Gilboa wrote:
> >>On any change of the display settings, driven by client message or guest user,
> >>normalize all display positions to non-negative coordinates, and update total
> >>width and height of the virtual desktop.
> >>
> >>The bug was due to wrong handling of (non-primary) displays positioned on
> >>negative coordinates.
> >
> >Looks good, but can you give a better example of how something goes
> >wrong in the patch comment?
> >
> >ACK
> >
> >>---
> >> vdagent/desktop_layout.cpp |   52 +++++++++++++++++++++++++++----------------
> >> vdagent/desktop_layout.h   |    1 +
> >> 2 files changed, 34 insertions(+), 19 deletions(-)
> >>
> >>diff --git a/vdagent/desktop_layout.cpp b/vdagent/desktop_layout.cpp
> >>index 0eada52..0ba7248 100644
> >>--- a/vdagent/desktop_layout.cpp
> >>+++ b/vdagent/desktop_layout.cpp
> >>@@ -44,10 +44,6 @@ void DesktopLayout::get_displays()
> >>     DEVMODE mode;
> >>     DWORD display_id;
> >>     DWORD dev_id = 0;
> >>-    LONG min_x = 0;
> >>-    LONG min_y = 0;
> >>-    LONG max_x = 0;
> >>-    LONG max_y = 0;
> >>     bool attached;
> >>     lock();
> >>@@ -81,22 +77,10 @@ void DesktopLayout::get_displays()
> >>         attached = !!(dev_info.StateFlags & DISPLAY_DEVICE_ATTACHED_TO_DESKTOP);
> >>         EnumDisplaySettings(dev_info.DeviceName, ENUM_CURRENT_SETTINGS, &mode);
> >>         _displays[display_id] = new DisplayMode(mode.dmPosition.x, mode.dmPosition.y,
> >>-                                            mode.dmPelsWidth, mode.dmPelsHeight,
> >>-                                            mode.dmBitsPerPel, attached);
> >>-        if (attached) {
> >>-            min_x = min(min_x, mode.dmPosition.x);
> >>-            min_y = min(min_y, mode.dmPosition.y);
> >>-            max_x = max(max_x, mode.dmPosition.x + (LONG)mode.dmPelsWidth);
> >>-            max_y = max(max_y, mode.dmPosition.y + (LONG)mode.dmPelsHeight);
> >>-        }
> >>-    }
> >>-    if (min_x || min_y) {
> >>-        for (Displays::iterator iter = _displays.begin(); iter != _displays.end(); iter++) {
> >>-            (*iter)->move_pos(-min_x, -min_y);
> >>-        }
> >>+                                                mode.dmPelsWidth, mode.dmPelsHeight,
> >>+                                                mode.dmBitsPerPel, attached);
> >>     }
> >>-    _total_width = max_x - min_x;
> >>-    _total_height = max_y - min_y;
> >>+    normalize_displays_pos();
> >>     unlock();
> >> }
> >>@@ -147,10 +131,40 @@ void DesktopLayout::set_displays()
> >>     }
> >>     if (dev_sets) {
> >>         ChangeDisplaySettingsEx(NULL, NULL, NULL, 0, NULL);
> >>+        normalize_displays_pos();
> >>     }
> >>     unlock();
> >> }
> >>+// Normalize all display positions to non-negative coordinates and update total width and height of
> >>+// the virtual desktop. Caller is responsible to lock() & unlock().
> >>+void DesktopLayout::normalize_displays_pos()
> >>+{
> >>+    Displays::iterator iter;
> >>+    DisplayMode* mode;
> >>+    LONG min_x = 0;
> >>+    LONG min_y = 0;
> >>+    LONG max_x = 0;
> >>+    LONG max_y = 0;
> >>+
> >>+    for (iter = _displays.begin(); iter != _displays.end(); iter++) {
> >>+        mode = *iter;
> >>+        if (mode->_attached) {
> >>+            min_x = min(min_x, mode->_pos_x);
> >>+            min_y = min(min_y, mode->_pos_y);
> >>+            max_x = max(max_x, mode->_pos_x + (LONG)mode->_width);
> >>+            max_y = max(max_y, mode->_pos_y + (LONG)mode->_height);
> >>+        }
> >>+    }
> >>+    if (min_x || min_y) {
> >>+        for (iter = _displays.begin(); iter != _displays.end(); iter++) {
> >>+            (*iter)->move_pos(-min_x, -min_y);
> >>+        }
> >>+    }
> >>+    _total_width = max_x - min_x;
> >>+    _total_height = max_y - min_y;
> >>+}
> >>+
> >> bool DesktopLayout::consistent_displays()
> >> {
> >>     DISPLAY_DEVICE dev_info;
> >>diff --git a/vdagent/desktop_layout.h b/vdagent/desktop_layout.h
> >>index caab84f..c43ff03 100644
> >>--- a/vdagent/desktop_layout.h
> >>+++ b/vdagent/desktop_layout.h
> >>@@ -73,6 +73,7 @@ public:
> >> private:
> >>     void clean_displays();
> >>+    void normalize_displays_pos();
> >>     static bool consistent_displays();
> >>     static bool is_attached(LPCTSTR dev_name);
> >>     static bool get_qxl_device_id(WCHAR* device_key, DWORD* device_id);
> >>-- 
> >>1.7.4.1
> >>
> >>_______________________________________________
> >>Spice-devel mailing list
> >>Spice-devel at lists.freedesktop.org
> >>http://lists.freedesktop.org/mailman/listinfo/spice-devel
>