Hey,
Forgot to comment on the patch at the same time as the log comments /o\
Looks good to me save for a few nits,
On Wed, Jul 29, 2015 at 05:21:15PM -0400, Sandy Stutsman wrote:
> If the client hasn't been updated to handle non multi-hat monitors_config,
> vd_agent will won't issue the MONITORS_CONFIG driver escape.
>
> This addresses: rhbz#1248196
> ---
> vdagent/desktop_layout.cpp | 4 ++++
> vdagent/desktop_layout.h | 5 +++--
> vdagent/vdagent.cpp | 4 ++++
> 3 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/vdagent/desktop_layout.cpp b/vdagent/desktop_layout.cpp
> index 7a34f7f..db9047f 100644
> --- a/vdagent/desktop_layout.cpp
> +++ b/vdagent/desktop_layout.cpp
> @@ -35,6 +35,7 @@ void DisplayMode::set_res(DWORD width, DWORD height, DWORD depth)
> DesktopLayout::DesktopLayout()
> : _total_width (0)
> , _total_height (0)
> + , _configurable(0)
I'd use something a bit more explicit, maybe
'send_monitors_position' ? Fine with me if you want to stick with
'configurable'. Using 'false' would be better than '0' I think.
> {
> MUTEX_INIT(_mutex);
> get_displays();
> @@ -365,6 +366,9 @@ bool DesktopLayout::update_monitor_config(LPCTSTR dev_name, DisplayMode* mode)
> if (!mode || !mode->get_attached())
> return false;
>
> + //Don't configure monitors unless the client supports it
> + if(!_configurable) return FALSE;
> +
> HDC hdc = CreateDC(dev_name, NULL, NULL, NULL);
>
> memset(&monitor_config, 0, sizeof(monitor_config));
> diff --git a/vdagent/desktop_layout.h b/vdagent/desktop_layout.h
> index 0e310e3..a2d32d4 100644
> --- a/vdagent/desktop_layout.h
> +++ b/vdagent/desktop_layout.h
> @@ -73,22 +73,23 @@ public:
> size_t get_display_count() { return _displays.size();}
> DWORD get_total_width() { return _total_width;}
> DWORD get_total_height() { return _total_height;}
> -
> + void set_configurable(bool flag) { _configurable = flag; }
> private:
> void clean_displays();
> void normalize_displays_pos();
> DisplayMode * get_primary_display();
> + bool update_monitor_config(LPCTSTR dev_name, DisplayMode* mode);
> 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,
> LONG normal_x, LONG normal_y, bool set_pos);
> - static bool update_monitor_config(LPCTSTR dev_name, DisplayMode* mode);
> private:
> mutex_t _mutex;
> Displays _displays;
> DWORD _total_width;
> DWORD _total_height;
> + bool _configurable;
> };
>
> #endif
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index efce981..d8c6bd2 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -835,6 +835,10 @@ bool VDAgent::handle_announce_capabilities(VDAgentAnnounceCapabilities* announce
> _client_caps_size = caps_size;
> }
> memcpy(_client_caps, announce_capabilities->caps, sizeof(_client_caps[0]) * caps_size);
> +
> + _desktop_layout->set_configurable(VD_AGENT_HAS_CAPABILITY(_client_caps,
> + _client_caps_size,
> + VD_AGENT_CAP_MONITORS_CONFIG_POSITION));
In my opinion, this would be more readable as
if (VD_AGENT_HAS_CAPABILITY(_client_caps, _client_caps_size, VD_AGENT_CAP_MONITORS_CONFIG_POSITION))
_desktop_layout->set_configurable(true);
Looks good otherwise,
Christophe
Attachment:
pgp4oB7rgmiGw.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel