[Spice-devel] [RFC PATCH spice-gtk v2 13/20] Rework the handling of monitors_config
Marc-André Lureau
marcandre.lureau at gmail.com
Fri Aug 17 14:27:26 UTC 2018
Hi
On Thu, Aug 16, 2018 at 6:27 PM Lukáš Hrázký <lhrazky at redhat.com> wrote:
>
> Before this commit, the code relied on
> spice_main_channel_update_display() called from the client application
> to add items to the list of monitor_configs to be sent to the server
> (stored in SpiceMainChannelPrivate.display). One of the disadvantages
> this approach has is that all data that you need in the list have to go
> through the client application. Thus adding any attribute to a
> monitor_config requires an API change.
What additional attribute do you need?
>
> This commit changes this by creating and maintaining our own list of
> monitor_configs under the session object. The methods that are exposed
> to the client app no longer create new items in this list.
What is the advantge of moving the config and the API to the session?
It "looks" better, but it doesn't seem necessary.
Before, we had a fixed array of MAX_DISPLAY size, and we could just
index into it. Now it's a dynamic array. Why?
>
> Now the spice_main_channel_update_display{,_enabled}() functions only
> update existing items in the spice-gtk-maintained list. The identifier
> for these functions was unfortunately the index in the array on the
> spice-gtk side and channel_id + monitor_id in e.g. virt-viewer. This
> worked under the assumption that there is either only one display
> channel or multiple display channels each with only one monitor.
> Presumably other clients can use whatever they like as the id, but doing
> something else most probably wouldn't work.
>
> So, we use the channel_id + monitor_id in the now-deprecated
> spice_main_channel_update_display{,_enabled}() to match against their
> single 'id' argument. This works for as long as the assumption mentioned
> above holds true, which doesn't for the guest streaming use case
> anymore. So for that use case a new client is needed using the new
> consistent API.
Hmm, streaming will require display channel ID != 0 & monitor ID ? Why?
>
> An unsorted list is used and the items are looked up by simple
> iteration. No more than 5 items are really expected in the list. The
> order of the items in the list is determined by their creation timing
> and therefore is undeterministic. The order should no longer matter for
> anything though.
>
> Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> ---
> src/channel-display.c | 23 +++++-
> src/channel-main.c | 156 +++++++++++++++++++++++++--------------
> src/channel-main.h | 3 +
> src/map-file | 4 +
> src/spice-glib-sym-file | 4 +
> src/spice-session-priv.h | 10 +++
> src/spice-session.c | 152 ++++++++++++++++++++++++++++++++++++++
> src/spice-session.h | 31 ++++++++
> src/spice-widget.c | 7 +-
> tools/spicy.c | 15 ++--
> 10 files changed, 334 insertions(+), 71 deletions(-)
>
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 7c663cb..868aa83 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -1049,8 +1049,10 @@ static void spice_display_channel_up(SpiceChannel *channel)
> out->marshallers->msgc_display_init(out->marshaller, &init);
> spice_msg_out_send_internal(out);
>
> - /* notify of existence of this monitor */
> - g_coroutine_object_notify(G_OBJECT(channel), "monitors");
> + // if we don't have MONITORS_CONFIG_CAPABILITY, notify of existence of this monitor
In general we use C /* */ comments
> + if (!spice_channel_test_capability(channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG)) {
> + g_coroutine_object_notify(G_OBJECT(channel), "monitors");
> + }
>
This looks like an unrelated change, please split it.
> if (preferred_compression != SPICE_IMAGE_COMPRESSION_INVALID) {
> spice_display_channel_change_preferred_compression(channel, preferred_compression);
> @@ -1869,6 +1871,7 @@ static void display_handle_surface_destroy(SpiceChannel *channel, SpiceMsgIn *in
> /* coroutine context */
> static void display_handle_monitors_config(SpiceChannel *channel, SpiceMsgIn *in)
> {
> + SpiceSession *session = spice_channel_get_session(channel);
> SpiceMsgDisplayMonitorsConfig *config = spice_msg_in_parsed(in);
> SpiceDisplayChannelPrivate *c = SPICE_DISPLAY_CHANNEL(channel)->priv;
> guint i;
> @@ -1907,6 +1910,22 @@ static void display_handle_monitors_config(SpiceChannel *channel, SpiceMsgIn *in
> mc->y = head->y;
> mc->width = head->width;
> mc->height = head->height;
> +
> + if (spice_session_get_monitor_config(session, channel->priv->channel_id, i)) {
> + spice_session_update_monitor_config_dimensions(session, channel->priv->channel_id, i,
> + head->x, head->y, head->width, head->height, FALSE);
> + } else {
> + spice_session_add_monitor_config(session, channel->priv->channel_id, i,
> + head->x, head->y, head->width, head->height);
> + }
> + }
> +
> + // On top of the above, create disabled configs for all monitors up to max_allowed
> + // if they don't exist.
> + for (i = config->count; i < config->max_allowed; i++) {
> + if (!spice_session_get_monitor_config(session, channel->priv->channel_id, i)) {
> + spice_session_add_monitor_config(session, channel->priv->channel_id, i, 0, 0, 0, 0);
> + }
> }
>
> g_coroutine_object_notify(G_OBJECT(channel), "monitors");
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 4c6bc70..a23bc64 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -52,20 +52,6 @@
>
> typedef struct spice_migrate spice_migrate;
>
> -typedef enum {
> - DISPLAY_UNDEFINED,
> - DISPLAY_DISABLED,
> - DISPLAY_ENABLED,
> -} SpiceDisplayState;
> -
> -typedef struct {
> - int x;
> - int y;
> - int width;
> - int height;
> - SpiceDisplayState display_state;
> -} SpiceDisplayConfig;
> -
> typedef struct {
> GHashTable *xfer_task;
> SpiceMainChannel *channel;
> @@ -102,7 +88,6 @@ struct _SpiceMainChannelPrivate {
> guint agent_msg_pos;
> uint8_t agent_msg_size;
> uint32_t agent_caps[VD_AGENT_CAPS_SIZE];
> - SpiceDisplayConfig display[MAX_DISPLAY];
> gint timer_id;
> GQueue *agent_msg_queue;
> GHashTable *file_xfer_tasks;
> @@ -1111,13 +1096,17 @@ gboolean spice_main_channel_send_monitor_config(SpiceMainChannel *channel)
> c = channel->priv;
> g_return_val_if_fail(c->agent_connected, FALSE);
>
> + GArray *monitor_configs = spice_session_get_monitor_configs(
> + spice_channel_get_session(SPICE_CHANNEL(channel)));
> +
> if (spice_main_channel_agent_test_capability(channel, VD_AGENT_CAP_SPARSE_MONITORS_CONFIG)) {
> - monitors = SPICE_N_ELEMENTS(c->display);
> + monitors = monitor_configs->len;
> } else {
> monitors = 0;
> - for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> - if (c->display[i].display_state == DISPLAY_ENABLED)
> + for (i = 0; i < monitor_configs->len; i++) {
> + if (g_array_index(monitor_configs, SpiceMonitorConfig, i).display_state == DISPLAY_ENABLED) {
> monitors += 1;
> + }
> }
> }
>
> @@ -1131,18 +1120,19 @@ gboolean spice_main_channel_send_monitor_config(SpiceMainChannel *channel)
>
> CHANNEL_DEBUG(channel, "sending new monitors config to guest");
> j = 0;
> - for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> - if (c->display[i].display_state != DISPLAY_ENABLED) {
> + for (i = 0; i < monitor_configs->len; i++) {
> + SpiceMonitorConfig *mc = &g_array_index(monitor_configs, SpiceMonitorConfig, i);
> + if (mc->display_state != DISPLAY_ENABLED) {
> if (spice_main_channel_agent_test_capability(channel,
> VD_AGENT_CAP_SPARSE_MONITORS_CONFIG))
> j++;
> continue;
> }
> mon->monitors[j].depth = c->display_color_depth ? c->display_color_depth : 32;
> - mon->monitors[j].width = c->display[i].width;
> - mon->monitors[j].height = c->display[i].height;
> - mon->monitors[j].x = c->display[i].x;
> - mon->monitors[j].y = c->display[i].y;
> + mon->monitors[j].width = mc->width;
> + mon->monitors[j].height = mc->height;
> + mon->monitors[j].x = mc->x;
> + mon->monitors[j].y = mc->y;
> CHANNEL_DEBUG(channel, "monitor #%d: %ux%u+%d+%d @ %u bpp", j,
> mon->monitors[j].width, mon->monitors[j].height,
> mon->monitors[j].x, mon->monitors[j].y,
> @@ -1477,15 +1467,18 @@ static void agent_clipboard_release(SpiceMainChannel *channel, guint selection)
>
> static gboolean any_display_has_dimensions(SpiceMainChannel *channel)
> {
> - SpiceMainChannelPrivate *c;
> guint i;
>
> g_return_val_if_fail(SPICE_IS_MAIN_CHANNEL(channel), FALSE);
> - c = channel->priv;
>
> - for (i = 0; i < MAX_DISPLAY; i++) {
> - if (c->display[i].width > 0 && c->display[i].height > 0)
> + GArray *monitor_configs = spice_session_get_monitor_configs(
> + spice_channel_get_session(SPICE_CHANNEL(channel)));
> +
> + for (i = 0; i < monitor_configs->len; i++) {
> + SpiceMonitorConfig *mc = &g_array_index(monitor_configs, SpiceMonitorConfig, i);
> + if (mc->width > 0 && mc->height > 0) {
> return TRUE;
> + }
> }
>
> return FALSE;
> @@ -1509,12 +1502,19 @@ static gboolean timer_set_display(gpointer data)
> }
>
> session = spice_channel_get_session(SPICE_CHANNEL(channel));
> + GArray *monitor_configs = spice_session_get_monitor_configs(session);
>
> if (!spice_main_channel_agent_test_capability(channel, VD_AGENT_CAP_SPARSE_MONITORS_CONFIG)) {
> /* ensure we have an explicit monitor configuration at least for
> number of display channels */
> - for (i = 0; i < spice_session_get_n_display_channels(session); i++)
> - if (c->display[i].display_state == DISPLAY_UNDEFINED) {
> +
> + guint n_display_channels = spice_session_get_n_display_channels(session);
> + if (n_display_channels < monitor_configs->len) {
> + SPICE_DEBUG("Not sending monitors config, missing monitors");
> + return FALSE;
> + }
> + for (i = 0; i < n_display_channels; i++)
> + if (g_array_index(monitor_configs, SpiceMonitorConfig, i).display_state == DISPLAY_UNDEFINED) {
> SPICE_DEBUG("Not sending monitors config, missing monitors");
> return FALSE;
> }
> @@ -1525,7 +1525,7 @@ static gboolean timer_set_display(gpointer data)
> }
>
> /* any context */
> -static void update_display_timer(SpiceMainChannel *channel, guint seconds)
> +void spice_main_channel_update_display_timer(SpiceMainChannel *channel, guint seconds)
> {
> SpiceMainChannelPrivate *c = channel->priv;
>
> @@ -2010,7 +2010,7 @@ static void main_agent_handle_msg(SpiceChannel *channel,
> }
> c->agent_caps_received = true;
> g_coroutine_signal_emit(self, signals[SPICE_MAIN_AGENT_UPDATE], 0);
> - update_display_timer(SPICE_MAIN_CHANNEL(channel), 0);
> + spice_main_channel_update_display_timer(SPICE_MAIN_CHANNEL(channel), 0);
>
> if (caps->request)
> agent_announce_caps(self);
> @@ -2626,7 +2626,7 @@ gboolean spice_main_channel_agent_test_capability(SpiceMainChannel *channel, gui
> /**
> * spice_main_update_display:
> * @channel: a #SpiceMainChannel
> - * @id: display ID
> + * @id: the display ID - assumed to be channel_id + monitor_id
Well, not exactly. display ID is rather: either monitor_id OR
channel_id... We don't support channel_id + monitor_id...
> * @x: x position
> * @y: y position
> * @width: display width
> @@ -2652,7 +2652,7 @@ void spice_main_update_display(SpiceMainChannel *channel, int id,
> /**
> * spice_main_channel_update_display:
> * @channel: a #SpiceMainChannel
> - * @id: display ID
> + * @id: the display ID - assumed to be channel_id + monitor_id
> * @x: x position
> * @y: y position
> * @width: display width
> @@ -2671,8 +2671,6 @@ void spice_main_update_display(SpiceMainChannel *channel, int id,
> void spice_main_channel_update_display(SpiceMainChannel *channel, int id, int x, int y, int width,
> int height, gboolean update)
> {
> - SpiceMainChannelPrivate *c;
> -
> g_return_if_fail(channel != NULL);
> g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
> g_return_if_fail(x >= 0);
> @@ -2680,28 +2678,48 @@ void spice_main_channel_update_display(SpiceMainChannel *channel, int id, int x,
> g_return_if_fail(width >= 0);
> g_return_if_fail(height >= 0);
>
> - c = SPICE_MAIN_CHANNEL(channel)->priv;
> + GArray *monitor_configs = spice_session_get_monitor_configs(
> + spice_channel_get_session(SPICE_CHANNEL(channel)));
>
> - g_return_if_fail(id < SPICE_N_ELEMENTS(c->display));
> + bool found = false;
> + SpiceMonitorConfig *mc = NULL;
> + for (size_t i = 0; i < monitor_configs->len; ++i) {
> + mc = &g_array_index(monitor_configs, SpiceMonitorConfig, i);
> + if (mc->channel_id + mc->monitor_id == id) {
> + found = true;
> + break;
> + }
> + }
>
> - SpiceDisplayConfig display = {
> - .x = x, .y = y, .width = width, .height = height,
> - .display_state = c->display[id].display_state
> - };
> + if (!found) {
> + g_warning("Display ID %d not found while trying to update the display (monitor config).", id);
> + return;
> + }
>
> - if (memcmp(&display, &c->display[id], sizeof(SpiceDisplayConfig)) == 0)
> + if (mc->x == x &&
> + mc->y == y &&
> + mc->width == width &&
> + mc->height == height) {
> return;
> + }
> +
> + CHANNEL_DEBUG(channel,
> + "Update display (monitor config) id: %d (channel_id: %u, monitor_id: %u), +%d+%d-%dx%d",
> + id, mc->channel_id, mc->monitor_id, x, y, width, height);
>
> - c->display[id] = display;
> + mc->x = x;
> + mc->y = y;
> + mc->width = width;
> + mc->height = height;
>
> if (update)
> - update_display_timer(channel, 1);
> + spice_main_channel_update_display_timer(channel, 1);
> }
>
> /**
> * spice_main_set_display:
> * @channel: a #SpiceMainChannel
> - * @id: display ID
> + * @id: the display ID - assumed to be channel_id + monitor_id
> * @x: x position
> * @y: y position
> * @width: display width
> @@ -2943,7 +2961,7 @@ void spice_main_channel_clipboard_selection_request(SpiceMainChannel *channel, g
> /**
> * spice_main_update_display_enabled:
> * @channel: a #SpiceMainChannel
> - * @id: display ID (if -1: set all displays)
> + * @id: the display ID - assumed to be channel_id + monitor_id
> * @enabled: wether display @id is enabled
> * @update: if %TRUE, update guest display state after 1sec.
> *
> @@ -2968,7 +2986,7 @@ void spice_main_update_display_enabled(SpiceMainChannel *channel, int id, gboole
> /**
> * spice_main_channel_update_display_enabled:
> * @channel: a #SpiceMainChannel
> - * @id: display ID (if -1: set all displays)
> + * @id: the display ID - assumed to be channel_id + monitor_id
> * @enabled: wether display @id is enabled
> * @update: if %TRUE, update guest display state after 1sec.
> *
> @@ -2986,33 +3004,57 @@ void spice_main_update_display_enabled(SpiceMainChannel *channel, int id, gboole
> void spice_main_channel_update_display_enabled(SpiceMainChannel *channel, int id, gboolean enabled,
> gboolean update)
> {
> - SpiceDisplayState display_state = enabled ? DISPLAY_ENABLED : DISPLAY_DISABLED;
> g_return_if_fail(channel != NULL);
> g_return_if_fail(SPICE_IS_MAIN_CHANNEL(channel));
> g_return_if_fail(id >= -1);
>
> - SpiceMainChannelPrivate *c = channel->priv;
> + GArray *monitor_configs = spice_session_get_monitor_configs(
> + spice_channel_get_session(SPICE_CHANNEL(channel)));
> +
> + SpiceDisplayState display_state = enabled ? DISPLAY_ENABLED : DISPLAY_DISABLED;
>
> if (id == -1) {
> + SPICE_DEBUG("Updating all monitor configs' state to %s", enabled ? "enabled" : "disabled");
> +
> gint i;
> - for (i = 0; i < G_N_ELEMENTS(c->display); i++) {
> - c->display[i].display_state = display_state;
> + for (i = 0; i < monitor_configs->len; i++) {
> + g_array_index(monitor_configs, SpiceMonitorConfig, i).display_state = display_state;
> }
> } else {
> - g_return_if_fail(id < G_N_ELEMENTS(c->display));
> - if (c->display[id].display_state == display_state)
> + bool found = false;
> + SpiceMonitorConfig *mc = NULL;
> + for (size_t i = 0; i < monitor_configs->len; ++i) {
> + mc = &g_array_index(monitor_configs, SpiceMonitorConfig, i);
> + if (mc->channel_id + mc->monitor_id == id) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found) {
> + g_warning("Display ID %d not found while trying to %s the display (monitor config).",
> + id, enabled ? "enable" : "disable");
> + return;
> + }
> +
> + if (mc->display_state == display_state) {
> return;
> - c->display[id].display_state = display_state;
> + }
> +
> + SPICE_DEBUG("Updating monitor config id: %d (channel_id: %u, monitor_id: %u) state to %s",
> + id, mc->channel_id, mc->monitor_id, enabled ? "enabled" : "disabled");
> +
> + mc->display_state = display_state;
> }
>
> if (update)
> - update_display_timer(channel, 1);
> + spice_main_channel_update_display_timer(channel, 1);
> }
>
> /**
> * spice_main_set_display_enabled:
> * @channel: a #SpiceMainChannel
> - * @id: display ID (if -1: set all displays)
> + * @id: the display ID - assumed to be channel_id + monitor_id
> * @enabled: wether display @id is enabled
> *
> * When sending monitor configuration to agent guest, don't set
> diff --git a/src/channel-main.h b/src/channel-main.h
> index fd7a3bb..0495bb2 100644
> --- a/src/channel-main.h
> +++ b/src/channel-main.h
> @@ -101,6 +101,9 @@ gboolean spice_main_channel_file_copy_finish(SpiceMainChannel *channel,
>
> void spice_main_channel_request_mouse_mode(SpiceMainChannel *channel, int mode);
>
> +// TODO had to add this to the interface to access it from spice-session, what to do with it?
> +void spice_main_channel_update_display_timer(SpiceMainChannel *channel, guint seconds);
> +
> #ifndef SPICE_DISABLE_DEPRECATED
> G_DEPRECATED_FOR(spice_main_channel_clipboard_selection_grab)
> void spice_main_clipboard_grab(SpiceMainChannel *channel, guint32 *types, int ntypes);
> diff --git a/src/map-file b/src/map-file
> index cdb81c3..0e4c375 100644
> --- a/src/map-file
> +++ b/src/map-file
> @@ -122,6 +122,8 @@ spice_record_channel_send_data;
> spice_record_send_data;
> spice_session_connect;
> spice_session_disconnect;
> +spice_display_state_get_type;
> +spice_session_get_monitor_config;
> spice_session_get_channels;
> spice_session_get_proxy_uri;
> spice_session_get_read_only;
> @@ -131,6 +133,8 @@ spice_session_is_for_migration;
> spice_session_migration_get_type;
> spice_session_new;
> spice_session_open_fd;
> +spice_session_update_monitor_config_dimensions;
> +spice_session_update_monitor_config_enabled;
> spice_session_verify_get_type;
> spice_set_session_option;
> spice_smartcard_channel_get_type;
> diff --git a/src/spice-glib-sym-file b/src/spice-glib-sym-file
> index b19844c..897a303 100644
> --- a/src/spice-glib-sym-file
> +++ b/src/spice-glib-sym-file
> @@ -101,6 +101,8 @@ spice_record_channel_send_data
> spice_record_send_data
> spice_session_connect
> spice_session_disconnect
> +spice_display_state_get_type
> +spice_session_get_monitor_config
> spice_session_get_channels
> spice_session_get_proxy_uri
> spice_session_get_read_only
> @@ -110,6 +112,8 @@ spice_session_is_for_migration
> spice_session_migration_get_type
> spice_session_new
> spice_session_open_fd
> +spice_session_update_monitor_config_dimensions
> +spice_session_update_monitor_config_enabled
> spice_session_verify_get_type
> spice_set_session_option
> spice_smartcard_channel_get_type
> diff --git a/src/spice-session-priv.h b/src/spice-session-priv.h
> index 03005aa..77cb8c6 100644
> --- a/src/spice-session-priv.h
> +++ b/src/spice-session-priv.h
> @@ -100,6 +100,16 @@ void spice_session_set_main_channel(SpiceSession *session, SpiceChannel *channel
> gboolean spice_session_set_migration_session(SpiceSession *session, SpiceSession *mig_session);
> SpiceAudio *spice_audio_get(SpiceSession *session, GMainContext *context);
> const gchar* spice_audio_data_mode_to_string(gint mode);
> +
> +GArray *spice_session_get_monitor_configs(SpiceSession *session);
> +
> +SpiceMonitorConfig *spice_session_get_monitor_config_mutable(SpiceSession *session,
> + uint32_t channel_id,
> + uint32_t monitor_id);
> +
> +void spice_session_add_monitor_config(SpiceSession *session, uint32_t channel_id, uint32_t display_id,
> + uint32_t x, uint32_t y, uint32_t width, uint32_t height);
> +
> G_END_DECLS
>
> #endif /* __SPICE_CLIENT_SESSION_PRIV_H__ */
> diff --git a/src/spice-session.c b/src/spice-session.c
> index b1aeb84..29a4c69 100644
> --- a/src/spice-session.c
> +++ b/src/spice-session.c
> @@ -90,6 +90,11 @@ struct _SpiceSessionPrivate {
> GStrv secure_channels;
> gint color_depth;
>
> + /* A list of monitor configs for all channels and monitors. It is filled up
> + * with monitors_config messages as they arrive on display channels.
> + */
> + GArray *monitor_configs;
> +
> int connection_id;
> int protocol;
> SpiceChannel *cmain; /* weak reference */
> @@ -288,6 +293,8 @@ static void spice_session_init(SpiceSession *session)
> s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
> s->glz_window = glz_decoder_window_new();
> update_proxy(session, NULL);
> +
> + s->monitor_configs = g_array_new(FALSE, TRUE, sizeof(SpiceMonitorConfig));
> }
>
> static void
> @@ -340,6 +347,8 @@ spice_session_dispose(GObject *gobject)
> g_clear_object(&s->proxy);
> g_clear_object(&s->webdav);
>
> + g_array_free(s->monitor_configs, TRUE);
> +
> /* Chain up to the parent class */
> if (G_OBJECT_CLASS(spice_session_parent_class)->dispose)
> G_OBJECT_CLASS(spice_session_parent_class)->dispose(gobject);
> @@ -2823,6 +2832,149 @@ gboolean spice_session_is_for_migration(SpiceSession *session)
> return session->priv->for_migration;
> }
>
> +void spice_session_add_monitor_config(SpiceSession *session, uint32_t channel_id, uint32_t monitor_id,
> + uint32_t x, uint32_t y, uint32_t width, uint32_t height)
> +{
> + GArray *monitor_configs = spice_session_get_monitor_configs(session);
> + g_assert(monitor_configs != NULL);
> +
> + SpiceMonitorConfig mc = {
> + .channel_id = channel_id,
> + .monitor_id = monitor_id,
> + .x = x,
> + .y = y,
> + .width = width,
> + .height = height,
> + .display_state = DISPLAY_DISABLED
> + };
> +
> + SPICE_DEBUG("Adding new monitor_config channel_id: %u, monitor_id: %u, +%u+%u:%ux%u",
> + channel_id, monitor_id, x, y, width, height);
> +
> + g_array_append_val(monitor_configs, mc);
> +}
> +
> +GArray *spice_session_get_monitor_configs(SpiceSession *session)
> +{
> + g_assert(session != NULL);
> +
> + return session->priv->monitor_configs;
> +}
> +
> +SpiceMonitorConfig *spice_session_get_monitor_config_mutable(SpiceSession *session,
> + uint32_t channel_id,
> + uint32_t monitor_id)
> +{
> + GArray *monitor_configs = spice_session_get_monitor_configs(session);
> + g_assert(monitor_configs != NULL);
> +
> + for (size_t i = 0; i < monitor_configs->len; ++i) {
> + SpiceMonitorConfig *d = &g_array_index(monitor_configs, SpiceMonitorConfig, i);
> + if (d->channel_id == channel_id && d->monitor_id == monitor_id) {
> + return d;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * spice_session_get_monitor_config:
> + * @session: a #SpiceSession
> + * @channel_id: a channel ID
> + * @monitor_id: a monitor ID
> + *
> + * Returns a #SpiceMonitorConfig identified by the @channel_id and @monitor_id.
> + *
> + * Returns: a pointer to SpiceMonitorConfig if found, %NULL otherwise
Why do you need that new API and expose the structure? I suggest to
make it a seperate commit with rationale.
> + *
> + * Since: 0.36
> + **/
> +const SpiceMonitorConfig *spice_session_get_monitor_config(SpiceSession *session,
> + uint32_t channel_id,
> + uint32_t monitor_id)
> +{
> + return spice_session_get_monitor_config_mutable(session, channel_id, monitor_id);
> +}
> +
> +/**
> + * spice_session_update_monitor_config_dimensions:
> + * @session: a #SpiceSession
> + * @channel_id: a channel ID
> + * @monitor_id: a monitor ID
> + * @x: X coordinate of the monitor
> + * @y: Y coordinate of the monitor
> + * @width: width of the monitor
> + * @height: height of the monitor
> + *
> + * Sets the dimensions (@x, @y, @width, @height) of a #SpiceMonitorConfig
> + * identified with @channel_id and @monitor_id.
> + *
> + * Since: 0.36
> + **/
> +void spice_session_update_monitor_config_dimensions(SpiceSession *session,
> + uint32_t channel_id, uint32_t monitor_id,
> + uint32_t x, uint32_t y,
> + uint32_t width, uint32_t height, gboolean update)
> +{
> + SpiceMonitorConfig *mc = spice_session_get_monitor_config_mutable(session, channel_id, monitor_id);
> +
> + g_return_if_fail(mc != NULL);
> +
> + if (mc->x == x &&
> + mc->y == y &&
> + mc->width == width &&
> + mc->height == height) {
> + return;
> + }
> +
> + SPICE_DEBUG("Updating monitor config channel_id: %u, monitor_id: %u, +%u+%u-%ux%u",
> + channel_id, monitor_id, x, y, width, height);
> +
> + mc->x = x;
> + mc->y = y;
> + mc->width = width;
> + mc->height = height;
> +
> + if (update) {
> + spice_main_channel_update_display_timer((SpiceMainChannel *) session->priv->cmain, 1);
> + }
> +}
> +
> +/**
> + * spice_session_update_monitor_config_enabled:
> + * @session: a #SpiceSession
> + * @channel_id: a channel ID
> + * @monitor_id: a monitor ID
> + * @enabled: The enabled flag to set
> + *
> + * Sets the @enabled flag of a #SpiceMonitorConfig
> + * identified with @channel_id and @monitor_id.
> + *
> + * Since: 0.36
> + **/
> +void spice_session_update_monitor_config_enabled(SpiceSession *session,
> + uint32_t channel_id, uint32_t monitor_id,
> + gboolean enabled, gboolean update)
> +{
> + SpiceMonitorConfig *mc = spice_session_get_monitor_config_mutable(session, channel_id, monitor_id);
> +
> + g_return_if_fail(mc != NULL);
> +
> + if (mc->display_state == (enabled ? DISPLAY_ENABLED : DISPLAY_DISABLED)) {
> + return;
> + }
> +
> + SPICE_DEBUG("%s monitor config channel_id: %u, monitor_id: %u",
> + enabled ? "Enabling" : "Disabling", channel_id, monitor_id);
> +
> + mc->display_state = enabled ? DISPLAY_ENABLED : DISPLAY_DISABLED;
> +
> + if (update) {
> + spice_main_channel_update_display_timer((SpiceMainChannel *) session->priv->cmain, 1);
> + }
> +}
> +
> G_GNUC_INTERNAL
> void spice_session_set_main_channel(SpiceSession *session, SpiceChannel *channel)
> {
> diff --git a/src/spice-session.h b/src/spice-session.h
> index cfe02b1..16523bb 100644
> --- a/src/spice-session.h
> +++ b/src/spice-session.h
> @@ -23,6 +23,8 @@
> #endif
>
> #include <glib-object.h>
> +#include <stdint.h>
> +
> #include "spice-types.h"
> #include "spice-uri.h"
> #include "spice-glib-enums.h"
> @@ -103,6 +105,22 @@ struct _SpiceSessionClass
> gchar _spice_reserved[SPICE_RESERVED_PADDING];
> };
>
> +typedef enum {
> + DISPLAY_UNDEFINED,
> + DISPLAY_DISABLED,
> + DISPLAY_ENABLED,
> +} SpiceDisplayState;
> +
> +typedef struct {
> + uint32_t channel_id;
> + uint32_t monitor_id;
> + uint32_t x;
> + uint32_t y;
> + uint32_t width;
> + uint32_t height;
> + SpiceDisplayState display_state;
> +} SpiceMonitorConfig;
> +
> GType spice_session_get_type(void);
>
> SpiceSession *spice_session_new(void);
> @@ -115,6 +133,19 @@ gboolean spice_session_get_read_only(SpiceSession *session);
> SpiceURI *spice_session_get_proxy_uri(SpiceSession *session);
> gboolean spice_session_is_for_migration(SpiceSession *session);
>
> +const SpiceMonitorConfig *spice_session_get_monitor_config(SpiceSession *session,
> + uint32_t channel_id,
> + uint32_t monitor_id);
> +
> +void spice_session_update_monitor_config_dimensions(SpiceSession *session,
> + uint32_t channel_id, uint32_t monitor_id,
> + uint32_t x, uint32_t y,
> + uint32_t width, uint32_t height, gboolean update);
> +
> +void spice_session_update_monitor_config_enabled(SpiceSession *session,
> + uint32_t channel_id, uint32_t monitor_id,
> + gboolean enabled, gboolean update);
> +
> G_END_DECLS
>
> #endif /* __SPICE_CLIENT_SESSION_H__ */
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 853c9df..e361c91 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -256,7 +256,7 @@ static void update_ready(SpiceDisplay *display)
> * application will manage the state of the displays.
> */
> if (d->resize_guest_enable) {
> - spice_main_channel_update_display_enabled(d->main, get_display_id(display), ready, TRUE);
> + spice_session_update_monitor_config_enabled(d->session, d->channel_id, d->monitor_id, ready, TRUE);
> }
>
> if (d->ready == ready)
> @@ -1204,8 +1204,9 @@ static void recalc_geometry(GtkWidget *widget)
> d->ww, d->wh, zoom);
>
> if (d->resize_guest_enable)
> - spice_main_channel_update_display(d->main, get_display_id(display),
> - d->area.x, d->area.y, d->ww / zoom, d->wh / zoom, TRUE);
> + spice_session_update_monitor_config_dimensions(d->session, d->channel_id, d->monitor_id,
> + d->area.x, d->area.y,
> + d->ww / zoom, d->wh / zoom, TRUE);
> }
>
> /* ---------------------------------------------------------------- */
> diff --git a/tools/spicy.c b/tools/spicy.c
> index 263c15f..fc2b88a 100644
> --- a/tools/spicy.c
> +++ b/tools/spicy.c
> @@ -614,11 +614,11 @@ static void menu_cb_resize_to(GtkAction *action G_GNUC_UNUSED,
>
> gtk_widget_show_all(dialog);
> if (gtk_dialog_run(GTK_DIALOG (dialog)) == GTK_RESPONSE_APPLY) {
> - spice_main_channel_update_display_enabled(win->conn->main, win->id + win->monitor_id, TRUE,
> - FALSE);
> - spice_main_channel_update_display(
> - win->conn->main,
> - win->id + win->monitor_id,
> + spice_session_update_monitor_config_enabled(win->conn->session, win->id, win->monitor_id,
> + TRUE, FALSE);
> + spice_session_update_monitor_config_dimensions(
> + win->conn->session,
> + win->id, win->monitor_id,
> gtk_spin_button_get_value_as_int(GTK_SPIN_BUTTON(spin_x)),
> gtk_spin_button_get_value_as_int(GTK_SPIN_BUTTON(spin_y)),
> gtk_spin_button_get_value_as_int(GTK_SPIN_BUTTON(spin_width)),
> @@ -1439,10 +1439,7 @@ static void del_window(spice_connection *conn, SpiceWindow *win)
>
> g_debug("del display monitor %d:%d", win->id, win->monitor_id);
> conn->wins[win->id * CHANNELID_MAX + win->monitor_id] = NULL;
> - if (win->id > 0)
> - spice_main_channel_update_display_enabled(conn->main, win->id, FALSE, TRUE);
> - else
> - spice_main_channel_update_display_enabled(conn->main, win->monitor_id, FALSE, TRUE);
> + spice_session_update_monitor_config_enabled(conn->session, win->id, win->monitor_id, FALSE, TRUE);
> spice_main_channel_send_monitor_config(conn->main);
>
> destroy_spice_window(win);
> --
> 2.18.0
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
All in all, it looks to me like you are moving away from the static
monitor config array, to a dynamic array that can support a mix of
channel_id & monitor_id. In short, it feels wrong because that's not
something we want to support. Or do we?
--
Marc-André Lureau
More information about the Spice-devel
mailing list