GNOME Bugzilla – Bug 650414
Need better APIs to handle image data
Last modified: 2011-07-25 19:39:58 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.
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
(mass changing milestones)
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.
(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.
(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!
(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.
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.
Patches available for Empathy and gnome-contacts to port their avatar handling: bug #655212 and bug #655213.
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 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(-)