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
