If you don't cache avatars (hint: don't do that) you always have to fetch them from the roster which is slow and bandwith consuming.

Note that there is plan to have an avatar cache reference implementation in tp-glib: https://bugs.freedesktop.org/show_bug.cgi?id=20035

Created attachment 158741 [details] [review]
[glib] make g_file_set_contents() take uint8*, not utf8

we agreed on #introspection yesterday that this was the right thing.
Also, since gjs converts strings to uint8*s, this doesn't stop you from
passing a string to GLib.file_set_contents() either.

Making the same change to g_file_get_contents() would be a lose at this
point though, since you'd end up with an array of bytes that was not
easily convertible to a string

Created attachment 158742 [details] [review]
telepathyClient: cache avatar images

I originally wrote this to share empathy's cache, but then changed my
mind at the last minute. But if we want to switch back, it's just a matter
of changing avatarManager._cacheDir. (It shouldn't cause any problems;
both programs use g_file_set_contents(), which does write-and-atomic-rename,
so they would never see a partially-written avatar file, though they might
end up both writing it.

Note that testing with google talk at least, there is an unpredictable
delay between when you change your avatar online and when you can find
out about it locally. Even killing mission-control doesn't make the new
avatar show up, so it seems that google itself is caching the old
information for a while after it changes.

Review of attachment 158742 [details] [review]:

Why is there "..." after empathy's in the commit message?

Looks good overall and works as expected. It did take it like 20 minutes to get the avatar initially, but then it populated it immediately after a restart. One big comment is that we should probably use targetIds such as foo@gmail.com, not tokens, as file names. This is what Empathy uses. They look like foo_40gmail_2ecom once they've undergone escapeAsIdentifier(). This will allow us to retrieve the avatars even before we get the tokens for the handle. We'll need to pass targetId to createIcon() and key tokens off of that in info, etc.

Other minor comments inlined.

::: js/ui/telepathyClient.js
@@ +176,3 @@
+
+        info.cacheDir = this._cacheDir + '/' + match[1];
+        GLib.mkdir_with_parents(info.cacheDir, 0700);

Do we not need to check for an error here?

@@ +179,3 @@
+
+        // info.token[handle] is the token for @handle's avatar
+        info.token = {};

Perhaps info.tokens is a more suitable name?

@@ +225,3 @@
+        if (token) {
+            let uri = GLib.filename_to_uri(this._getFileForToken(info, token), null);
+            iconBox.child = textureCache.load_uri_async(uri, iconBox._size, iconBox._size);

Checking if the file exists will be all we need to do here to decide how to populate the iconBox once we use targetId as a file name. However, with the current code we would have needed to do that too because it is possible that _setIcon() is called in a state when info.token[handle] is set, but the file doesn't yet exist. It can happen if we set it in _avatarUpdated() and then called RequestAvatarsRemote(), and then got a new createAvatar() request before _avatarRetrieved() got called or even had an error with file_set_contents() in _avatarRetrieved(). So we would need to check GLib.file_test(file, GLib.FileTest.EXISTS)) here.

@@ +280,3 @@
     createAvatar: function(conn, handle, size) {
         let iconBox = new St.Bin({ style_class: 'avatar-box' });
+        iconBox._size = size;

info properties are also only used within the AvatarManager, so not sure why there should be an underscore here, but not on any of the info properties.

@@ +298,3 @@
 
+        // If we already have the icon cached, this will fill it
+        // in. Otherwise it will fill in the default icon.

This will change once we use targetId as a file name, but with the current code the comment would have to be "If we already have the icon cached and know the token for the handle, this will fill it in. Otherwise it will fill in the default icon."

Review of attachment 158741 [details] [review]:

Would be nice to add explanation you gave in the bug comment to the annotation and to the commit message, but looks good overall.

Comment on attachment 158741 [details] [review]
[glib] make g_file_set_contents() take uint8*, not utf8

Attachment 158741 [details] pushed as b76aba7 - [glib] make g_file_set_contents() take uint8*, not utf8

(In reply to comment #4)
> One big comment is that we should probably use targetIds such as
> foo@gmail.com, not tokens, as file names. This is what Empathy uses.

hm... are you talking about the avatar.bin files under ~/.mission-control/accounts? Those are the avatars for *your own* accounts, corresponding to org.freedesktop.Telepathy.Account.Interface.Avatar, and are cached by mission-control, not empathy. Avatars for *other people's* accounts, corresponding to org.freedesktop.Telepathy.Connection.Interface.Avatars, are under ~/.cache/telepathy/avatars, and (despite the "telepathy") are cached by empathy, using token names.

> This will allow
> us to retrieve the avatars even before we get the tokens for the handle.

Yeah, if token fetching isn't going to be instantaneous-ish, it might be useful to remember the last-known mappings...

However, we can't store just the image data in a file named with just the account name; we have to store the token somewhere, so we can tell if the user has changed their avatar since we last downloaded it.

> +        GLib.mkdir_with_parents(info.cacheDir, 0700);
> 
> Do we not need to check for an error here?

Well... there aren't any sane reasons it could fail, and not much we could do to recover in the insane cases.

> +        // info.token[handle] is the token for @handle's avatar
> +        info.token = {};
> 
> Perhaps info.tokens is a more suitable name?

I tend towards using singular names for arrays if you refer to individual elements more than you refer to the array as a whole, because IMHO it reads better. "token[handle]" is the token for handle.

oops, missed on

(In reply to comment #4)
> @@ +280,3 @@
>      createAvatar: function(conn, handle, size) {
>          let iconBox = new St.Bin({ style_class: 'avatar-box' });
> +        iconBox._size = size;
> 
> info properties are also only used within the AvatarManager, so not sure why
> there should be an underscore here, but not on any of the info properties.

The "info" objects are created and owned by us and no one else ever even sees them, so underscores would be superfluous.

iconBox OTOH is an St.Bin, and has its own properties, and while it doesn't currently have a "size" property, it might in the future, and we don't want to conflict with that, so we use "_size"

Created attachment 159625 [details] [review]
telepathyClient: cache avatar images

check for existence of cached image before deciding whether to use that
or the default image. fix a comment

Review of attachment 159625 [details] [review]:

Looks good!

I was looking at ~/.cache/Empathy/avatars directory. Upon closer inspection, names like foo_40gmail_2ecom are directory names there, and then the tokens are the file names in these directories. I suppose we could move to that directory structure if we wanted to save both targetIds and tokens. I see that  ~/.cache/telepathy/avatars just has the tokens.

> > +        // info.token[handle] is the token for @handle's avatar
> > +        info.token = {};
> > 
> > Perhaps info.tokens is a more suitable name?
> 
> I tend towards using singular names for arrays if you refer to individual
> elements more than you refer to the array as a whole, because IMHO it reads
> better. "token[handle]" is the token for handle.

I think it is a confusing distinction and arrays/maps should generally be named as plurals, but ok :).

::: js/ui/telepathyClient.js
@@ +249,3 @@
+    _setIcon: function(iconBox, info, handle) {
+        let textureCache = St.TextureCache.get_default();
+        let token = info.token[handle], file;

I think it'd be clearer if we initialized file on a separate line.

Attachment 159625 [details] pushed as e836569 - telepathyClient: cache avatar images