[Spice-devel] Fwd: [vdagent-win PATCH] Use Windows coordinates with primary monitor at (0, 0) when making WinAPI calls.

[Uri Lublin]

On 03/21/2015 01:06 AM, Sandy Stutsman wrote:
> From: Sandy Stutsman <sstutsma at redhat.com>
>

Hi Sandy,

Looks good.
I have some comments below.
I'll try to test it tomorrow.

> Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1174129.  Windows
> sets the monitor positioned at (0,0) to be the primary monitor.
> While VDAgent normalizes the monitor positions so that there no negative
> coordinates.  If the normalized positions passed to the Windows APIs,
> the leftmost monitor will always be set to primary. We need to keep
> track of the primary monitor and its normalized position to be able to
> adjust the monitor coordinates that are passed to Windows.
> ---
>   vdagent/desktop_layout.cpp | 34 ++++++++++++++++++++++++++++++----
>   vdagent/desktop_layout.h   |  8 +++++---
>   2 files changed, 35 insertions(+), 7 deletions(-)
>   mode change 100644 => 100755 vdagent/desktop_layout.cpp
>
> diff --git a/vdagent/desktop_layout.cpp b/vdagent/desktop_layout.cpp
> old mode 100644
> new mode 100755
> index 7850e6d..1f82f2d
> --- a/vdagent/desktop_layout.cpp
> +++ b/vdagent/desktop_layout.cpp
> @@ -90,6 +90,23 @@ void DesktopLayout::get_displays()
>       unlock();
>   }
>
> +DisplayMode * DesktopLayout::get_primary_display()
> +{
> +    DisplayMode * mode;
> +    boolean bFound(false);

bFound is not used.

> +    for (unsigned int i = 0; i < get_display_count(); i++)
> +    {
> +        mode = _displays.at(i);
> +        if (!mode)
> +            return mode;

Maybe better to "continue" here and check all displays.

> +        if (mode->is_primary())
> +            return mode;
> +    }
> +
> +    //Default to the first display
> +    return _displays.at(0);

And here we can return NULL, if primary is not found.
(Just for consistency with "return mode" above if
  that line is not changed. Returning the first display
  should work)


> +}
> +
>   void DesktopLayout::set_displays()
>   {
>       DISPLAY_DEVICE dev_info;
> @@ -107,6 +124,12 @@ void DesktopLayout::set_displays()
>       dev_info.cb = sizeof(dev_info);
>       ZeroMemory(&dev_mode, sizeof(dev_mode));
>       dev_mode.dmSize = sizeof(dev_mode);
> +
> +    //Get the normalized position of the primary monitor
> +    DisplayMode * primary(get_primary_display());
> +    LONG normal_x = primary ? primary->get_pos_x() : 0;
> +    LONG normal_y = primary ? primary->get_pos_y() : 0;
> +
>       while (EnumDisplayDevices(NULL, dev_id, &dev_info, 0)) {
>           dev_id++;
>           if (dev_info.StateFlags & DISPLAY_DEVICE_MIRRORING_DRIVER) {
> @@ -121,7 +144,8 @@ void DesktopLayout::set_displays()
>               vd_printf("display_id %lu out of range, #displays %zu" , display_id, _displays.size());
>               break;
>           }
> -        if (!init_dev_mode(dev_info.DeviceName, &dev_mode, _displays.at(display_id), true)) {
> +        if (!init_dev_mode(dev_info.DeviceName, &dev_mode, _displays.at(display_id),
> +            normal_x, normal_y, true)) {
>               vd_printf("No suitable mode found for display %S", dev_info.DeviceName);
>               break;
>           }
> @@ -240,7 +264,7 @@ bool DesktopLayout::get_qxl_device_id(WCHAR* device_key, DWORD* device_id)
>   }
>
>   bool DesktopLayout::init_dev_mode(LPCTSTR dev_name, DEVMODE* dev_mode, DisplayMode* mode,
> -                                  bool set_pos)
> +                                  LONG normal_x, LONG normal_y, bool set_pos)
>   {
>       DWORD closest_diff = -1;
>       DWORD best = -1;
> @@ -320,8 +344,10 @@ bool DesktopLayout::init_dev_mode(LPCTSTR dev_name, DEVMODE* dev_mode, DisplayMo
>       }
>       dev_mode->dmFields = DM_BITSPERPEL | DM_PELSWIDTH | DM_PELSHEIGHT;
>       if (set_pos) {
> -        dev_mode->dmPosition.x = mode->_pos_x;
> -        dev_mode->dmPosition.y = mode->_pos_y;
> +
> +        //Convert the position so that the primary is always at (0,0)
> +        dev_mode->dmPosition.x = mode->_pos_x - normal_x;
> +        dev_mode->dmPosition.y = mode->_pos_y - normal_y;
>           dev_mode->dmFields |= DM_POSITION;
>       }
>
> diff --git a/vdagent/desktop_layout.h b/vdagent/desktop_layout.h
> index c43ff03..3abbe58 100644
> --- a/vdagent/desktop_layout.h
> +++ b/vdagent/desktop_layout.h
> @@ -31,6 +31,7 @@ public:
>           , _depth (depth)
>           , _attached (attached)
>       {
> +        _primary =  (pos_x == 0 && pos_y == 0 && attached);
>       }
>
>       LONG get_pos_x() { return _pos_x;}
> @@ -44,7 +45,7 @@ public:
>       void set_res(DWORD width, DWORD height, DWORD depth);
>       void set_depth(DWORD depth) { _depth = depth;}
>       void set_attached(bool attached) { _attached = attached;}
> -
> +    bool is_primary(){ return _primary; }

nitpick: add a space    ^ between {} and (

>   private:
>       LONG _pos_x;
>       LONG _pos_y;
> @@ -52,7 +53,7 @@ private:
>       DWORD _height;
>       DWORD _depth;
>       bool _attached;
> -
> +    bool _primary;
>       friend class DesktopLayout;
>   };
>
> @@ -74,10 +75,11 @@ public:
>   private:
>       void clean_displays();
>       void normalize_displays_pos();
> +    DisplayMode * get_primary_display();
>       static bool consistent_displays();
>       static bool is_attached(LPCTSTR dev_name);
>       static bool get_qxl_device_id(WCHAR* device_key, DWORD* device_id);
> -    static bool init_dev_mode(LPCTSTR dev_name, DEVMODE* dev_mode, DisplayMode* mode, bool set_pos);
> +    static bool init_dev_mode(LPCTSTR dev_name, DEVMODE* dev_mode, DisplayMode* mode, LONG normal_x, LONG normal_y, bool set_pos);
nitpick: This line is pretty long.

Thanks,
     Uri.