changeset 2123:c42aab22c2c0

plugin XEP-0054, quick frontend(app): various improvments: - memory.cache is now used - getAvatar and setAvatar has been renamed to avatarGet and avatarSet to follow new convention - getAvatar now return (optionally) full path, and may request vCard or only return avatar in cache - getAvatarFile has been removed as it is not needed anymore (avatarGet can return full path) - getCard has been removed from bridge as it was only used to request avatar - new internal method getBareOfFull return jid to use depending of the jid being part of a MUC or not - cache is now set per profile in client, instead of a general one for all profiles - thanks to the use of memory.cache, correct extension is now used in saved file, according to MIME type - fixed and better cache handling - a warning message is shown if given avatar hash differs from computed one - empty hash value is now in a constant, and ignored if received - QuickApp has been updated to follow new behaviour - Primitivus has been fixed (it was not declaring not using avatars correclty) - jp has been updated to follow new methods name
author Goffi <goffi@goffi.org>
date Sun, 15 Jan 2017 17:51:37 +0100 (2017-01-15)
parents 3970ebcf8830
children cf63e4209643
files frontends/src/jp/cmd_avatar.py frontends/src/primitivus/primitivus frontends/src/quick_frontend/quick_app.py frontends/src/quick_frontend/quick_chat.py frontends/src/quick_frontend/quick_contact_list.py src/core/constants.py src/plugins/plugin_xep_0054.py
diffstat 7 files changed, 332 insertions(+), 223 deletions(-) [+]
line wrap: on
line diff
--- a/frontends/src/jp/cmd_avatar.py	Sun Jan 15 16:00:41 2017 +0100
+++ b/frontends/src/jp/cmd_avatar.py	Sun Jan 15 17:51:37 2017 +0100
@@ -46,7 +46,7 @@
             self.disp(_(u"file [{}] doesn't exist !").format(path), error=True)
             self.host.quit(1)
         path = os.path.abspath(path)
-        self.host.bridge.setAvatar(path, self.profile, callback=self._avatarCb, errback=self._avatarEb)
+        self.host.bridge.avatarSet(path, self.profile, callback=self._avatarCb, errback=self._avatarEb)
 
     def _avatarCb(self):
         self.disp(_("avatar has been set"), 1)
@@ -85,7 +85,7 @@
             import webbrowser
             webbrowser.open(path)
 
-    def _getAvatarCb(self, avatar_path):
+    def _avatarGetCb(self, avatar_path):
         if not avatar_path:
             self.disp(_(u"No avatar found."), 1)
             self.host.quit(C.EXIT_NOT_FOUND)
@@ -96,12 +96,12 @@
 
         self.host.quit()
 
-    def _getAvatarEb(self, failure_):
+    def _avatarGetEb(self, failure_):
         self.disp(_("error while getting avatar: {msg}").format(msg=failure_), error=True)
         self.host.quit(C.EXIT_ERROR)
 
     def start(self):
-        self.host.bridge.getAvatar(self.args.jid, self.profile, callback=self._getAvatarCb, errback=self._getAvatarEb)
+        self.host.bridge.avatarGet(self.args.jid, False, False, self.profile, callback=self._avatarGetCb, errback=self._avatarGetEb)
 
 
 class Avatar(base.CommandBase):
--- a/frontends/src/primitivus/primitivus	Sun Jan 15 16:00:41 2017 +0100
+++ b/frontends/src/primitivus/primitivus	Sun Jan 15 17:51:37 2017 +0100
@@ -280,7 +280,7 @@
 
 class PrimitivusApp(QuickApp, InputHistory):
     MB_HANDLER = False
-    AVATAR_HANDLER = False
+    AVATARS_HANDLER = False
 
     def __init__(self):
         bridge_module = dynamic_import.bridge(bridge_name, 'sat_frontends.bridge')
--- a/frontends/src/quick_frontend/quick_app.py	Sun Jan 15 16:00:41 2017 +0100
+++ b/frontends/src/quick_frontend/quick_app.py	Sun Jan 15 17:51:37 2017 +0100
@@ -50,7 +50,7 @@
     #       and a way to keep some XMLUI request between sessions is expected in backend
     host = None
     bridge = None
-    cache_keys_to_get = ['avatar']
+    # cache_keys_to_get = ['avatar']
 
     def __init__(self, profile):
         self.profile = profile
@@ -106,7 +106,10 @@
 
     def _plug_profile_getFeaturesCb(self, features):
         self.host.features = features
-        self.host.bridge.getEntitiesData([], ProfileManager.cache_keys_to_get, profile=self.profile, callback=self._plug_profile_gotCachedValues, errback=self._plug_profile_failedCachedValues)
+        # FIXME: we don't use cached value at the moment, but keep the code for later use
+        #        it was previously used for avatars, but as we don't get full path here, it's better to request later
+        # self.host.bridge.getEntitiesData([], ProfileManager.cache_keys_to_get, profile=self.profile, callback=self._plug_profile_gotCachedValues, errback=self._plug_profile_failedCachedValues)
+        self._plug_profile_gotCachedValues({})
 
     def _plug_profile_failedCachedValues(self, failure):
         log.error(u"Couldn't get cached values: {}".format(failure))
@@ -817,15 +820,9 @@
             if entity in self.contact_lists[profile]:
                 self.contact_lists[profile].setCache(entity, 'nick', value)
                 self.callListeners('nick', entity, value, profile=profile)
-        elif key == "avatar":
-            if value: # and entity in self.contact_lists[profile]:
-                # FIXME: contact_list check is removed to work around 'avatar' issues
-                #        (avatar can't be requested if we don't save them here)
-                #        avatar need to be refactored properly in backend and here
-                def gotFilename(filename):
-                    self.contact_lists[profile].setCache(entity, 'avatar', filename)
-                    self.callListeners('avatar', entity, filename, profile=profile)
-                self.bridge.getAvatarFile(value, callback=gotFilename)
+        elif key == "avatar" and self.AVATARS_HANDLER:
+            if value and entity in self.contact_lists[profile]:
+                self.getAvatar(entity, ignore_cache=True, profile=profile)
 
     def actionManager(self, action_data, callback=None, ui_show_cb=None, user_action=True, profile=C.PROF_KEY_NONE):
         """Handle backend action
@@ -886,15 +883,42 @@
 
         self.bridge.launchAction(callback_id, data, profile, callback=action_cb, errback=self.dialogFailure)
 
-    def getAvatar(self, entity, profile):
-        """return avatar path for an entity"""
+    def _avatarGetCb(self, avatar_path, entity, contact_list, profile):
+        path = avatar_path or self.getDefaultAvatar(entity)
+        contact_list.setCache(entity, "avatar", path)
+        self.callListeners('avatar', entity, path, profile=profile)
+
+    def _avatarGetEb(self, failure, entity, contact_list):
+        log.warning(u"Can't get avatar: {}".format(failure))
+        contact_list.setCache(entity, "avatar", self.getDefaultAvatar(entity))
+
+    def getAvatar(self, entity, cache_only=True, hash_only=False, ignore_cache=False, profile=C.PROF_KEY_NONE):
+        """return avatar path for an entity
+
+        @param entity(jid.JID): entity to get avatar from
+        @param cache_only(bool): if False avatar will be requested if not in cache
+            with current vCard based implementation, it's better to keep True
+            except if we request avatars for roster items
+        @param hash_only(bool): if True avatar hash is returned, else full path
+        @param ignore_cache(bool): if False, won't check local cache and will request backend in every case
+        @return (unicode, None): avatar full path (None if no avatar found)
+        """
         contact_list = self.contact_lists[profile]
-        avatar = contact_list.getCache(entity, "avatar")
+        if ignore_cache:
+            avatar = None
+        else:
+            avatar = contact_list.getCache(entity, "avatar", bare_default=None)
         if avatar is None:
-            self.bridge.getCard(unicode(entity), profile)
+            self.bridge.avatarGet(
+                unicode(entity),
+                cache_only,
+                hash_only,
+                profile=profile,
+                callback=lambda path: self._avatarGetCb(path, entity, contact_list, profile),
+                errback=lambda failure: self._avatarGetEb(failure, entity, contact_list))
+            # we set avatar to empty string to avoid requesting several time the same avatar
+            # while we are waiting for avatarGet result
             contact_list.setCache(entity, "avatar", "")
-        if not avatar:
-            avatar = self.getDefaultAvatar(entity)
         return avatar
 
     def getDefaultAvatar(self, entity=None):
--- a/frontends/src/quick_frontend/quick_chat.py	Sun Jan 15 16:00:41 2017 +0100
+++ b/frontends/src/quick_frontend/quick_chat.py	Sun Jan 15 17:51:37 2017 +0100
@@ -126,7 +126,9 @@
 
     @property
     def avatar(self):
-        return self.host.getAvatar(self.from_jid, self.profile)
+        """avatar full path or None if no avatar is found"""
+        ret = self.host.getAvatar(self.from_jid, profile=self.profile)
+        return ret
 
     def getNick(self, entity):
         """Return nick of an entity when possible"""
@@ -583,16 +585,25 @@
 
     def onAvatar(self, entity, filename, profile):
         if self.type == C.CHAT_GROUP:
-            if entity.bare == self.target and entity.resource in self.occupants:
-                self.occupants[entity.resource].update({'avatar': filename})
+            if entity.bare == self.target:
+                try:
+                    self.occupants[entity.resource].update({'avatar': filename})
+                except KeyError:
+                    # can happen for a message in history where the
+                    # entity is not here anymore
+                    pass
+
                 for m in self.messages.values():
                     if m.nick == entity.resource:
                         for w in m.widgets:
                             w.update({'avatar': filename})
         else:
-            if entity.bare == self.target.bare:
-                log.info("entity avatar updated")
-                # TODO: call a specific method
+            if entity.bare == self.target.bare or entity.bare == self.host.profiles[profile].whoami.bare:
+                log.info(u"avatar updated for {}".format(entity))
+                for m in self.messages.values():
+                    if m.from_jid.bare == entity.bare:
+                        for w in m.widgets:
+                            w.update({'avatar': filename})
 
 
 quick_widgets.register(QuickChat)
--- a/frontends/src/quick_frontend/quick_contact_list.py	Sun Jan 15 16:00:41 2017 +0100
+++ b/frontends/src/quick_frontend/quick_contact_list.py	Sun Jan 15 17:51:37 2017 +0100
@@ -219,6 +219,9 @@
         @return: full cache if no name is given, or value of "name", or None
         """
         # FIXME: resource handling need to be reworked
+        # FIXME: bare_default work for requesting full jid to get bare jid, but not the other way
+        #        e.g.: if we have set an avatar for user@server.tld/resource and we request user@server.tld
+        #        we won't get the avatar set in the resource
         try:
             cache = self._cache[entity.bare]
         except KeyError:
--- a/src/core/constants.py	Sun Jan 15 16:00:41 2017 +0100
+++ b/src/core/constants.py	Sun Jan 15 17:51:37 2017 +0100
@@ -285,6 +285,7 @@
     IGNORE = 'ignore'
     NO_LIMIT = -1 # used in bridge when a integer value is expected
     DEFAULT_MAX_AGE = 1209600 # default max age of cached files, in seconds
+    HASH_SHA1_EMPTY = 'da39a3ee5e6b4b0d3255bfef95601890afd80709'
 
 
     ## ANSI escape sequences ##
--- a/src/plugins/plugin_xep_0054.py	Sun Jan 15 16:00:41 2017 +0100
+++ b/src/plugins/plugin_xep_0054.py	Sun Jan 15 17:51:37 2017 +0100
@@ -24,10 +24,8 @@
 log = getLogger(__name__)
 from twisted.internet import threads, defer
 from twisted.words.protocols.jabber import jid
-from twisted.words.protocols.jabber.xmlstream import IQ
 from twisted.words.xish import domish
 from twisted.python.failure import Failure
-import os.path
 
 from zope.interface import implements
 
@@ -37,6 +35,7 @@
 from hashlib import sha1
 from sat.core import exceptions
 from sat.memory import persistent
+import mimetypes
 try:
     from PIL import Image
 except:
@@ -49,7 +48,7 @@
     from wokkel.subprotocols import XMPPHandler
 
 AVATAR_PATH = "avatars"
-AVATAR_DIM = (64, 64)
+AVATAR_DIM = (64, 64)  # FIXME: dim are not adapted to modern resolutions !
 
 IQ_GET = '/iq[@type="get"]'
 NS_VCARD = 'vcard-temp'
@@ -60,6 +59,7 @@
 VCARD_UPDATE = PRESENCE + '/x[@xmlns="' + NS_VCARD_UPDATE + '"]'
 
 CACHED_DATA = {'avatar', 'nick'}
+MAX_AGE = 60 * 60 * 24 * 365
 
 PLUGIN_INFO = {
     "name": "XEP 0054 Plugin",
@@ -80,36 +80,17 @@
     #      - get missing values
 
     def __init__(self, host):
-        log.info(_("Plugin XEP_0054 initialization"))
+        log.info(_(u"Plugin XEP_0054 initialization"))
         self.host = host
-        self.avatar_path = os.path.join(self.host.memory.getConfig('', 'local_dir'), AVATAR_PATH)
-        if not os.path.exists(self.avatar_path):
-            os.makedirs(self.avatar_path)
-        self.cache = {}
-        host.bridge.addMethod("getCard", ".plugin", in_sign='ss', out_sign='', method=self._getCard, async=True)
-        host.bridge.addMethod("getAvatarFile", ".plugin", in_sign='s', out_sign='s', method=self.getAvatarFile)
-        host.bridge.addMethod("getAvatar", ".plugin", in_sign='ss', out_sign='s', method=self._getAvatar, async=True)
-        host.bridge.addMethod("setAvatar", ".plugin", in_sign='ss', out_sign='', method=self.setAvatar, async=True)
-        host.trigger.add("presence_available", self.presenceAvailableTrigger)
-        host.memory.setSignalOnUpdate("avatar")
-        host.memory.setSignalOnUpdate("nick")
+        host.bridge.addMethod(u"avatarGet", u".plugin", in_sign=u'sbbs', out_sign=u's', method=self._getAvatar, async=True)
+        host.bridge.addMethod(u"avatarSet", u".plugin", in_sign=u'ss', out_sign=u'', method=self._setAvatar, async=True)
+        host.trigger.add(u"presence_available", self.presenceAvailableTrigger)
+        host.memory.setSignalOnUpdate(u"avatar")
+        host.memory.setSignalOnUpdate(u"nick")
 
     def getHandler(self, profile):
         return XEP_0054_handler(self)
 
-    def presenceAvailableTrigger(self, presence_elt, client):
-        if client.jid.userhost() in self.cache[client.profile]:
-            try:
-                avatar_hash = self.cache[client.profile][client.jid.userhost()]['avatar']
-            except KeyError:
-                log.info(u"No avatar in cache for {}".format(client.jid.userhost()))
-                return True
-            x_elt = domish.Element((NS_VCARD_UPDATE, 'x'))
-            x_elt.addElement('photo', content=avatar_hash)
-            presence_elt.addChild(x_elt)
-
-        return True
-
     def isRoom(self, client, entity_jid):
         """Tell if a jid is a MUC one
 
@@ -123,48 +104,86 @@
         else:
             return True
 
+    def getBareOrFull(self, client, jid_):
+        """use full jid if jid_ is an occupant of a room, bare jid else
+
+        @param jid_(jid.JID): entity to test
+        @return (jid.JID): bare or full jid
+        """
+        if jid_.resource:
+            if not self.isRoom(client, jid_):
+                return jid_.userhostJID()
+        return jid_
+
+    def presenceAvailableTrigger(self, presence_elt, client):
+        if client.jid.userhost() in client._cache_0054:
+            try:
+                avatar_hash = client._cache_0054[client.jid.userhost()]['avatar']
+            except KeyError:
+                log.info(u"No avatar in cache for {}".format(client.jid.userhost()))
+                return True
+            x_elt = domish.Element((NS_VCARD_UPDATE, 'x'))
+            x_elt.addElement('photo', content=avatar_hash)
+            presence_elt.addChild(x_elt)
+        return True
+
+    @defer.inlineCallbacks
+    def profileConnecting(self, profile):
+        client = self.host.getClient(profile)
+        client._cache_0054 = persistent.PersistentBinaryDict(NS_VCARD, profile)
+        yield client._cache_0054.load()
+        self._fillCachedValues(profile)
+
     def _fillCachedValues(self, profile):
-        #FIXME: this is really suboptimal, need to be reworked
-        #       the current naive approach keeps a map between all jids of all profiles
+        #FIXME: this may need to be reworked
+        #       the current naive approach keeps a map between all jids
         #       in persistent cache, then put avatar hashs in memory.
-        #       Hashes should be shared between profiles
-        for jid_s, data in self.cache[profile].iteritems():
+        #       Hashes should be shared between profiles (or not ? what
+        #       if the avatar is different depending on who is requesting it
+        #       this is not possible with vcard-tmp, but it is with XEP-0084).
+        #       Loading avatar on demand per jid may be a option to investigate.
+        client = self.host.getClient(profile)
+        for jid_s, data in client._cache_0054.iteritems():
             jid_ = jid.JID(jid_s)
             for name in CACHED_DATA:
                 try:
+                    value = data[name]
+                    if value is None:
+                        log.error(u"{name} value for {jid_} is None, ignoring".format(name=name, jid_=jid_))
+                        continue
                     self.host.memory.updateEntityData(jid_, name, data[name], silent=True, profile_key=profile)
                 except KeyError:
                     pass
 
-    @defer.inlineCallbacks
-    def profileConnecting(self, profile):
-        self.cache[profile] = persistent.PersistentBinaryDict(NS_VCARD, profile)
-        yield self.cache[profile].load()
-        self._fillCachedValues(profile)
-
-    def profileDisconnected(self, profile):
-        log.debug(u"Deleting profile cache for avatars")
-        del self.cache[profile]
-
     def updateCache(self, client, jid_, name, value):
         """update cache value
 
         save value in memory in case of change
         @param jid_(jid.JID): jid of the owner of the vcard
         @param name(str): name of the item which changed
-        @param value(unicode): new value of the item
+        @param value(unicode, None): new value of the item
+            None to delete
         """
-        if jid_.resource:
-            if not self.isRoom(client, jid_):
-                # VCard are retrieved with bare jid
-                # but MUC room is a special case
-                jid_ = jid.userhostJID()
+        jid_ = self.getBareOrFull(client, jid_)
+        jid_s = jid_.full()
 
-        self.host.memory.updateEntityData(jid_, name, value, profile_key=client.profile)
-        if name in CACHED_DATA:
-            jid_s = jid_.userhost()
-            self.cache[client.profile].setdefault(jid_s, {})[name] = value
-            self.cache[client.profile].force(jid_s)
+        if value is None:
+            try:
+                self.host.memory.delEntityDatum(jid_, name, client.profile)
+            except (KeyError, exceptions.UnknownEntityError):
+                pass
+            if name in CACHED_DATA:
+                try:
+                    del client._cache_0054[jid_s][name]
+                except KeyError:
+                    pass
+                else:
+                    client._cache_0054.force(jid_s)
+        else:
+            self.host.memory.updateEntityData(jid_, name, value, profile_key=client.profile)
+            if name in CACHED_DATA:
+                client._cache_0054.setdefault(jid_s, {})[name] = value
+                client._cache_0054.force(jid_s)
 
     def getCache(self, client, entity_jid, name):
         """return cached value for jid
@@ -172,105 +191,106 @@
         @param entity_jid: target contact
         @param name: name of the value ('nick' or 'avatar')
         @return: wanted value or None"""
-        if entity_jid.resource:
-            if not self.isRoom(client, entity_jid):
-                # VCard are retrieved with bare jid
-                # but MUC room is a special case
-                entity_jid = jid.userhostJID()
+        entity_jid = self.getBareOrFull(client, entity_jid)
         try:
             data = self.host.memory.getEntityData(entity_jid, [name], client.profile)
         except exceptions.UnknownEntityError:
             return None
         return data.get(name)
 
-    def _getFilename(self, hash_):
-        """Get filename from hash
-
-        @param hash_: hash of the avatar
-        @return (str): absolute filename of the avatar
-        """
-        return os.path.join(self.avatar_path, hash_)
-
-    def saveAvatarFile(self, data, hash_):
-        """Save the avatar picture if it doesn't already exists
-
-        @param data(str): binary image of the avatar
-        @param hash_(str): hash of the binary data (will be used for the filename)
-        """
-        filename = self._getFilename(hash_)
-        if not os.path.exists(filename):
-            with open(filename, 'wb') as file_:
-                file_.write(data)
-            log.debug(_(u"file saved to %s") % hash_)
+    def savePhoto(self, client, photo_elt, entity_jid):
+        """Parse a <PHOTO> photo_elt and save the picture"""
+        # XXX: this method is launched in a separate thread
+        try:
+            mime_type = unicode(photo_elt.elements(NS_VCARD, 'TYPE').next())
+        except StopIteration:
+            log.warning(u"no MIME type found, assuming image/png")
+            mime_type = u"image/png"
         else:
-            log.debug(_(u"file [%s] already in cache") % hash_)
+            if not mime_type:
+                log.warning(u"empty MIME type, assuming image/png")
+                mime_type = u"image/png"
+            elif mime_type not in ("image/gif", "image/jpeg", "image/png"):
+                if mime_type == "image/x-png":
+                    # XXX: this old MIME type is still used by some clients
+                    mime_type = "image/png"
+                else:
+                    # TODO: handle other image formats (svg?)
+                    log.warning(u"following avatar image format is not handled: {type} [{jid}]".format(
+                        type=mime_type, jid=entity_jid.full()))
+                    raise Failure(exceptions.DataError())
 
-    def savePhoto(self, photo_elt, target):
-        """Parse a <PHOTO> photo_elt and save the picture"""
+            ext = mimetypes.guess_extension(mime_type, strict=False)
+            assert ext is not None
+            log.debug(u'photo of type {type} with extension {ext} found [{jid}]'.format(
+                    type=mime_type, ext=ext, jid=entity_jid.full()))
         try:
-            type_ = unicode(photo_elt.elements(NS_VCARD, 'TYPE').next())
+            buf = str(photo_elt.elements(NS_VCARD, 'BINVAL').next())
         except StopIteration:
-            type_ = None
-        else:
-            if type_ and type_ not in ("image/gif", "image/jpeg", "image/png"):
-                # TODO: handle other image formats (svg?)
-                log.warning(u"following avatar image format is not handled: {type} [{jid}]".format(
-                    type=type_, jid=target.full()))
-                return
-            log.debug(u'Photo of type {type} found [{jid}]'.format(
-                    type=type_, jid=target.full()))
-        try:
-            bin_elt = photo_elt.elements(NS_VCARD, 'BINVAL').next()
-        except StopIteration:
-            return
-        buf = str(bin_elt)
+            log.warning(u"BINVAL element not found")
+            raise Failure(exceptions.NotFound())
         if not buf:
-            log.warning(u"empty avatar for {jid}".format(jid=target.full()))
-            return
+            log.warning(u"empty avatar for {jid}".format(jid=entity_jid.full()))
+            raise Failure(exceptions.NotFound())
         log.debug(_(u'Decoding binary'))
         decoded = b64decode(buf)
+        del buf
         image_hash = sha1(decoded).hexdigest()
-        self.saveAvatarFile(decoded, image_hash)
+        with client.cache.cacheData(
+            PLUGIN_INFO['import_name'],
+            image_hash,
+            mime_type,
+            # we keep in cache for 1 year
+            MAX_AGE
+            ) as f:
+            f.write(decoded)
         return image_hash
 
     @defer.inlineCallbacks
-    def vCard2Dict(self, client, vcard, target):
+    def vCard2Dict(self, client, vcard, entity_jid):
         """Convert a VCard to a dict, and save binaries"""
-        log.debug(_("parsing vcard"))
-        dictionary = {}
+        log.debug((u"parsing vcard"))
+        vcard_dict = {}
 
         for elem in vcard.elements():
             if elem.name == 'FN':
-                dictionary['fullname'] = unicode(elem)
+                vcard_dict['fullname'] = unicode(elem)
             elif elem.name == 'NICKNAME':
-                dictionary['nick'] = unicode(elem)
-                self.updateCache(client, target, 'nick', dictionary['nick'])
+                vcard_dict['nick'] = unicode(elem)
+                self.updateCache(client, entity_jid, 'nick', vcard_dict['nick'])
             elif elem.name == 'URL':
-                dictionary['website'] = unicode(elem)
+                vcard_dict['website'] = unicode(elem)
             elif elem.name == 'EMAIL':
-                dictionary['email'] = unicode(elem)
+                vcard_dict['email'] = unicode(elem)
             elif elem.name == 'BDAY':
-                dictionary['birthday'] = unicode(elem)
+                vcard_dict['birthday'] = unicode(elem)
             elif elem.name == 'PHOTO':
                 # TODO: handle EXTVAL
-                avatar_hash = yield threads.deferToThread(self.savePhoto, elem, target)
-                if avatar_hash is None: # can happen e.g. in case of empty photo elem
-                    continue
-                dictionary['avatar'] = avatar_hash
-                self.updateCache(client, target, 'avatar', dictionary['avatar'])
+                try:
+                    avatar_hash = yield threads.deferToThread(
+                        self.savePhoto, client, elem, entity_jid)
+                except (exceptions.DataError, exceptions.NotFound) as e:
+                    avatar_hash = ''
+                    vcard_dict['avatar'] = avatar_hash
+                except Exception as e:
+                    log.error(u"avatar saving error: {}".format(e))
+                    avatar_hash = None
+                else:
+                    vcard_dict['avatar'] = avatar_hash
+                self.updateCache(client, entity_jid, 'avatar', avatar_hash)
             else:
-                log.debug(_('FIXME: [%s] VCard tag is not managed yet') % elem.name)
+                log.debug(u'FIXME: [{}] VCard tag is not managed yet'.format(elem.name))
 
-        # if a data in cache doesn't exist anymore, we need to reset it
-        # so we check CACHED_DATA no gotten (i.e. not in dictionary keys)
+        # if a data in cache doesn't exist anymore, we need to delete it
+        # so we check CACHED_DATA no gotten (i.e. not in vcard_dict keys)
         # and we reset them
-        for datum in CACHED_DATA.difference(dictionary.keys()):
-            log.debug(u"reseting vcard datum [{datum}] for {entity}".format(datum=datum, entity=target.full()))
-            self.updateCache(client, target, datum, '')
+        for datum in CACHED_DATA.difference(vcard_dict.keys()):
+            log.debug(u"reseting vcard datum [{datum}] for {entity}".format(datum=datum, entity=entity_jid.full()))
+            self.updateCache(client, entity_jid, datum, None)
 
-        defer.returnValue(dictionary)
+        defer.returnValue(vcard_dict)
 
-    def _getCardCb(self, iq_elt, to_jid, client):
+    def _vCardCb(self, iq_elt, to_jid, client):
         """Called after the first get IQ"""
         log.debug(_("VCard found"))
 
@@ -286,71 +306,89 @@
         d = self.vCard2Dict(client, vcard_elt, from_jid)
         return d
 
-    def _getCardEb(self, failure_, to_jid, client):
+    def _vCardEb(self, failure_, to_jid, client):
         """Called when something is wrong with registration"""
         log.warning(u"Can't get vCard for {jid}: {failure}".format(jid=to_jid.full, failure=failure_))
-        self.updateCache(client, to_jid, "avatar", '')
+        self.updateCache(client, to_jid, "avatar", None)
 
-    def _getCard(self, target_s, profile_key=C.PROF_KEY_NONE):
-        client = self.host.getClient(profile_key)
-        return self.getCard(client, jid.JID(target_s))
-
-    def getCard(self, client, target):
+    def getCard(self, client, entity_jid):
         """Ask server for VCard
 
-        @param target(jid.JID): jid from which we want the VCard
+        @param entity_jid(jid.JID): jid from which we want the VCard
         @result: id to retrieve the profile
         """
-        target_bare = target.userhostJID()
-        to_jid = target if self.isRoom(client, target_bare) else target_bare
-        log.debug(_(u"Asking for %s's VCard") % to_jid.userhost())
+        entity_jid = self.getBareOrFull(client, entity_jid)
+        log.debug(u"Asking for {}'s VCard".format(entity_jid.full()))
         reg_request = client.IQ('get')
         reg_request["from"] = client.jid.full()
-        reg_request["to"] = to_jid.full()
+        reg_request["to"] = entity_jid.full()
         reg_request.addElement('vCard', NS_VCARD)
-        d = reg_request.send(to_jid.full()).addCallbacks(self._getCardCb, self._getCardEb, callbackArgs=[to_jid, client], errbackArgs=[to_jid, client])
+        d = reg_request.send(entity_jid.full()).addCallbacks(self._vCardCb, self._vCardEb, callbackArgs=[entity_jid, client], errbackArgs=[entity_jid, client])
+        return d
+
+    def _getCardCb(self, dummy, client, entity):
+        try:
+            return client._cache_0054[entity.full()]['avatar']
+        except KeyError:
+            raise Failure(exceptions.NotFound())
+
+    def _getAvatar(self, entity, cache_only, hash_only, profile):
+        client = self.host.getClient(profile)
+        d = self.getAvatar(client, jid.JID(entity))
+        d.addErrback(lambda dummy: '')
+
         return d
 
-    def getAvatarFile(self, avatar_hash):
-        """Give the full path of avatar from hash
-        @param avatar_hash(unicode): SHA1 hash
-        @return(unicode): full_path or empty string if avatar_hash is empty
-            or if avatar is not found in cache
+    def getAvatar(self, client, entity, cache_only=True, hash_only=False):
+        """get avatar full path or hash
+
+        if avatar is not in local cache, it will be requested to the server
+        @param entity(jid.JID): entity to get avatar from
+        @param cache_only(bool): if False, will request vCard if avatar is
+            not in cache
+        @param hash_only(bool): if True only return hash, not full path
+        @raise exceptions.NotFound: no avatar found
         """
-        if not avatar_hash:
-            return ""
-        filename = self.avatar_path + '/' + avatar_hash
-        if not os.path.exists(filename):
-            log.error(_(u"Asking for an uncached avatar [%s]") % avatar_hash)
-            return ""
-        return filename
+        entity = self.getBareOrFull(client, entity)
+        full_path = None
 
-    def _getAvatar(self, entity, profile):
-        client = self.host.getClient(profile)
-        return self.getAvatar(client, jid.JID(entity))
-
-    def _getAvatarGotCard(self, dummy, client, entity):
         try:
-            self.cache[client.profile][entity.full()]['avatar']
+            # we first check if we have avatar in cache
+            avatar_hash = client._cache_0054[entity.full()]['avatar']
+            if avatar_hash:
+                # avatar is known and exists
+                full_path = client.cache.getFilePath(avatar_hash)
+                if full_path is None:
+                    # cache file is not available (probably expired)
+                    raise KeyError
+            else:
+                # avatar has already been checked but it is not set
+                full_path = u''
         except KeyError:
-            return ""
+            # avatar is not in cache
+            if cache_only:
+                return defer.fail(Failure(exceptions.NotFound()))
+            # we request vCard to get avatar
+            d = self.getCard(client, entity)
+            d.addCallback(self._getCardCb, client, entity)
+        else:
+            # avatar is in cache, we can return hash
+            d = defer.succeed(avatar_hash)
 
-    def getAvatar(self, client, entity):
-        try:
-            avatar_hash = self.cache[client.profile][entity.full()]['avatar']
-        except KeyError:
-            d = self.getCard(client, entity)
-            d.addCallback(self._getAvatarGotCard, client, entity)
-        else:
-            d = defer.succeed(avatar_hash)
-        d.addCallback(self.getAvatarFile)
+        if not hash_only:
+            # full path is requested
+            if full_path is None:
+                d.addCallback(client.cache.getFilePath)
+            else:
+                d.addCallback(lambda dummy: full_path)
         return d
 
-    def _buildSetAvatar(self, vcard_set, filepath):
+    def _buildSetAvatar(self, client, vcard_set, file_path):
+        # XXX: this method is executed in a separate thread
         try:
-            img = Image.open(filepath)
+            img = Image.open(file_path)
         except IOError:
-            return Failure(exceptions.DataError("Can't open image"))
+            return Failure(exceptions.DataError(u"Can't open image"))
 
         if img.size != AVATAR_DIM:
             img.thumbnail(AVATAR_DIM, Image.ANTIALIAS)
@@ -372,24 +410,34 @@
         photo_elt = vcard_elt.addElement('PHOTO')
         photo_elt.addElement('TYPE', content='image/png')
         photo_elt.addElement('BINVAL', content=b64encode(img_buf.getvalue()))
-        img_hash = sha1(img_buf.getvalue()).hexdigest()
-        self.saveAvatarFile(img_buf.getvalue(), img_hash)
-        return (vcard_set, img_hash)
+        image_hash = sha1(img_buf.getvalue()).hexdigest()
+        with client.cache.cacheData(
+            PLUGIN_INFO['import_name'],
+            image_hash,
+            "image/png",
+            MAX_AGE
+            ) as f:
+            f.write(img_buf.getvalue())
+        return vcard_set, image_hash
 
-    def setAvatar(self, filepath, profile_key=C.PROF_KEY_NONE):
+    def _setAvatar(self, file_path, profile_key=C.PROF_KEY_NONE):
+        client = self.host.getClient(profile_key)
+        return self.setAvatar(client, file_path)
+
+    def setAvatar(self, client, file_path):
         """Set avatar of the profile
-        @param filepath: path of the image of the avatar"""
+
+        @param file_path: path of the image of the avatar
+        """
         #TODO: This is a temporary way of setting the avatar, as other VCard information is not managed.
         #      A proper full VCard management should be done (and more generaly a public/private profile)
-        client = self.host.getClient(profile_key)
-
-        vcard_set = IQ(client.xmlstream, 'set')
-        d = threads.deferToThread(self._buildSetAvatar, vcard_set, filepath)
+        vcard_set = client.IQ()
+        d = threads.deferToThread(self._buildSetAvatar, client, vcard_set, file_path)
 
         def elementBuilt(result):
             """Called once the image is at the right size/format, and the vcard set element is build"""
-            set_avatar_elt, img_hash = result
-            self.updateCache(client, client.jid.userhostJID(), 'avatar', img_hash)
+            set_avatar_elt, image_hash = result
+            self.updateCache(client, client.jid.userhostJID(), 'avatar', image_hash)
             return set_avatar_elt.send().addCallback(lambda ignore: client.presence.available()) # FIXME: should send the current presence, not always "available" !
 
         d.addCallback(elementBuilt)
@@ -413,15 +461,24 @@
     def getDiscoItems(self, requestor, target, nodeIdentifier=''):
         return []
 
+    def _checkAvatarHash(self, dummy, client, entity, given_hash):
+        """check that hash in cash (i.e. computed hash) is the same as given one"""
+        # XXX: if they differ, the avater will be requested on each connection
+        # TODO: try to avoid re-requesting avatar in this case
+        computed_hash = self.plugin_parent.getCache(client, entity, 'avatar')
+        if computed_hash != given_hash:
+            log.warning(u"computed hash differs from given hash for {entity}:\n"
+                "computed: {computed}\ngiven: {given}".format(
+                entity=entity, computed=computed_hash, given=given_hash))
+
     def update(self, presence):
         """Called on <presence/> stanza with vcard data
 
         Check for avatar information, and get VCard if needed
         @param presend(domish.Element): <presence/> stanza
         """
-        from_jid = jid.JID(presence['from'])
-        if from_jid.resource and not self.plugin_parent.isRoom(self.parent, from_jid):
-            from_jid = from_jid.userhostJID()
+        client = self.parent
+        entity_jid = self.plugin_parent.getBareOrFull(client, jid.JID(presence['from']))
         #FIXME: wokkel's data_form should be used here
         try:
             x_elt = presence.elements(NS_VCARD_UPDATE, 'x').next()
@@ -433,22 +490,35 @@
         except StopIteration:
             return
 
-        hash_ = str(photo_elt)
-        if not hash_:
-            return
-        old_avatar = self.plugin_parent.getCache(self.parent, from_jid, 'avatar')
-        filename = self.plugin_parent._getFilename(hash_)
-        if not old_avatar or old_avatar != hash_:
-            if os.path.exists(filename):
-                log.debug(u"New avatar found for [{}], it's already in cache, we use it".format(from_jid.full()))
-                self.plugin_parent.updateCache(self.parent, from_jid, 'avatar', hash_)
+        hash_ = unicode(photo_elt).strip()
+        if hash_ == C.HASH_SHA1_EMPTY:
+            hash_ = u''
+        old_avatar = self.plugin_parent.getCache(client, entity_jid, 'avatar')
+
+        if old_avatar == hash_:
+            # no change, we can return...
+            if hash_:
+                # ...but we double check that avatar is in cache
+                file_path = client.cache.getFilePath(hash_)
+                if file_path is None:
+                    log.error(u"Avatar for [{}] should be in cache but it is not! We get it".format(entity_jid.full()))
+                    self.plugin_parent.getCard(client, entity_jid)
             else:
-                log.debug(u'New avatar found for [{}], requesting vcard'.format(from_jid.full()))
-                self.plugin_parent.getCard(self.parent, from_jid)
+                log.debug(u"avatar for {} already in cache".format(entity_jid.full()))
+            return
+
+        if not hash_:
+            # the avatar has been removed
+            # XXX: we use empty string instead of None to indicate that we took avatar
+            #      but it is empty on purpose
+            self.plugin_parent.updateCache(client, entity_jid, 'avatar', '')
+            return
+
+        file_path = client.cache.getFilePath(hash_)
+        if file_path is not None:
+            log.debug(u"New avatar found for [{}], it's already in cache, we use it".format(entity_jid.full()))
+            self.plugin_parent.updateCache(client, entity_jid, 'avatar', hash_)
         else:
-            if os.path.exists(filename):
-                log.debug(u"avatar for {} already in cache".format(from_jid.full()))
-            else:
-                log.error(u"Avatar for [{}] should be in cache but it is not ! We get it".format(from_jid.full()))
-                self.plugin_parent.getCard(self.parent, from_jid)
-
+            log.debug(u'New avatar found for [{}], requesting vcard'.format(entity_jid.full()))
+            d = self.plugin_parent.getCard(client, entity_jid)
+            d.addCallback(self._checkAvatarHash, client, entity_jid, hash_)