Re: [Spice-devel] [spice-gtk v1 1/2] Deprecate “color-depth” properties
Hi, On Wed, Dec 12, 2018 at 05:46:57PM +0100, Christophe Fergeau wrote: > On Tue, Dec 11, 2018 at 01:48:14PM +0100, Victor Toso wrote: > > @@ -297,8 +296,8 @@ static void spice_main_get_property(GObject*object, > > case PROP_DISPLAY_DISABLE_ANIMATION: > > g_value_set_boolean(value, c->display_disable_animation); > > break; > > -case PROP_DISPLAY_COLOR_DEPTH: > > -g_value_set_uint(value, c->display_color_depth); > > +case PROP_DISPLAY_COLOR_DEPTH: /* FIXME: deprecated */ > > +g_value_set_uint(value, 32); > > This deserves a warning too I think. Ok > > @@ -551,11 +547,19 @@ static void > > spice_main_channel_class_init(SpiceMainChannelClass *klass) > >G_PARAM_CONSTRUCT | > >G_PARAM_STATIC_STRINGS)); > > > > +/** > > + * SpiceMainChannel:color-depth: > > + * > > + * Deprecated: 0.36: Deprecated due lack of support in drivers, only > > Windows 7 and older. > > + * This option is currently ignored. > > "due to lack of support" Thanks, fixed > > @@ -667,8 +666,8 @@ static void spice_session_get_property(GObject > > *gobject, > > case PROP_SECURE_CHANNELS: > > g_value_set_boxed(value, s->secure_channels); > > break; > > -case PROP_COLOR_DEPTH: > > -g_value_set_int(value, s->color_depth); > > +case PROP_COLOR_DEPTH: /* FIXME: deprecated */ > > +g_value_set_int(value, 0); > > Why not 32? I'd add a warning too. That's the default of the property, "0" means "don't set it". > Acked-by: Christophe Fergeau > hopefully not too many people are using it. > > Christophe Indeed. I'll send a v2 later Today. Cheers, Victor > > break; > > case PROP_AUDIO: > > g_value_set_boolean(value, s->audio); > > @@ -808,7 +807,7 @@ static void spice_session_set_property(GObject > > *gobject, > > s->secure_channels = g_value_dup_boxed(value); > > break; > > case PROP_COLOR_DEPTH: > > -s->color_depth = g_value_get_int(value); > > +spice_info("SpiceSession::color-depth has been deprecated. > > Property is ignored"); > > break; > > case PROP_AUDIO: > > s->audio = g_value_get_boolean(value); > > @@ -1106,6 +1105,9 @@ static void > > spice_session_class_init(SpiceSessionClass *klass) > > * Display color depth to set on new display channels. If 0, don't set. > > * > > * Since: 0.7 > > + * > > + * Deprecated: 0.36: Deprecated due lack of support in drivers, only > > Windows 7 and older. > > + * This option is currently ignored. > > **/ > > g_object_class_install_property > > (gobject_class, PROP_COLOR_DEPTH, > > @@ -1113,6 +1115,7 @@ static void > > spice_session_class_init(SpiceSessionClass *klass) > >"Color depth", > >"Display channel color depth", > >0, 32, 0, > > + G_PARAM_DEPRECATED | > >G_PARAM_READWRITE | > >G_PARAM_STATIC_STRINGS)); > > > > @@ -2224,8 +2227,6 @@ void spice_session_channel_new(SpiceSession *session, > > SpiceChannel *channel) > > "disable-font-smooth", all || > > spice_strv_contains(s->disable_effects, "font-smooth"), > > "disable-animation", all || > > spice_strv_contains(s->disable_effects, "animation"), > > NULL); > > -if (s->color_depth != 0) > > -g_object_set(channel, "color-depth", s->color_depth, NULL); > > > > CHANNEL_DEBUG(channel, "new main channel, switching"); > > s->cmain = channel; > > -- > > 2.19.2 > > > > ___ > > Spice-devel mailing list > > Spice-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/spice-devel signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v1 1/2] Deprecate “color-depth” properties
Hi, On Thu, Dec 13, 2018 at 06:16:36PM +0200, Uri Lublin wrote: > On 12/11/18 2:48 PM, Victor Toso wrote: > > From: Victor Toso > > > > With commit 1a980f3712 we deprecated some command line options. The > > color-depth one is the only one which is not used for testing/debug > > purposes. > > > > The intention of this option is to set guest's video driver color > > configuration, currently only windows guest agent uses this feature up > > to Windows 7. Windows 8 and up, setting color depth below 32 would > > fail. We should not encourage the usage of this feature. > > > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1543538 > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1350853 > > > > This patch deprecates both SpiceMainChannel::color-depth and > > SpiceSession::color-depth while ignoring any set/get to it. > > Hi, > > Since Windows 7 guest is supported (still), I think we > better not deprecate features that work on Windows 7, > even if it's the only guest it works on (unless the > impact on user experience is pretty small). Well, they still can go to the guest's settings in Display > QXL and change it. This would only affect users that change color-depth on guest often by spice command line which is a hard use case to make a case in keeping. Code with this patches will not interfere with color-depth so if it was 16, it'll be kept 16 till the user changes this default in the guest configs. > Uri. > > > > > Signed-off-by: Victor Toso > > --- > > src/channel-main.c | 27 +-- > > src/spice-session.c | 13 +++-- > > 2 files changed, 20 insertions(+), 20 deletions(-) > > > > diff --git a/src/channel-main.c b/src/channel-main.c > > index 4c6bc70..b5e1930 100644 > > --- a/src/channel-main.c > > +++ b/src/channel-main.c > > @@ -89,7 +89,6 @@ struct _SpiceMainChannelPrivate { > > boolagent_caps_received; > > gbooleanagent_display_config_sent; > > -guint8 display_color_depth; > > gbooleandisplay_disable_wallpaper:1; > > gbooleandisplay_disable_font_smooth:1; > > gbooleandisplay_disable_animation:1; > > @@ -297,8 +296,8 @@ static void spice_main_get_property(GObject*object, > > case PROP_DISPLAY_DISABLE_ANIMATION: > > g_value_set_boolean(value, c->display_disable_animation); > > break; > > -case PROP_DISPLAY_COLOR_DEPTH: > > -g_value_set_uint(value, c->display_color_depth); > > +case PROP_DISPLAY_COLOR_DEPTH: /* FIXME: deprecated */ > > +g_value_set_uint(value, 32); > > break; > > case PROP_DISABLE_DISPLAY_POSITION: > > g_value_set_boolean(value, c->disable_display_position); > > @@ -331,12 +330,9 @@ static void spice_main_set_property(GObject *gobject, > > guint prop_id, > > case PROP_DISPLAY_DISABLE_ANIMATION: > > c->display_disable_animation = g_value_get_boolean(value); > > break; > > -case PROP_DISPLAY_COLOR_DEPTH: { > > -guint color_depth = g_value_get_uint(value); > > -g_return_if_fail(color_depth % 8 == 0); > > -c->display_color_depth = color_depth; > > +case PROP_DISPLAY_COLOR_DEPTH: > > +spice_info("SpiceMainChannel::color-depth has been deprecated. > > Property is ignored"); > > break; > > -} > > case PROP_DISABLE_DISPLAY_POSITION: > > c->disable_display_position = g_value_get_boolean(value); > > break; > > @@ -551,11 +547,19 @@ static void > > spice_main_channel_class_init(SpiceMainChannelClass *klass) > > G_PARAM_CONSTRUCT | > > G_PARAM_STATIC_STRINGS)); > > +/** > > + * SpiceMainChannel:color-depth: > > + * > > + * Deprecated: 0.36: Deprecated due lack of support in drivers, only > > Windows 7 and older. > > + * This option is currently ignored. > > + * > > + **/ > > g_object_class_install_property > > (gobject_class, PROP_DISPLAY_COLOR_DEPTH, > >g_param_spec_uint("color-depth", > > "Color depth", > > "Color depth", 0, 32, 0, > > + G_PARAM_DEPRECATED | > > G_PARAM_READWRITE | > > G_PARAM_CONSTRUCT | > > G_PARAM_STATIC_STRINGS)); > > @@ -1138,7 +1142,7 @@ gboolean > > spice_main_channel_send_monitor_config(SpiceMainChannel *channel) > > j++; > > continue; > > } > > -mon->monitors[j].depth = c->display_color_depth ? > > c->display_color_depth : 32; > > +mon->monitors[j].depth = 32; > > mon->monitors[j].width = c->display[i].width; > > mon->monitors[j].height = c->display[i].height; > > mon->monitors[j].x = c->displ
Re: [Spice-devel] [spice-gtk v1 1/2] Deprecate “color-depth” properties
On 12/11/18 2:48 PM, Victor Toso wrote: From: Victor Toso With commit 1a980f3712 we deprecated some command line options. The color-depth one is the only one which is not used for testing/debug purposes. The intention of this option is to set guest's video driver color configuration, currently only windows guest agent uses this feature up to Windows 7. Windows 8 and up, setting color depth below 32 would fail. We should not encourage the usage of this feature. Related: https://bugzilla.redhat.com/show_bug.cgi?id=1543538 Related: https://bugzilla.redhat.com/show_bug.cgi?id=1350853 This patch deprecates both SpiceMainChannel::color-depth and SpiceSession::color-depth while ignoring any set/get to it. Hi, Since Windows 7 guest is supported (still), I think we better not deprecate features that work on Windows 7, even if it's the only guest it works on (unless the impact on user experience is pretty small). Uri. Signed-off-by: Victor Toso --- src/channel-main.c | 27 +-- src/spice-session.c | 13 +++-- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/channel-main.c b/src/channel-main.c index 4c6bc70..b5e1930 100644 --- a/src/channel-main.c +++ b/src/channel-main.c @@ -89,7 +89,6 @@ struct _SpiceMainChannelPrivate { boolagent_caps_received; gbooleanagent_display_config_sent; -guint8 display_color_depth; gbooleandisplay_disable_wallpaper:1; gbooleandisplay_disable_font_smooth:1; gbooleandisplay_disable_animation:1; @@ -297,8 +296,8 @@ static void spice_main_get_property(GObject*object, case PROP_DISPLAY_DISABLE_ANIMATION: g_value_set_boolean(value, c->display_disable_animation); break; -case PROP_DISPLAY_COLOR_DEPTH: -g_value_set_uint(value, c->display_color_depth); +case PROP_DISPLAY_COLOR_DEPTH: /* FIXME: deprecated */ +g_value_set_uint(value, 32); break; case PROP_DISABLE_DISPLAY_POSITION: g_value_set_boolean(value, c->disable_display_position); @@ -331,12 +330,9 @@ static void spice_main_set_property(GObject *gobject, guint prop_id, case PROP_DISPLAY_DISABLE_ANIMATION: c->display_disable_animation = g_value_get_boolean(value); break; -case PROP_DISPLAY_COLOR_DEPTH: { -guint color_depth = g_value_get_uint(value); -g_return_if_fail(color_depth % 8 == 0); -c->display_color_depth = color_depth; +case PROP_DISPLAY_COLOR_DEPTH: +spice_info("SpiceMainChannel::color-depth has been deprecated. Property is ignored"); break; -} case PROP_DISABLE_DISPLAY_POSITION: c->disable_display_position = g_value_get_boolean(value); break; @@ -551,11 +547,19 @@ static void spice_main_channel_class_init(SpiceMainChannelClass *klass) G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS)); +/** + * SpiceMainChannel:color-depth: + * + * Deprecated: 0.36: Deprecated due lack of support in drivers, only Windows 7 and older. + * This option is currently ignored. + * + **/ g_object_class_install_property (gobject_class, PROP_DISPLAY_COLOR_DEPTH, g_param_spec_uint("color-depth", "Color depth", "Color depth", 0, 32, 0, + G_PARAM_DEPRECATED | G_PARAM_READWRITE | G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS)); @@ -1138,7 +1142,7 @@ gboolean spice_main_channel_send_monitor_config(SpiceMainChannel *channel) j++; continue; } -mon->monitors[j].depth = c->display_color_depth ? c->display_color_depth : 32; +mon->monitors[j].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; @@ -1302,11 +1306,6 @@ static void agent_display_config(SpiceMainChannel *channel) config.flags |= VD_AGENT_DISPLAY_CONFIG_FLAG_DISABLE_ANIMATION; } -if (c->display_color_depth != 0) { -config.flags |= VD_AGENT_DISPLAY_CONFIG_FLAG_SET_COLOR_DEPTH; -config.depth = c->display_color_depth; -} - CHANNEL_DEBUG(channel, "display_config: flags: %u, depth: %u", config.flags, config.depth); agent_msg_queue(channel, VD_AGENT_DISPLAY_CONFIG, sizeof(VDAgentDisplayConfig), &config); diff --git a/src/spice-session.c b/src/spice-session.c index 7ea1c21..859ea74 100644 --- a/src/spice-session.c +++ b/src/spice-session.c @@ -83,7 +83,6 @@ struct _SpiceSessionPrivate { GStrv disable_effects; GStrv secure_channels; -g
Re: [Spice-devel] [spice-gtk v1 1/2] Deprecate “color-depth” properties
On Tue, Dec 11, 2018 at 01:48:14PM +0100, Victor Toso wrote: > From: Victor Toso > > With commit 1a980f3712 we deprecated some command line options. The > color-depth one is the only one which is not used for testing/debug > purposes. > > The intention of this option is to set guest's video driver color > configuration, currently only windows guest agent uses this feature up > to Windows 7. Windows 8 and up, setting color depth below 32 would > fail. We should not encourage the usage of this feature. > > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1543538 > Related: https://bugzilla.redhat.com/show_bug.cgi?id=1350853 > > This patch deprecates both SpiceMainChannel::color-depth and > SpiceSession::color-depth while ignoring any set/get to it. > > Signed-off-by: Victor Toso > --- > src/channel-main.c | 27 +-- > src/spice-session.c | 13 +++-- > 2 files changed, 20 insertions(+), 20 deletions(-) > > diff --git a/src/channel-main.c b/src/channel-main.c > index 4c6bc70..b5e1930 100644 > --- a/src/channel-main.c > +++ b/src/channel-main.c > @@ -89,7 +89,6 @@ struct _SpiceMainChannelPrivate { > boolagent_caps_received; > > gbooleanagent_display_config_sent; > -guint8 display_color_depth; > gbooleandisplay_disable_wallpaper:1; > gbooleandisplay_disable_font_smooth:1; > gbooleandisplay_disable_animation:1; > @@ -297,8 +296,8 @@ static void spice_main_get_property(GObject*object, > case PROP_DISPLAY_DISABLE_ANIMATION: > g_value_set_boolean(value, c->display_disable_animation); > break; > -case PROP_DISPLAY_COLOR_DEPTH: > -g_value_set_uint(value, c->display_color_depth); > +case PROP_DISPLAY_COLOR_DEPTH: /* FIXME: deprecated */ > +g_value_set_uint(value, 32); This deserves a warning too I think. > break; > case PROP_DISABLE_DISPLAY_POSITION: > g_value_set_boolean(value, c->disable_display_position); > @@ -331,12 +330,9 @@ static void spice_main_set_property(GObject *gobject, > guint prop_id, > case PROP_DISPLAY_DISABLE_ANIMATION: > c->display_disable_animation = g_value_get_boolean(value); > break; > -case PROP_DISPLAY_COLOR_DEPTH: { > -guint color_depth = g_value_get_uint(value); > -g_return_if_fail(color_depth % 8 == 0); > -c->display_color_depth = color_depth; > +case PROP_DISPLAY_COLOR_DEPTH: > +spice_info("SpiceMainChannel::color-depth has been deprecated. > Property is ignored"); > break; > -} > case PROP_DISABLE_DISPLAY_POSITION: > c->disable_display_position = g_value_get_boolean(value); > break; > @@ -551,11 +547,19 @@ static void > spice_main_channel_class_init(SpiceMainChannelClass *klass) >G_PARAM_CONSTRUCT | >G_PARAM_STATIC_STRINGS)); > > +/** > + * SpiceMainChannel:color-depth: > + * > + * Deprecated: 0.36: Deprecated due lack of support in drivers, only > Windows 7 and older. > + * This option is currently ignored. "due to lack of support" > + * > + **/ > g_object_class_install_property > (gobject_class, PROP_DISPLAY_COLOR_DEPTH, > g_param_spec_uint("color-depth", > "Color depth", > "Color depth", 0, 32, 0, > + G_PARAM_DEPRECATED | > G_PARAM_READWRITE | > G_PARAM_CONSTRUCT | > G_PARAM_STATIC_STRINGS)); > @@ -1138,7 +1142,7 @@ gboolean > spice_main_channel_send_monitor_config(SpiceMainChannel *channel) > j++; > continue; > } > -mon->monitors[j].depth = c->display_color_depth ? > c->display_color_depth : 32; > +mon->monitors[j].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; > @@ -1302,11 +1306,6 @@ static void agent_display_config(SpiceMainChannel > *channel) > config.flags |= VD_AGENT_DISPLAY_CONFIG_FLAG_DISABLE_ANIMATION; > } > > -if (c->display_color_depth != 0) { > -config.flags |= VD_AGENT_DISPLAY_CONFIG_FLAG_SET_COLOR_DEPTH; > -config.depth = c->display_color_depth; > -} > - > CHANNEL_DEBUG(channel, "display_config: flags: %u, depth: %u", > config.flags, config.depth); > > agent_msg_queue(channel, VD_AGENT_DISPLAY_CONFIG, > sizeof(VDAgentDisplayConfig), &config); > diff --git a/src/spice-session.c b/src/spice-session.c > index 7ea1c21..859ea74 100644 > --- a/src/spice-session.c > +++ b/src/spice-session.c > @@ -83,7 +83,6 @@ struct _SpiceSessionPrivate { > > GStrv disable_effect