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

[Alon Levy]

On Wed, Feb 01, 2012 at 04:38:44PM +0200, Arnon Gilboa wrote:
> Alon Levy wrote:
> >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 :)
> >
> Primary monitor is always located (in windows, what about linux?) in
> (0,0). When you position a secondary monitor above/to the left of
> the primary, it will have negative coordinate. This can be the case
> when guest display settings are changed either manually on the
> guest, or due to a message (auto-conf's VD_AGENT_MONITORS_CONFIG)
> from the client to the agent.
> 
> Does it answer your question? :/

Yes, it does. Why the sour face?

> 
> >>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
>