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