Improve Salut integration so buddies show up in the shell:

* Consider a Buddy to be valid after either we've tried and either succeeded
  or failed to get its buddy-properties and alias
* Only try to manipulate the subscribe and publish lists on Gabble
* Separate the concepts of "cleanup" and "stop" for Telepathy plugins
* Disable Gabble/Salut properly in response to debug flags
* Don't try to send buddy properties that could be None over D-Bus

6 files changed, 146 insertions, 90 deletions

diff --git a/src/buddy.py b/src/buddy.py
index d225cc8..cd4cb8a 100644
--- a/src/buddy.py
+++ b/src/buddy.py
@@ -121,7 +121,8 @@ class Buddy(ExportedGObject):
         'validity-changed':
             # The buddy's validity changed.
             # Validity starts off False, and becomes True when the buddy
-            # has a color, a nick and a key.
+            # either has, or has tried and failed to get, a color, a nick
+            # and a key.
             # * the new validity: bool
             (gobject.SIGNAL_RUN_FIRST, None, [bool]),
         'property-changed':
@@ -140,15 +141,25 @@ class Buddy(ExportedGObject):
     }
 
     __gproperties__ = {
-        _PROP_KEY          : (str, None, None, None, gobject.PARAM_READWRITE),
-        _PROP_ICON         : (object, None, None, gobject.PARAM_READWRITE),
-        _PROP_NICK         : (str, None, None, None, gobject.PARAM_READWRITE),
-        _PROP_COLOR        : (str, None, None, None, gobject.PARAM_READWRITE),
-        _PROP_CURACT       : (str, None, None, None, gobject.PARAM_READWRITE),
+        _PROP_KEY          : (str, None, None, None,
+                              gobject.PARAM_CONSTRUCT_ONLY |
+                              gobject.PARAM_READWRITE),
+        _PROP_ICON         : (object, None, None, gobject.PARAM_READABLE),
+        _PROP_NICK         : (str, None, None, None,
+                              gobject.PARAM_CONSTRUCT_ONLY |
+                              gobject.PARAM_READWRITE),
+        _PROP_COLOR        : (str, None, None, None,
+                              gobject.PARAM_CONSTRUCT_ONLY |
+                              gobject.PARAM_READWRITE),
+        _PROP_CURACT       : (str, None, None, None,
+                              gobject.PARAM_CONSTRUCT_ONLY |
+                              gobject.PARAM_READWRITE),
         _PROP_VALID        : (bool, None, None, False, gobject.PARAM_READABLE),
         _PROP_OWNER        : (bool, None, None, False, gobject.PARAM_READABLE),
         _PROP_OBJID        : (str, None, None, None, gobject.PARAM_READABLE),
-        _PROP_IP4_ADDRESS  : (str, None, None, None, gobject.PARAM_READWRITE)
+        _PROP_IP4_ADDRESS  : (str, None, None, None,
+                              gobject.PARAM_CONSTRUCT_ONLY |
+                              gobject.PARAM_READWRITE)
     }
 
     def __init__(self, bus, object_id, **kwargs):
@@ -172,7 +183,7 @@ class Buddy(ExportedGObject):
         #: Telepathy plugin -> (handle, identifier e.g. JID)
         self._handles = {}
 
-        self._valid = False
+        self._awaiting = set(('alias', 'properties'))
         self._owner = False
         self._key = None
         self._icon = ''
@@ -198,8 +209,9 @@ class Buddy(ExportedGObject):
         ExportedGObject.__init__(self, bus, self._object_path,
                                  gobject_properties=kwargs)
 
-        if icon_data:
-            self.props.icon = icon_data
+        if icon_data is not None:
+            self._icon = str(icon_data)
+            self.IconChanged(self._icon)
 
     def do_get_property(self, pspec):
         """Retrieve current value for the given property specifier
@@ -223,7 +235,7 @@ class Buddy(ExportedGObject):
                 return None
             return self._current_activity
         elif pspec.name == _PROP_VALID:
-            return self._valid
+            return not self._awaiting
         elif pspec.name == _PROP_OWNER:
             return self._owner
         elif pspec.name == _PROP_IP4_ADDRESS:
@@ -236,7 +248,6 @@ class Buddy(ExportedGObject):
         value -- value to set
 
         emits 'icon-changed' signal on icon setting
-        calls _update_validity on all calls
         """
         if pspec.name == _PROP_ICON:
             if str(value) != self._icon:
@@ -255,8 +266,6 @@ class Buddy(ExportedGObject):
         elif pspec.name == _PROP_IP4_ADDRESS:
             self._ip4_address = value
 
-        self._update_validity()
-
     # dbus signals
     @dbus.service.signal(_BUDDY_INTERFACE,
                         signature="ay")
@@ -373,10 +382,10 @@ class Buddy(ExportedGObject):
                 "" if no current activity
         """
         props = {}
-        props[_PROP_NICK] = self.props.nick
-        props[_PROP_OWNER] = self.props.owner
-        props[_PROP_KEY] = self.props.key
-        props[_PROP_COLOR] = self.props.color
+        props[_PROP_NICK] = self.props.nick or ''
+        props[_PROP_OWNER] = self.props.owner or ''
+        props[_PROP_KEY] = self.props.key or ''
+        props[_PROP_COLOR] = self.props.color or ''
 
         if self.props.ip4_address:
             props[_PROP_IP4_ADDRESS] = self.props.ip4_address
@@ -464,8 +473,7 @@ class Buddy(ExportedGObject):
         properties -- set of property values to set
 
         if no change, no events generated
-        if change, generates property-changed and
-            calls _update_validity
+        if change, generates property-changed
         """
         changed = False
         changed_props = {}
@@ -508,7 +516,7 @@ class Buddy(ExportedGObject):
         # Try emitting PropertyChanged before updating validity
         # to avoid leaking a PropertyChanged signal before the buddy is
         # actually valid the first time after creation
-        if self._valid:
+        if not self._awaiting:
             dbus_changed = {}
             for key, value in changed_props.items():
                 if value:
@@ -519,29 +527,35 @@ class Buddy(ExportedGObject):
 
             self._property_changed(changed_props)
 
-        self._update_validity()
-
     def _property_changed(self, changed_props):
         pass
 
-    def _update_validity(self):
-        """Check whether we are now valid
+    def update_buddy_properties(self, tp, props):
+        """Update the buddy properties (those that come from the GetProperties
+        method of the org.laptop.Telepathy.BuddyInfo interface) from the
+        given Telepathy connection.
 
-        validity is True if color, nick and key are non-null
-
-        emits validity-changed if we have changed validity
+        Other properties, such as 'nick', may not be set via this method.
         """
-        try:
-            old_valid = self._valid
-            if self._color and self._nick and self._key:
-                self._valid = True
-            else:
-                self._valid = False
+        self.set_properties(props)
+        # If the properties didn't contain the key or color, then we're never
+        # going to get one.
+        self._awaiting.discard('properties')
+        if not self._awaiting:
+            self.emit('validity-changed', True)
+
+    def update_alias(self, tp, alias):
+        """Update the alias from the given Telepathy connection.
+        """
+        self.set_properties({'nick': alias})
+        self._awaiting.discard('alias')
+        if not self._awaiting:
+            self.emit('validity-changed', True)
 
-            if old_valid != self._valid:
-                self.emit("validity-changed", self._valid)
-        except AttributeError:
-            self._valid = False
+    def update_current_activity(self, tp, current_activity):
+        """Update the current activity from the given Telepathy connection.
+        """
+        self.set_properties({'current-activity': current_activity})
 
     def update_avatar(self, tp, new_avatar_token, icon=None, mime_type=None):
         """Handle update of the avatar"""
@@ -846,6 +860,9 @@ class ShellOwner(GenericOwner):
                 bus_name=self._SHELL_SERVICE,
                 path=self._SHELL_PATH)
 
+        # we already know our own nick, color, key
+        self._awaiting = None
+
     def set_registered(self, value):
         """Handle notification that we have been registered"""
         if value:
@@ -853,7 +870,10 @@ class ShellOwner(GenericOwner):
 
     def _icon_changed_cb(self, icon):
         """Handle icon change, set property to generate event"""
-        self.props.icon = icon
+        icon = str(icon)
+        if icon != self._icon:
+            self._icon = icon
+            self.IconChanged(icon)
 
     def _color_changed_cb(self, color):
         """Handle color change, set property to generate event"""
diff --git a/src/linklocal_plugin.py b/src/linklocal_plugin.py
index e4a821b..1e98ac3 100644
--- a/src/linklocal_plugin.py
+++ b/src/linklocal_plugin.py
@@ -76,7 +76,13 @@ class LinkLocalPlugin(TelepathyPlugin):
             if had_avahi:
                 _logger.info('Avahi disappeared from the system bus - '
                              'stopping...')
-                self.cleanup()
+                self.stop()
+
+    def cleanup(self):
+        TelepathyPlugin.cleanup(self)
+        if self._watch is not None:
+            self._watch.cancel()
+        self._watch = None
 
     def _could_connect(self):
         return self._have_avahi
diff --git a/src/presenceservice.py b/src/presenceservice.py
index 22825b0..2662f72 100644
--- a/src/presenceservice.py
+++ b/src/presenceservice.py
@@ -102,13 +102,18 @@ class PresenceService(ExportedGObject):
         self._registry.LoadManagers()
 
         # Set up the Telepathy plugins
-        self._server_plugin = ServerPlugin(self._registry, self._owner)
-        self._ll_plugin = LinkLocalPlugin(self._registry, self._owner)
         self._plugins = []
         debug_flags = set(environ.get('PRESENCE_SERVICE_DEBUG', '').split(','))
-        if 'disable-gabble' not in debug_flags:
+        _logger.debug('Debug flags: %r', debug_flags)
+        if 'disable-gabble' in debug_flags:
+            self._server_plugin = None
+        else:
+            self._server_plugin = ServerPlugin(self._registry, self._owner)
             self._plugins.append(self._server_plugin)
-        if 'disable-salut' not in debug_flags:
+        if 'disable-salut' in debug_flags:
+            self._ll_plugin = None
+        else:
+            self._ll_plugin = LinkLocalPlugin(self._registry, self._owner)
             self._plugins.append(self._ll_plugin)
         self._connected_plugins = set()
 
@@ -178,7 +183,9 @@ class PresenceService(ExportedGObject):
             self._conn_matches[conn].append(m)
 
             def buddy_properties_changed(contact, properties):
-                self._buddy_properties_changed(tp, contact, properties)
+                buddy = self._handles_buddies[tp].get(contact)
+                if buddy is not None and buddy is not self._owner:
+                    buddy.update_buddy_properties(tp, properties)
             m = conn[CONN_INTERFACE_BUDDY_INFO].connect_to_signal(
                 'PropertiesChanged', buddy_properties_changed)
             self._conn_matches[conn].append(m)
@@ -188,8 +195,9 @@ class PresenceService(ExportedGObject):
                     room == 0):
                     act_id = ''
                     room = 0
-                self._buddy_properties_changed(tp, contact,
-                                               {'current-activity': act_id})
+                buddy = self._handles_buddies[tp].get(contact)
+                if buddy is not None and buddy is not self._owner:
+                    buddy.update_current_activity(tp, act_id)
                 # FIXME: do something useful with the room handle?
             m = conn[CONN_INTERFACE_BUDDY_INFO].connect_to_signal(
                 'CurrentActivityChanged', buddy_curact_changed)
@@ -218,8 +226,9 @@ class PresenceService(ExportedGObject):
         if CONN_INTERFACE_ALIASING in conn:
             def aliases_changed(aliases):
                 for contact, alias in aliases:
-                    self._buddy_properties_changed(tp, contact,
-                                                   {'nick': alias})
+                    buddy = self._handles_buddies[tp].get(contact)
+                    if buddy is not None and buddy is not self._owner:
+                        buddy.update_alias(tp, alias)
             m = conn[CONN_INTERFACE_ALIASING].connect_to_signal(
                     'AliasesChanged', aliases_changed)
             self._conn_matches[conn].append(m)
@@ -285,8 +294,9 @@ class PresenceService(ExportedGObject):
             def got_aliases(aliases):
                 gobject.idle_add(self._run_contacts_online_queue)
                 for contact, alias in izip(handles, aliases):
-                    self._buddy_properties_changed(tp, contact,
-                                                   {'nick': alias})
+                    buddy = self._handles_buddies[tp].get(contact)
+                    if buddy is not None and buddy is not self._owner:
+                        buddy.update_alias(tp, alias)
             def request_aliases():
                 try:
                     conn[CONN_INTERFACE_ALIASING].RequestAliases(handles,
@@ -326,7 +336,9 @@ class PresenceService(ExportedGObject):
                 _logger.warning('Error %s: %s', when, e)
             def got_properties(props):
                 gobject.idle_add(self._run_contacts_online_queue)
-                self._buddy_properties_changed(tp, contact, props)
+                buddy = self._handles_buddies[tp].get(contact)
+                if buddy is not None and buddy is not self._owner:
+                    buddy.update_buddy_properties(tp, props)
             def get_properties():
                 try:
                     conn[CONN_INTERFACE_BUDDY_INFO].GetProperties(contact,
@@ -336,11 +348,15 @@ class PresenceService(ExportedGObject):
                 except Exception, e:
                     gobject.idle_add(self._run_contacts_online_queue)
                     handle_error(e, 'fetching buddy properties')
+            def got_current_activity(current_activity, room):
+                gobject.idle_add(self._run_contacts_online_queue)
+                buddy = self._handles_buddies[tp].get(contact)
+                if buddy is not None and buddy is not self._owner:
+                    buddy.update_current_activity(tp, current_activity)
             def get_current_activity():
                 try:
                     conn[CONN_INTERFACE_BUDDY_INFO].GetCurrentActivity(contact,
-                        reply_handler=lambda c, room:
-                            got_properties({'current-activity': c}),
+                        reply_handler=got_current_activity,
                         error_handler=lambda e:
                             handle_error(e, 'fetching current activity'))
                 except Exception, e:
@@ -409,13 +425,6 @@ class PresenceService(ExportedGObject):
             _logger.debug("Buddy %s icon updated" % buddy.props.nick)
             buddy.update_avatar(tp, new_avatar_token, avatar, mime_type)
 
-    def _buddy_properties_changed(self, tp, handle, properties):
-        buddy = self._handles_buddies[tp].get(handle)
-        if buddy:
-            buddy.set_properties(properties)
-            _logger.debug("Buddy %s properties updated: %s", buddy.props.nick,
-                          properties.keys())
-
     def _new_activity(self, activity_id, tp, room):
         try:
             objid = self._get_next_object_id()
diff --git a/src/pstest.py b/src/pstest.py
index 3505a4d..094e15b 100644
--- a/src/pstest.py
+++ b/src/pstest.py
@@ -68,6 +68,9 @@ class TestOwner(GenericOwner):
                 key=pubkey, nick=nick, color=color, icon=icon,
                 registered=registered, key_hash=privkey_hash)
 
+        # we already know our own nick, color, key
+        self._awaiting = None
+
         # Only do the random stuff if randomize is true
         if randomize:
             self._ps.connect('connection-status', self._ps_connection_status_cb)
diff --git a/src/server_plugin.py b/src/server_plugin.py
index 7c6bdd3..526a639 100644
--- a/src/server_plugin.py
+++ b/src/server_plugin.py
@@ -57,7 +57,11 @@ class ServerPlugin(TelepathyPlugin):
 
         # Monitor IPv4 address as an indicator of the network connection
         self._ip4am = psutils.IP4AddressMonitor.get_instance()
-        self._ip4am.connect('address-changed', self._ip4_address_changed_cb)
+        self._ip4am_sigid = self._ip4am.connect('address-changed', self._ip4_address_changed_cb)
+
+    def cleanup(self):
+        TelepathyPlugin.cleanup(self)
+        self._ip4am.disconnect(self._ip4am_sigid)
 
     def _ip4_address_changed_cb(self, ip4am, address):
         _logger.debug("::: IP4 address now %s", address)
@@ -69,7 +73,7 @@ class ServerPlugin(TelepathyPlugin):
                 self.start()
         else:
             _logger.debug("::: invalid IP4 address, will disconnect")
-            self.cleanup()
+            self.stop()
 
     def _get_account_info(self):
         """Retrieve connection manager parameters for this account
@@ -220,3 +224,38 @@ class ServerPlugin(TelepathyPlugin):
                     ret[handle] = 'xmpp/' + psutils.escape_identifier(jid)
 
         return ret
+
+    def _connected_cb(self):
+        TelepathyPlugin._connected_cb(self)
+
+        publish_handles, local_pending, remote_pending = \
+                self._publish_channel[CHANNEL_INTERFACE_GROUP].GetAllMembers()
+
+        if local_pending:
+            # accept pending subscriptions
+            # FIXME: do this async
+            publish[CHANNEL_INTERFACE_GROUP].AddMembers(local_pending, '')
+
+        # request subscriptions from people subscribed to us if we're not
+        # subscribed to them
+        not_subscribed = set(publish_handles)
+        not_subscribed -= self._subscribe_members
+        subscribe[CHANNEL_INTERFACE_GROUP].AddMembers(not_subscribed, '')
+
+    def _publish_members_changed_cb(self, message, added, removed,
+                                    local_pending, remote_pending,
+                                    actor, reason):
+        TelepathyPlugin._publish_members_changed_cb()
+
+        if local_pending:
+            # accept all requested subscriptions
+            self._publish_channel[CHANNEL_INTERFACE_GROUP].AddMembers(
+                    local_pending, '')
+
+        # subscribe to people who've subscribed to us, if necessary
+        if self._subscribe_channel is not None:
+            added = list(set(added) - self._subscribe_members
+                         - self._subscribe_remote_pending)
+            if added:
+                self._subscribe_channel[CHANNEL_INTERFACE_GROUP].AddMembers(
+                        added, '')
diff --git a/src/telepathy_plugin.py b/src/telepathy_plugin.py
index 0e75d62..bbc4201 100644
--- a/src/telepathy_plugin.py
+++ b/src/telepathy_plugin.py
@@ -237,7 +237,7 @@ class TelepathyPlugin(gobject.GObject):
             _logger.debug("%r: connected", self)
             self._connected_cb()
         elif status == CONNECTION_STATUS_DISCONNECTED:
-            self.cleanup()
+            self.stop()
             _logger.debug("%r: disconnected (reason %r)", self, reason)
             if reason == CONNECTION_STATUS_REASON_AUTHENTICATION_FAILED:
                 # FIXME: handle connection failure; retry later?
@@ -256,7 +256,7 @@ class TelepathyPlugin(gobject.GObject):
     def _could_connect(self):
         return True
 
-    def cleanup(self):
+    def stop(self):
         """If we still have a connection, disconnect it"""
 
         matches = self._matches
@@ -279,6 +279,9 @@ class TelepathyPlugin(gobject.GObject):
             gobject.source_remove(self._reconnect_id)
             self._reconnect_id = 0
 
+    def cleanup(self):
+        self.stop()
+
     def _contacts_offline(self, handles):
         """Handle contacts going offline (send message, update set)"""
         self._online_contacts -= handles
@@ -340,19 +343,7 @@ class TelepathyPlugin(gobject.GObject):
 
     def _publish_members_changed_cb(self, message, added, removed,
             local_pending, remote_pending, actor, reason):
-
-        if local_pending:
-            # accept all requested subscriptions
-            self._publish_channel[CHANNEL_INTERFACE_GROUP].AddMembers(
-                    local_pending, '')
-
-        # subscribe to people who've subscribed to us, if necessary
-        if self._subscribe_channel is not None:
-            added = list(set(added) - self._subscribe_members
-                         - self._subscribe_remote_pending)
-            if added:
-                self._subscribe_channel[CHANNEL_INTERFACE_GROUP].AddMembers(
-                        added, '')
+        pass
 
     def _presence_update_cb(self, presence):
         """Send update for online/offline status of presence"""
@@ -420,8 +411,6 @@ class TelepathyPlugin(gobject.GObject):
         m = publish[CHANNEL_INTERFACE_GROUP].connect_to_signal(
                 'MembersChanged', self._publish_members_changed_cb)
         self._matches.append(m)
-        publish_handles, local_pending, remote_pending = \
-                publish[CHANNEL_INTERFACE_GROUP].GetAllMembers()
 
         # the group of contacts for whom you wish to receive presence
         subscribe = self._conn.request_channel(CHANNEL_TYPE_CONTACT_LIST,
@@ -436,21 +425,11 @@ class TelepathyPlugin(gobject.GObject):
         self._subscribe_local_pending = set(subscribe_lp)
         self._subscribe_remote_pending = set(subscribe_rp)
 
-        if local_pending:
-            # accept pending subscriptions
-            # FIXME: do this async
-            publish[CHANNEL_INTERFACE_GROUP].AddMembers(local_pending, '')
-
         # FIXME: do this async?
         self.self_handle = self._conn[CONN_INTERFACE].GetSelfHandle()
         self.self_identifier = self._conn[CONN_INTERFACE].InspectHandles(
                 HANDLE_TYPE_CONTACT, [self.self_handle])[0]
 
-        # request subscriptions from people subscribed to us if we're not
-        # subscribed to them
-        not_subscribed = list(set(publish_handles) - set(subscribe_handles))
-        subscribe[CHANNEL_INTERFACE_GROUP].AddMembers(not_subscribed, '')
-
         # Request presence for everyone we're subscribed to
         self._conn[CONN_INTERFACE_PRESENCE].RequestPresence(subscribe_handles)