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

[Arnon Gilboa]

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? :/

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
        

_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel