After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 650414 - Need better APIs to handle image data
Need better APIs to handle image data
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: folks-0.6.0
Assigned To: folks-maint
folks-maint
Depends on: 655154 655211
Blocks: 652721 653684 655212 655213
 
 
Reported: 2011-05-17 17:25 UTC by Alexander Larsson
Modified: 2011-07-25 19:39 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Change AvatarDetails.avatar to have type LoadableIcon (26.81 KB, patch)
2011-06-14 13:55 UTC, Philip Withnall
needs-work Details | Review
Change AvatarDetails.avatar to have type LoadableIcon (updated) (56.63 KB, patch)
2011-07-24 16:02 UTC, Philip Withnall
committed Details | Review

Description Alexander Larsson 2011-05-17 17:25:16 UTC
Fields that are images, like avatar are currently just available as essentially a pathname. This is a pretty bad representation in the API. Not all backends are storing each image in a separate file, so folks would have to copy out and sync the image data to separate files. And there are general problems with lifecycle control with out of band data files.

Its also no good having access to the raw data from the api, because the implies either sync i/o or folks loading all the data for all contacts.

The proper API needs to expose a way for applications to on-demand get the data (sync or async) when/if they need it. In GIO this can be done using the GIcon API. The field returns a GIcon which is an abstract type that can be backed by e.g. GFileIcon or a GLoadableIcon subclass that e.g. streams directly from a database.
Comment 1 Philip Withnall 2011-06-14 13:55:54 UTC
Created attachment 189907 [details] [review]
Change AvatarDetails.avatar to have type LoadableIcon

http://cgit.collabora.com/git/user/pwith/folks/log/?h=650414-gicon-avatars
Comment 2 Travis Reitter 2011-06-21 14:55:48 UTC
(mass changing milestones)
Comment 3 Travis Reitter 2011-07-19 20:24:08 UTC
Review of attachment 189907 [details] [review]:

+  /**
+   * Fetch an avatar from the cache by its globally unique ID, if the avatar
+   * exists in the cache. If the avatar doesn't exist in the cache, `null` will
+   * be returned.
+   *
+   * @param id the globally unique ID for the avatar
+   * @since 0.5.UNRELEASED
+   */
+  public async LoadableIcon? load_avatar (string id) throws GLib.Error

Simplify the main comment by use of @return.


+   * For performance reasons, if the provided avatar is a {@link FileIcon}, the
+   * cache may symlink to the existing avatar file. This won't affect the avata
+   * URI returned by this method.

This should probably be a hard link or copy so we can guarantee the lifetime of the icon for our client.


+++ b/tools/inspect/utils.vala
@@ -264,9 +264,9 @@ private class Folks.Inspect.Utils
       else if (prop_name == "avatar")
         {
           string ret = null;
-          File avatar = (File) prop_value.get_object ();
+          LoadableIcon? avatar = (LoadableIcon) prop_value.get_object ();
           if (avatar != null)
-            ret = avatar.get_uri ();
+            ret = "%p".printf (avatar);

I think the URI would be more useful here.


Please port the relevant Tracker tests to the EDS backend, since we'll be removing the Tracker backend from the main source tree within a couple releases or so.
Comment 4 Philip Withnall 2011-07-21 19:41:59 UTC
(In reply to comment #3)
> Review of attachment 189907 [details] [review]:
> 
> +  /**
> +   * Fetch an avatar from the cache by its globally unique ID, if the avatar
> +   * exists in the cache. If the avatar doesn't exist in the cache, `null`
> will
> +   * be returned.
> +   *
> +   * @param id the globally unique ID for the avatar
> +   * @since 0.5.UNRELEASED
> +   */
> +  public async LoadableIcon? load_avatar (string id) throws GLib.Error
> 
> Simplify the main comment by use of @return.

Whoops, fixed, and the other cases in AvatarCache where @return was missing.

> +   * For performance reasons, if the provided avatar is a {@link FileIcon},
> the
> +   * cache may symlink to the existing avatar file. This won't affect the
> avata
> +   * URI returned by this method.
> 
> This should probably be a hard link or copy so we can guarantee the lifetime of
> the icon for our client.

I hadn't thought about this before, but if we use hard links we limit ourselves to having the cache on the same file system as whatever file is provided to us by the backend, which might not be the case. For example, the backend could return a default avatar from the system partition, or something.

Added to that, GIO doesn't allow creation of hard links (as far as I know), so we'd have to break out of GIO and use libc, which feels nasty.

I'm open to persuasion, though.

> +++ b/tools/inspect/utils.vala
> @@ -264,9 +264,9 @@ private class Folks.Inspect.Utils
>        else if (prop_name == "avatar")
>          {
>            string ret = null;
> -          File avatar = (File) prop_value.get_object ();
> +          LoadableIcon? avatar = (LoadableIcon) prop_value.get_object ();
>            if (avatar != null)
> -            ret = avatar.get_uri ();
> +            ret = "%p".printf (avatar);
> 
> I think the URI would be more useful here.

We can only have the URI if the LoadableIcon is also a FileIcon. I've added a check to see if it is — and if so, the URI (and address) will be printed. If not, just the address will be printed as above.

> Please port the relevant Tracker tests to the EDS backend, since we'll be
> removing the Tracker backend from the main source tree within a couple releases
> or so.

Will do. I'll try and get it all finished off tonight, modulo the issues above.
Comment 5 Travis Reitter 2011-07-21 21:19:36 UTC
(In reply to comment #4)

> > This should probably be a hard link or copy so we can guarantee the lifetime of
> > the icon for our client.
> 
> I hadn't thought about this before, but if we use hard links we limit ourselves
> to having the cache on the same file system as whatever file is provided to us
> by the backend, which might not be the case. For example, the backend could
> return a default avatar from the system partition, or something.

I suppose this is possible, so let's just make a copy.

> Added to that, GIO doesn't allow creation of hard links (as far as I know), so
> we'd have to break out of GIO and use libc, which feels nasty.

Huh - interesting that it doesn't support hard links. I guess it's trying to be as portable as possible.

> I'm open to persuasion, though.

I think the copying route is our best option.

> > +++ b/tools/inspect/utils.vala
> > @@ -264,9 +264,9 @@ private class Folks.Inspect.Utils
> >        else if (prop_name == "avatar")
> >          {
> >            string ret = null;
> > -          File avatar = (File) prop_value.get_object ();
> > +          LoadableIcon? avatar = (LoadableIcon) prop_value.get_object ();
> >            if (avatar != null)
> > -            ret = avatar.get_uri ();
> > +            ret = "%p".printf (avatar);
> > 
> > I think the URI would be more useful here.
> 
> We can only have the URI if the LoadableIcon is also a FileIcon. I've added a
> check to see if it is — and if so, the URI (and address) will be printed. If
> not, just the address will be printed as above.

Sounds fine.

> > Please port the relevant Tracker tests to the EDS backend, since we'll be
> > removing the Tracker backend from the main source tree within a couple releases
> > or so.
> 
> Will do. I'll try and get it all finished off tonight, modulo the issues above.

Thanks!
Comment 6 Philip Withnall 2011-07-22 23:04:19 UTC
(In reply to comment #5)
> (In reply to comment #4)
> 
> > > This should probably be a hard link or copy so we can guarantee the lifetime of
> > > the icon for our client.
> > 
> > I hadn't thought about this before, but if we use hard links we limit ourselves
> > to having the cache on the same file system as whatever file is provided to us
> > by the backend, which might not be the case. For example, the backend could
> > return a default avatar from the system partition, or something.
> 
> I suppose this is possible, so let's just make a copy.
> 
> > Added to that, GIO doesn't allow creation of hard links (as far as I know), so
> > we'd have to break out of GIO and use libc, which feels nasty.
> 
> Huh - interesting that it doesn't support hard links. I guess it's trying to be
> as portable as possible.
> 
> > I'm open to persuasion, though.
> 
> I think the copying route is our best option.

Agreed. The updated patch will do this.

> > > Please port the relevant Tracker tests to the EDS backend, since we'll be
> > > removing the Tracker backend from the main source tree within a couple releases
> > > or so.
> > 
> > Will do. I'll try and get it all finished off tonight, modulo the issues above.
> 
> Thanks!

Finishing off the patch took a lot longer than expected, and I hit some Vala binding bugs on the way (bug #655154). I've finished the patch but haven't had a chance to test it yet, so I'll attach it here once I have.
Comment 7 Philip Withnall 2011-07-24 16:02:24 UTC
Created attachment 192562 [details] [review]
Change AvatarDetails.avatar to have type LoadableIcon (updated)

https://www.gitorious.org/folks/folks/commits/650414-gicon-avatars-rebase2

Massively updated branch. This implements some unit tests for AvatarCache, adds eds support and fixes a number of bugs (including the ones mentioned above). It should be ready to merge (bar any problems found in review).

It depends on the following bugs, both in Vala bindings: bug #655154 and bug #655211.
Comment 8 Philip Withnall 2011-07-24 16:21:05 UTC
Patches available for Empathy and gnome-contacts to port their avatar handling: bug #655212 and bug #655213.
Comment 9 Travis Reitter 2011-07-25 18:01:29 UTC
Review of attachment 192562 [details] [review]:

Edsf.MemoryIcon:

+  public uint hash ()
+  ...
+      for (uint i = 0; i < this._image_data.length; i++)
+        {
+          hash = (hash << 5) + hash + this._image_data[i];
+        }

It really isn't clear to me what's going on here (even after reading the implementation docs for g_str_hash()). Could you please add a bit more detail in a comment block?


+  public void test_remove_avatar ()
+    {
...
+      assert (avatar == null);
+      
+    }

The second-to-last line has several spaces before the newline - just cut it.


Other than that, this looks really good. Please merge once bug #655154 and bug #655211 are fixed (and ping the Vala devs as necessary).
Comment 10 Philip Withnall 2011-07-25 19:39:50 UTC
Comment on attachment 192562 [details] [review]
Change AvatarDetails.avatar to have type LoadableIcon (updated)

Fixed and merged. I haven't bumped our Vala dependency, but that will need doing before release.

commit 3862f876a9bef82f4fcf838a00c81ef5c57ae353
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Mon Jun 13 10:54:23 2011 +0100

    Bug 650414 — Need better APIs to handle image data
    
    Change AvatarDetails.avatar to have type LoadableIcon. By introducing
    a libfolks-wide avatar cache, propagate this change to all the backends.
    
    This breaks the API of AvatarDetails.
    
    Closes: bgo#650414

 NEWS                                        |    3 +
 backends/eds/lib/Makefile.am                |    1 +
 backends/eds/lib/edsf-persona-store.vala    |   89 +++------
 backends/eds/lib/edsf-persona.vala          |  155 +++++++--------
 backends/eds/lib/memory-icon.vala           |  133 ++++++++++++
 backends/libsocialweb/lib/swf-persona.vala  |   14 +-
 backends/telepathy/lib/tpf-persona.vala     |   13 +-
 backends/tracker/lib/trf-persona-store.vala |   56 ++++-
 backends/tracker/lib/trf-persona.vala       |   24 ++-
 folks/Makefile.am                           |    1 +
 folks/avatar-cache.vala                     |  222 ++++++++++++++++++++
 folks/avatar-details.vala                   |    7 +-
 folks/individual.vala                       |    8 +-
 tests/eds/add-persona.vala                  |   23 +--
 tests/eds/avatar-details.vala               |   20 +--
 tests/eds/set-avatar.vala                   |   23 +--
 tests/folks/Makefile.am                     |    8 +
 tests/folks/avatar-cache.vala               |  301 +++++++++++++++++++++++++++
 tests/tracker/add-persona.vala              |    9 +-
 tests/tracker/avatar-details-interface.vala |   34 +---
 tests/tracker/avatar-updates.vala           |   27 ++-
 tests/tracker/set-avatar.vala               |    6 +-
 tools/inspect/utils.vala                    |   16 ++-
 23 files changed, 915 insertions(+), 278 deletions(-)