Re: [spice-gtk v2 1/2] Deprecate “color-depth” properties

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Tue, Jan 08, 2019 at 04:22:55PM +0100, Victor Toso wrote:
> Hi,
> 
> On Tue, Jan 08, 2019 at 04:09:54PM +0100, Christophe Fergeau wrote:
> > On Fri, Dec 14, 2018 at 04:29:46PM +0100, Victor Toso wrote:
> > > From: Victor Toso <me at victortoso.com>
> > > 
> > > 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 <victortoso at redhat.com>
> > > Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> > 
> > Still fine with me, though it's indeed a good question that Uri raised,
> > whether we want to keep this alive a bit longer for win7 since it's
> > still supported by MS.
> 
>   | Microsoft ended mainstream support for Windows 7 on January 13,
>   | 2015, but extended support won't end until January 14, 2020.
> 
> Oh well, IMHO we could remove it for 0.36 but we can do it for
> after 0.36 as well.

Now that 0.36 is out, I'm wondering if this two patches are fine
for you? CC: teuf and Uri :)

> Thanks for the review
>  
> > Christophe
> > 
> > > ---
> > >  src/channel-main.c  | 26 +++++++++++++-------------
> > >  src/spice-session.c | 12 +++++++-----
> > >  2 files changed, 20 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/src/channel-main.c b/src/channel-main.c
> > > index 4c6bc70..02e2ffd 100644
> > > --- a/src/channel-main.c
> > > +++ b/src/channel-main.c
> > > @@ -89,7 +89,6 @@ struct _SpiceMainChannelPrivate  {
> > >      bool                        agent_caps_received;
> > >  
> > >      gboolean                    agent_display_config_sent;
> > > -    guint8                      display_color_depth;
> > >      gboolean                    display_disable_wallpaper:1;
> > >      gboolean                    display_disable_font_smooth:1;
> > >      gboolean                    display_disable_animation:1;
> > > @@ -298,7 +297,8 @@ static void spice_main_get_property(GObject    *object,
> > >          g_value_set_boolean(value, c->display_disable_animation);
> > >          break;
> > >      case PROP_DISPLAY_COLOR_DEPTH:
> > > -        g_value_set_uint(value, c->display_color_depth);
> > > +        spice_info("SpiceMainChannel::color-depth has been deprecated. Property is ignored");
> > > +        g_value_set_uint(value, 32);
> > >          break;
> > >      case PROP_DISABLE_DISPLAY_POSITION:
> > >          g_value_set_boolean(value, c->disable_display_position);
> > > @@ -331,12 +331,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 +548,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 to 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 +1143,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 +1307,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..682c3d9 100644
> > > --- a/src/spice-session.c
> > > +++ b/src/spice-session.c
> > > @@ -83,7 +83,6 @@ struct _SpiceSessionPrivate {
> > >  
> > >      GStrv             disable_effects;
> > >      GStrv             secure_channels;
> > > -    gint              color_depth;
> > >  
> > >      int               connection_id;
> > >      int               protocol;
> > > @@ -668,7 +667,8 @@ static void spice_session_get_property(GObject    *gobject,
> > >          g_value_set_boxed(value, s->secure_channels);
> > >          break;
> > >      case PROP_COLOR_DEPTH:
> > > -        g_value_set_int(value, s->color_depth);
> > > +        spice_info("SpiceSession::color-depth has been deprecated. Property is ignored");
> > > +        g_value_set_int(value, 0);
> > >          break;
> > >      case PROP_AUDIO:
> > >          g_value_set_boolean(value, s->audio);
> > > @@ -808,7 +808,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 +1106,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 to 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 +1116,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 +2228,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 at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 
> -------------- next part --------------
> A non-text attachment was scrubbed...
> Name: signature.asc
> Type: application/pgp-signature
> Size: 833 bytes
> Desc: not available
> URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190108/31c4cb09/attachment.sig>

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]