[Spice-devel] [PATCH spice-gtk 01/15] audio: do not access session private struct

[Jonathon Jongsma]

On Tue, 2014-11-25 at 14:19 +0100, Marc-André Lureau wrote:
> ---
>  gtk/spice-audio.c        | 50 ++++++++++--------------------------------------
>  gtk/spice-session-priv.h |  1 +
>  gtk/spice-session.c      | 42 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 53 insertions(+), 40 deletions(-)
> 
> diff --git a/gtk/spice-audio.c b/gtk/spice-audio.c
> index 638f667..7754736 100644
> --- a/gtk/spice-audio.c
> +++ b/gtk/spice-audio.c
> @@ -166,17 +166,18 @@ static void connect_channel(SpiceAudio *self, SpiceChannel *channel)
>  
>  static void update_audio_channels(SpiceAudio *self, SpiceSession *session)
>  {
> -    if (session->priv->audio) {
> -        GList *list, *tmp;
> -
> -        list = spice_session_get_channels(session);
> -        for (tmp = g_list_first(list); tmp != NULL; tmp = g_list_next(tmp)) {
> -            connect_channel(self, tmp->data);
> -        }
> -        g_list_free(list);
> -    } else {
> +    GList *list, *tmp;
> +
> +    if (!spice_session_get_audio_enabled(session)) {
>          g_debug("FIXME: disconnect audio channels");
> +        return;
> +    }
> +
> +    list = spice_session_get_channels(session);
> +    for (tmp = g_list_first(list); tmp != NULL; tmp = g_list_next(tmp)) {
> +        connect_channel(self, tmp->data);
>      }
> +    g_list_free(list);
>  }
>  
>  static void channel_new(SpiceSession *session, SpiceChannel *channel, SpiceAudio *self)
> @@ -229,34 +230,3 @@ SpiceAudio *spice_audio_new(SpiceSession *session, GMainContext *context,
>  
>      return self;
>  }
> -
> -/**
> - * spice_audio_get:
> - * @session: the #SpiceSession to connect to
> - * @context: (allow-none): a #GMainContext to attach to (or %NULL for default).
> - *
> - * Gets the #SpiceAudio associated with the passed in #SpiceSession.
> - * A new #SpiceAudio instance will be created the first time this
> - * function is called for a certain #SpiceSession.
> - *
> - * Note that this function returns a weak reference, which should not be used
> - * after the #SpiceSession itself has been unref-ed by the caller.
> - *
> - * Returns: (transfer none): a weak reference to a #SpiceAudio
> - * instance or %NULL if failed.
> - **/
> -SpiceAudio *spice_audio_get(SpiceSession *session, GMainContext *context)
> -{
> -    static GStaticMutex mutex = G_STATIC_MUTEX_INIT;
> -    SpiceAudio *self;
> -
> -    g_static_mutex_lock(&mutex);
> -    self = session->priv->audio_manager;
> -    if (self == NULL) {
> -        self = spice_audio_new(session, context, NULL);
> -        session->priv->audio_manager = self;
> -    }
> -    g_static_mutex_unlock(&mutex);
> -
> -    return self;
> -}
> diff --git a/gtk/spice-session-priv.h b/gtk/spice-session-priv.h
> index 8e102e0..55e2ed2 100644
> --- a/gtk/spice-session-priv.h
> +++ b/gtk/spice-session-priv.h
> @@ -172,6 +172,7 @@ guint32 spice_session_get_playback_latency(SpiceSession *session);
>  void spice_session_sync_playback_latency(SpiceSession *session);
>  const gchar* spice_session_get_shared_dir(SpiceSession *session);
>  void spice_session_set_shared_dir(SpiceSession *session, const gchar *dir);
> +gboolean spice_session_get_audio_enabled(SpiceSession *session);
>  
>  G_END_DECLS
>  
> diff --git a/gtk/spice-session.c b/gtk/spice-session.c
> index c44a3e1..948c238 100644
> --- a/gtk/spice-session.c
> +++ b/gtk/spice-session.c
> @@ -32,6 +32,7 @@
>  #include "wocky-http-proxy.h"
>  #include "spice-uri-priv.h"
>  #include "channel-playback-priv.h"
> +#include "spice-audio.h"
>  
>  struct channel {
>      SpiceChannel      *channel;
> @@ -2279,3 +2280,44 @@ SpiceURI *spice_session_get_proxy_uri(SpiceSession *session)
>  
>      return s->proxy;
>  }
> +
> +/**
> + * spice_audio_get:
> + * @session: the #SpiceSession to connect to
> + * @context: (allow-none): a #GMainContext to attach to (or %NULL for default).
> + *
> + * Gets the #SpiceAudio associated with the passed in #SpiceSession.
> + * A new #SpiceAudio instance will be created the first time this
> + * function is called for a certain #SpiceSession.
> + *
> + * Note that this function returns a weak reference, which should not be used
> + * after the #SpiceSession itself has been unref-ed by the caller.
> + *
> + * Returns: (transfer none): a weak reference to a #SpiceAudio
> + * instance or %NULL if failed.
> + **/
> +SpiceAudio *spice_audio_get(SpiceSession *session, GMainContext *context)
> +{
> +    static GStaticMutex mutex = G_STATIC_MUTEX_INIT;
> +    SpiceAudio *self;
> +
> +    g_return_val_if_fail(SPICE_IS_SESSION(session), NULL);
> +
> +    g_static_mutex_lock(&mutex);
> +    self = session->priv->audio_manager;
> +    if (self == NULL) {
> +        self = spice_audio_new(session, context, NULL);
> +        session->priv->audio_manager = self;
> +    }
> +    g_static_mutex_unlock(&mutex);
> +
> +    return self;
> +}

I would prefer that this move of spice_audio_get() to a different source
file happened in a separate commit. It's a self-contained change and
makes the other commit harder to understand since it makes me try to
figure out how these different parts are related. Otherwise ACK.

> +
> +G_GNUC_INTERNAL
> +gboolean spice_session_get_audio_enabled(SpiceSession *session)
> +{
> +    g_return_val_if_fail(SPICE_IS_SESSION(session), FALSE);
> +
> +    return session->priv->audio;
> +}