[Bug 63402] Avatar image caching is not asynchronous

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=63402

--- Comment #5 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
Thanks for looking into this!

I'm reviewing a flattened version here. I'd suggest attaching a new, flattened
version with these changes - it'll be easier to review that way.

+  write_data *avatar_data = g_slice_new (write_data);
+  write_data *mime_data = g_slice_new (write_data);
+  AvatarFileWrittenData *avatar_file_written_data = g_slice_new
+      (AvatarFileWrittenData);

These don't need to be separate structs. I'd use a single struct to represent
the whole operation:

typedef struct {
    TpContact *self;
    gchar *token;
    gchar *mime_type;
    GFile *mime_file;
} WriteAvatarData;

(and if you discover you need more fields, you can add them).

You don't actually need to copy the avatar data into this struct, as far as I
can see, because you only use it for the first g_file_replace_contents_async()
call (which will, internally, copy it).

It looks as though you leak the memory for these structs in error code paths. I
would suggest not allocating them until you need them (that would be just after
the comment "Save avatar in cache..." here, I think) and then you won't need to
worry about cleanup on errors.

+  avatar_file = g_file_new_for_path (filename);

You don't unref this, so it's leaked. Unref it after calling
g_file_replace_contents_async().

+      g_free (avatar_data->str);
+      g_slice_free (write_data, avatar_data);
+      g_free (mime_data->str);
+      g_slice_free (write_data, mime_data);
+      g_object_unref (avatar_file_written_data->mime_file);
+      g_slice_free (AvatarFileWrittenData, avatar_file_written_data);

This should be in a new function avatar_file_written_data_free() (or
write_avatar_data_free() if you use the naming I suggested) so you don't have
to keep duplicating it.

I haven't reviewed all the duplicate copies of freeing the struct and its
contents, so there might be memory leaks there - but when you do this, it'll be
obvious at a glance whether things are being freed correctly.

+  if (avatar_data->self == NULL)
+    {
+      g_free (avatar_data->str);
+      g_slice_free (write_data, avatar_data);
+      g_free (mime_data->str);
+      g_slice_free (write_data, mime_data);
+      g_object_unref (avatar_file_written_data->mime_file);
+      g_slice_free (AvatarFileWrittenData, avatar_file_written_data);
+      return;
+    }
+  /* Update the avatar token if a newer one is given */
+  contact_set_avatar_token (avatar_data->self, avatar_data->str, FALSE);
+
+  tp_clear_object (&avatar_data->self->priv->avatar_file);
+  avatar_data->self->priv->avatar_file = (GFile *) file;
+  g_free (avatar_data->str);
+  g_slice_free (write_data, avatar_data);
+
+  /* Now that the avatar-file has been written, write the mime-file. */
+  g_file_replace_contents_async (avatar_file_written_data->mime_file,
+      mime_data->str, strlen (mime_data->str), NULL, FALSE,
G_FILE_CREATE_NONE,
+      NULL, &mime_file_written, mime_data);
+
+  g_slice_free (AvatarFileWrittenData, avatar_file_written_data);

There's a logic error here: if the TpContact is NULL, you don't write the MIME
type to a file. If you're signalling the avatar token here, I'd do:

    if (data->self != NULL)
      {
        contact_set_avatar_token (data->self, data->token, FALSE);
      }

    g_file_replace_contents_async(...)

but actually it'd make more sense to delay signalling the changed avatar token
until the end as well - the application can't usefully act on it until you've
cached the file.

If writing the avatar data fails, it's probably still worth trying to write out
the MIME type, and if either or both fail, I think it's still worth signalling
the token change (and perhaps also the MIME type and avatar-file), even though
the current implementation didn't. That simplifies things down to one code
path.

General points
--------------

In both g_file_replace_contents_async calls, I think you should use
G_FILE_CREATE_PRIVATE|G_FILE_CREATE_REPLACE_DESTINATION for the overwrite -
we're potentially dealing with private data here, and REPLACE_DESTINATION gives
the closest match for how g_file_set_contents() worked.

Minor nitpicking stuff
----------------------

+typedef struct{

Coding style: space before "{".

+static void mime_file_written (GObject *file,
+                               GAsyncResult *res,
+                               gpointer user_data){

Coding style: newlines after "static void" and ")" here. We also usually indent
the second and subsequent arguments in function definitions by 4 spaces, rather
than lining them up with the "(" - please follow
<http://telepathy.freedesktop.org/wiki/Style/> and/or the style of nearby code.

+  GError* error = NULL;
+  write_data* mime_data = (write_data *) user_data;

Coding style: "type *value" not "type* value"
(<http://lists.freedesktop.org/archives/systemd-devel/2013-April/010753.html>
for the reason).

You don't need a cast when going from gpointer (or equivalently, void *) to
SomeType *.

There are a lot of (GFile *) casts here. I'd prefer this:

    static void
    something_written (GObject *source_object,
        GAsyncResult *res,
        gpointer user_data)
    {
      GFile *file = G_FILE (source_object);
      WriteFileData *data = user_data;
      GError *error = NULL;

      if (!g_file_replace_contents_finish (file, res, NULL, &error))
        {
          ...
        }

       ... (and the same for all other uses of the GFile) ...
    }

Coding style: blank line after the variable declarations, please (like in the
example immediately before this comment).

Coding style: it's conventional to check the return from the _finish function,
rather than the contents of 'error', as in the same example. If it fails, it's
OK to assume that error is non-NULL.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.