[Spice-devel,spice-gtk,v1] gtk-session: Use GWeakRef

Submitted by Victor Toso on Feb. 20, 2017, 2:49 p.m.

Details

Message ID 20170220144902.8423-1-victortoso@redhat.com
State New
Headers show
Series "gtk-session: Use GWeakRef" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Feb. 20, 2017, 2:49 p.m.
From: Victor Toso <me@victortoso.com>

The custom WeakRef structure was introduced in 9baba9fd89 (2012) to
fix rhbz#743773. Since glib 2.32, GWeakRef was introduced and it
behaves similarly to our WeakRef.

Moving to GWeakRef to remove some code which exists in glib.

Note that I'm keeping to utility functions:
- get_weak_ref(gpointer object): which returns a newly allocated
  GWeakRef, initialized with @object;
- free_weak_ref(gpointer pdata): which frees the GWeakRef and returns
  the original object or NULL. It also takes care of an extra
  reference to the object.

Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/spice-gtk-session.c | 52 +++++++++++++++++++------------------------------
 1 file changed, 20 insertions(+), 32 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
index 5688cba..88f4488 100644
--- a/src/spice-gtk-session.c
+++ b/src/spice-gtk-session.c
@@ -575,31 +575,26 @@  static const struct {
     }
 };
 
-typedef struct _WeakRef {
-    GObject *object;
-} WeakRef;
-
-static void weak_notify_cb(WeakRef *weakref, GObject *object)
-{
-    weakref->object = NULL;
-}
-
-static WeakRef* weak_ref(GObject *object)
+static GWeakRef* get_weak_ref(gpointer object)
 {
-    WeakRef *weakref = g_new(WeakRef, 1);
-
-    g_object_weak_ref(object, (GWeakNotify)weak_notify_cb, weakref);
-    weakref->object = object;
-
+    GWeakRef *weakref = g_new(GWeakRef, 1);
+    g_weak_ref_init(weakref, object);
     return weakref;
 }
 
-static void weak_unref(WeakRef* weakref)
+static gpointer free_weak_ref(gpointer data)
 {
-    if (weakref->object)
-        g_object_weak_unref(weakref->object, (GWeakNotify)weak_notify_cb, weakref);
+    GWeakRef *weakref = data;
+    gpointer object = g_weak_ref_get(weakref);
 
+    g_weak_ref_clear(weakref);
     g_free(weakref);
+    if (object != NULL) {
+        /* The main referece still exists as object is not NULL, so we can
+         * remove the strong reference given by g_weak_ref_get */
+        g_object_unref(object);
+    }
+    return object;
 }
 
 static void clipboard_get_targets(GtkClipboard *clipboard,
@@ -607,9 +602,7 @@  static void clipboard_get_targets(GtkClipboard *clipboard,
                                   gint n_atoms,
                                   gpointer user_data)
 {
-    WeakRef *weakref = user_data;
-    SpiceGtkSession *self = (SpiceGtkSession*)weakref->object;
-    weak_unref(weakref);
+    SpiceGtkSession *self = free_weak_ref(user_data);
 
     if (self == NULL)
         return;
@@ -706,7 +699,7 @@  static void clipboard_owner_change(GtkClipboard        *clipboard,
         s->clip_hasdata[selection] = TRUE;
         if (s->auto_clipboard_enable && !read_only(self))
             gtk_clipboard_request_targets(clipboard, clipboard_get_targets,
-                                          weak_ref(G_OBJECT(self)));
+                                          get_weak_ref(self));
         break;
     default:
         s->clip_hasdata[selection] = FALSE;
@@ -939,14 +932,11 @@  static void clipboard_received_text_cb(GtkClipboard *clipboard,
                                        const gchar *text,
                                        gpointer user_data)
 {
-    WeakRef *weakref = user_data;
-    SpiceGtkSession *self = (SpiceGtkSession*)weakref->object;
+    SpiceGtkSession *self = free_weak_ref(user_data);
     char *conv = NULL;
     int len = 0;
     int selection;
 
-    weak_unref(weakref);
-
     if (self == NULL)
         return;
 
@@ -982,9 +972,7 @@  static void clipboard_received_cb(GtkClipboard *clipboard,
                                   GtkSelectionData *selection_data,
                                   gpointer user_data)
 {
-    WeakRef *weakref = user_data;
-    SpiceGtkSession *self = (SpiceGtkSession*)weakref->object;
-    weak_unref(weakref);
+    SpiceGtkSession *self = free_weak_ref(user_data);
 
     if (self == NULL)
         return;
@@ -1055,7 +1043,7 @@  static gboolean clipboard_request(SpiceMainChannel *main, guint selection,
 
     if (type == VD_AGENT_CLIPBOARD_UTF8_TEXT) {
         gtk_clipboard_request_text(cb, clipboard_received_text_cb,
-                                   weak_ref(G_OBJECT(self)));
+                                   get_weak_ref(self));
     } else {
         for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
             if (atom2agent[m].vdagent == type)
@@ -1066,7 +1054,7 @@  static gboolean clipboard_request(SpiceMainChannel *main, guint selection,
 
         atom = gdk_atom_intern_static_string(atom2agent[m].xatom);
         gtk_clipboard_request_contents(cb, atom, clipboard_received_cb,
-                                       weak_ref(G_OBJECT(self)));
+                                       get_weak_ref(self));
     }
 
     return TRUE;
@@ -1239,7 +1227,7 @@  void spice_gtk_session_copy_to_guest(SpiceGtkSession *self)
 
     if (s->clip_hasdata[selection] && !s->clip_grabbed[selection]) {
         gtk_clipboard_request_targets(s->clipboard, clipboard_get_targets,
-                                      weak_ref(G_OBJECT(self)));
+                                      get_weak_ref(self));
     }
 }
 

Comments

On Mon, Feb 20, 2017 at 03:49:02PM +0100, Victor Toso wrote:
> From: Victor Toso <me@victortoso.com>
> 
> The custom WeakRef structure was introduced in 9baba9fd89 (2012) to
> fix rhbz#743773. Since glib 2.32, GWeakRef was introduced and it
> behaves similarly to our WeakRef.
> 
> Moving to GWeakRef to remove some code which exists in glib.
> 
> Note that I'm keeping to utility functions:
> - get_weak_ref(gpointer object): which returns a newly allocated
>   GWeakRef, initialized with @object;
> - free_weak_ref(gpointer pdata): which frees the GWeakRef and returns
>   the original object or NULL. It also takes care of an extra
>   reference to the object.

Yes, this dropping of the strong ref is a bit odd at first, but in the
end it's no different than how the code was working before, we expect
the object to stay alive for the duration of the callback if it was
alive when entering in the callback.

> 
> Signed-off-by: Victor Toso <victortoso@redhat.com>
> ---
>  src/spice-gtk-session.c | 52 +++++++++++++++++++------------------------------
>  1 file changed, 20 insertions(+), 32 deletions(-)
> 
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 5688cba..88f4488 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -575,31 +575,26 @@ static const struct {
>      }
>  };
>  
> -typedef struct _WeakRef {
> -    GObject *object;
> -} WeakRef;
> -
> -static void weak_notify_cb(WeakRef *weakref, GObject *object)
> -{
> -    weakref->object = NULL;
> -}
> -
> -static WeakRef* weak_ref(GObject *object)
> +static GWeakRef* get_weak_ref(gpointer object)
>  {
> -    WeakRef *weakref = g_new(WeakRef, 1);
> -
> -    g_object_weak_ref(object, (GWeakNotify)weak_notify_cb, weakref);
> -    weakref->object = object;
> -
> +    GWeakRef *weakref = g_new(GWeakRef, 1);
> +    g_weak_ref_init(weakref, object);
>      return weakref;
>  }
>  
> -static void weak_unref(WeakRef* weakref)
> +static gpointer free_weak_ref(gpointer data)
>  {
> -    if (weakref->object)
> -        g_object_weak_unref(weakref->object, (GWeakNotify)weak_notify_cb, weakref);
> +    GWeakRef *weakref = data;
> +    gpointer object = g_weak_ref_get(weakref);
>  
> +    g_weak_ref_clear(weakref);
>      g_free(weakref);
> +    if (object != NULL) {
> +        /* The main referece still exists as object is not NULL, so we can

'reference'

Looks good to me, though I'd probably return a ref to the callers, and
add a g_object_unref() to the end of the callbacks.

Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Hi,

On Tue, Feb 21, 2017 at 05:16:26PM +0100, Christophe Fergeau wrote:
> On Mon, Feb 20, 2017 at 03:49:02PM +0100, Victor Toso wrote:
> > From: Victor Toso <me@victortoso.com>
> > 
> > The custom WeakRef structure was introduced in 9baba9fd89 (2012) to
> > fix rhbz#743773. Since glib 2.32, GWeakRef was introduced and it
> > behaves similarly to our WeakRef.
> > 
> > Moving to GWeakRef to remove some code which exists in glib.
> > 
> > Note that I'm keeping to utility functions:

Fixed typo: to -> two

> > - get_weak_ref(gpointer object): which returns a newly allocated
> >   GWeakRef, initialized with @object;
> > - free_weak_ref(gpointer pdata): which frees the GWeakRef and returns
> >   the original object or NULL. It also takes care of an extra
> >   reference to the object.
>
> Yes, this dropping of the strong ref is a bit odd at first, but in the
> end it's no different than how the code was working before, we expect
> the object to stay alive for the duration of the callback if it was
> alive when entering in the callback.
> 
> > 
> > Signed-off-by: Victor Toso <victortoso@redhat.com>
> > ---
> >  src/spice-gtk-session.c | 52 +++++++++++++++++++------------------------------
> >  1 file changed, 20 insertions(+), 32 deletions(-)
> > 
> > diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> > index 5688cba..88f4488 100644
> > --- a/src/spice-gtk-session.c
> > +++ b/src/spice-gtk-session.c
> > @@ -575,31 +575,26 @@ static const struct {
> >      }
> >  };
> >  
> > -typedef struct _WeakRef {
> > -    GObject *object;
> > -} WeakRef;
> > -
> > -static void weak_notify_cb(WeakRef *weakref, GObject *object)
> > -{
> > -    weakref->object = NULL;
> > -}
> > -
> > -static WeakRef* weak_ref(GObject *object)
> > +static GWeakRef* get_weak_ref(gpointer object)
> >  {
> > -    WeakRef *weakref = g_new(WeakRef, 1);
> > -
> > -    g_object_weak_ref(object, (GWeakNotify)weak_notify_cb, weakref);
> > -    weakref->object = object;
> > -
> > +    GWeakRef *weakref = g_new(GWeakRef, 1);
> > +    g_weak_ref_init(weakref, object);
> >      return weakref;
> >  }
> >  
> > -static void weak_unref(WeakRef* weakref)
> > +static gpointer free_weak_ref(gpointer data)
> >  {
> > -    if (weakref->object)
> > -        g_object_weak_unref(weakref->object, (GWeakNotify)weak_notify_cb, weakref);
> > +    GWeakRef *weakref = data;
> > +    gpointer object = g_weak_ref_get(weakref);
> >  
> > +    g_weak_ref_clear(weakref);
> >      g_free(weakref);
> > +    if (object != NULL) {
> > +        /* The main referece still exists as object is not NULL, so we can
>
> 'reference'

Fixed

>
> Looks good to me, though I'd probably return a ref to the callers, and
> add a g_object_unref() to the end of the callbacks.
>
> Acked-by: Christophe Fergeau <cfergeau@redhat.com>

Yeah, it was my first option too but I didn't like to outcome as much as
having the unref in the helper function.

Thanks, I'll be pushing it soon

        toso