2005-11-07 Robert McQueen <robot101@debian.org>

	* python/_dbus.py: Add WeakReferenceDictionary cache of dbus.Bus
	instances to stop madness of creating new instances representing
	the same bus connection all the time, rendering any tracking of
	match rules and bus names quite meaningless. Caught a bug where
	the private argument to SessionBus() and friends was being passed
	in as use_default_mainloop by mistake. Still some problems with
	multiple dbus_binding.Connection instances representing the same
	low-level connection (eg when you use both SessionBus() and
	StarterBus() in same process), but it's a lot better now than it
	was.

	* python/dbus_bindings.pyx: Add constants with the return values
	for bus_request_name().

	* python/service.py: Store bus name instances in a per-dbus.Bus cache
	and retrieve the same instances for the same name, so deletion can be
	done with refcounting. Also now throws some kind of error if you
	don't actually get the name you requested, unlike previously...

	* test/python/test-client.py: Add tests for instance caching of buses
	and bus name objects.

5 files changed, 198 insertions, 32 deletions

diff --git a/ChangeLog b/ChangeLog
index b90e2e0e..a870e5da 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,27 @@
+2005-11-07  Robert McQueen  <robot101@debian.org>
+
+	* python/_dbus.py: Add WeakReferenceDictionary cache of dbus.Bus
+	instances to stop madness of creating new instances representing
+	the same bus connection all the time, rendering any tracking of
+	match rules and bus names quite meaningless. Caught a bug where
+	the private argument to SessionBus() and friends was being passed
+	in as use_default_mainloop by mistake. Still some problems with
+	multiple dbus_binding.Connection instances representing the same
+	low-level connection (eg when you use both SessionBus() and
+	StarterBus() in same process), but it's a lot better now than it
+	was.
+
+	* python/dbus_bindings.pyx: Add constants with the return values
+	for bus_request_name().
+
+	* python/service.py: Store bus name instances in a per-dbus.Bus cache
+	and retrieve the same instances for the same name, so deletion can be
+	done with refcounting. Also now throws some kind of error if you
+	don't actually get the name you requested, unlike previously...
+
+	* test/python/test-client.py: Add tests for instance caching of buses
+	and bus name objects.
+
 2005-11-04  Robert McQueen  <robot101@debian.org>
 
 	* python/dbus_bindings.pyx, test/python/test-client.py: Fix
diff --git a/python/_dbus.py b/python/_dbus.py
index bb4c0428..2376f172 100644
--- a/python/_dbus.py
+++ b/python/_dbus.py
@@ -42,14 +42,14 @@ print(dbus_object.ListServices())
 """
 
 import dbus
-
 import dbus_bindings
+import weakref
 
 from proxies import *
 from exceptions import *
 from matchrules import *
 
-class Bus:
+class Bus(object):
     """A connection to a DBus daemon.
 
     One of three possible standard buses, the SESSION, SYSTEM,
@@ -65,19 +65,54 @@ class Bus:
     ProxyObjectClass = ProxyObject
 
     START_REPLY_SUCCESS = dbus_bindings.DBUS_START_REPLY_SUCCESS
-    START_REPLY_ALREADY_RUNNING = dbus_bindings.DBUS_START_REPLY_ALREADY_RUNNING 
+    START_REPLY_ALREADY_RUNNING = dbus_bindings.DBUS_START_REPLY_ALREADY_RUNNING
+
+    _shared_instances = weakref.WeakValueDictionary()
+
+    def __new__(cls, bus_type=TYPE_SESSION, use_default_mainloop=True, private=False):
+        if (not private and bus_type in cls._shared_instances):
+            return cls._shared_instances[bus_type]
+
+        # this is a bit odd, but we create instances of the subtypes
+        # so we can return the shared instances if someone tries to
+        # construct one of them (otherwise we'd eg try and return an
+        # instance of Bus from __new__ in SessionBus). why are there
+        # three ways to construct this class? we just don't know.
+        if bus_type == cls.TYPE_SESSION:
+            subclass = SessionBus
+        elif bus_type == cls.TYPE_SYSTEM:
+            subclass = SystemBus
+        elif bus_type == cls.TYPE_STARTER:
+            subclass = StarterBus
+        else:
+            raise ValueError('invalid bus_type %s' % bus_type)
+
+        bus = object.__new__(subclass)
 
-    def __init__(self, bus_type=TYPE_SESSION, use_default_mainloop=True, private=False):
-        self._bus_type = bus_type
-        self._connection = dbus_bindings.bus_get(bus_type, private)
+        bus._bus_type = bus_type
+        bus._bus_names = weakref.WeakValueDictionary()
+        bus._match_rule_tree = SignalMatchTree()
 
-        self._connection.add_filter(self._signal_func)
-        self._match_rule_tree = SignalMatchTree()
+        # FIXME: if you get a starter and a system/session bus connection
+        # in the same process, it's the same underlying connection that
+        # is returned by bus_get, but we initialise it twice
+        bus._connection = dbus_bindings.bus_get(bus_type, private)
+        bus._connection.add_filter(bus._signal_func)
 
         if use_default_mainloop:
             func = getattr(dbus, "_dbus_mainloop_setup_function", None)
-            if func != None:
-                func(self)
+            if func:
+                func(bus)
+
+        if not private:
+            cls._shared_instances[bus_type] = bus
+
+        return bus
+
+    def __init__(self, *args, **keywords):
+        # do nothing here because this can get called multiple times on the
+        # same object if __new__ returns a shared instance
+        pass
 
     def close(self):
         self._connection.close()
@@ -87,20 +122,20 @@ class Bus:
 
     def get_session(private=False):
         """Static method that returns the session bus"""
-        return SessionBus(private)
+        return SessionBus(private=private)
 
     get_session = staticmethod(get_session)
 
     def get_system(private=False):
         """Static method that returns the system bus"""
-        return SystemBus(private)
+        return SystemBus(private=private)
 
     get_system = staticmethod(get_system)
 
 
     def get_starter(private=False):
         """Static method that returns the starter bus"""
-        return StarterBus(private)
+        return StarterBus(private=private)
 
     get_starter = staticmethod(get_starter)
 
@@ -213,24 +248,24 @@ class Bus:
 class SystemBus(Bus):
     """The system-wide message bus
     """
-    def __init__(self, use_default_mainloop=True, private=False):
-        Bus.__init__(self, Bus.TYPE_SYSTEM, use_default_mainloop, private)
+    def __new__(cls, use_default_mainloop=True, private=False):
+        return Bus.__new__(cls, Bus.TYPE_SYSTEM, use_default_mainloop, private)
 
 class SessionBus(Bus):
     """The session (current login) message bus
     """
-    def __init__(self, use_default_mainloop=True, private=False):
-        Bus.__init__(self, Bus.TYPE_SESSION, use_default_mainloop, private)
+    def __new__(cls, use_default_mainloop=True, private=False):
+        return Bus.__new__(cls, Bus.TYPE_SESSION, use_default_mainloop, private)
 
 class StarterBus(Bus):
     """The bus that activated this process (if
     this process was launched by DBus activation)
     """
-    def __init__(self, use_default_mainloop=True, private=False):
-        Bus.__init__(self, Bus.TYPE_STARTER, use_default_mainloop, private)
+    def __new__(cls, use_default_mainloop=True, private=False):
+        return Bus.__new__(cls, Bus.TYPE_STARTER, use_default_mainloop, private)
 
 class Interface:
-    """An inteface into a remote object
+    """An interface into a remote object
 
     An Interface can be used to wrap ProxyObjects
     so that calls can be routed to their correct
diff --git a/python/dbus_bindings.pyx b/python/dbus_bindings.pyx
index a96a5adc..75e448ee 100644
--- a/python/dbus_bindings.pyx
+++ b/python/dbus_bindings.pyx
@@ -1748,8 +1748,13 @@ def bus_register(Connection connection):
 
     return retval
 
-SERVICE_FLAG_PROHIBIT_REPLACEMENT = 0x1
-SERVICE_FLAG_REPLACE_EXISTING     = 0x2
+NAME_FLAG_PROHIBIT_REPLACEMENT = 0x1
+NAME_FLAG_REPLACE_EXISTING     = 0x2
+
+REQUEST_NAME_REPLY_PRIMARY_OWNER = 1
+REQUEST_NAME_REPLY_IN_QUEUE      = 2
+REQUEST_NAME_REPLY_EXISTS        = 3
+REQUEST_NAME_REPLY_ALREADY_OWNER = 4
 
 def bus_request_name(Connection connection, service_name, flags=0):
     cdef DBusError error
diff --git a/python/service.py b/python/service.py
index 14a2d6d3..d1973b04 100644
--- a/python/service.py
+++ b/python/service.py
@@ -6,19 +6,60 @@ from exceptions import UnknownMethodException
 from decorators import method
 from decorators import signal
 
-class BusName:
+class BusName(object):
     """A base class for exporting your own Named Services across the Bus
     """
-    def __init__(self, named_service, bus=None):
-        self._named_service = named_service
-                             
+    def __new__(cls, name, bus=None):
+        # get default bus
         if bus == None:
-            # Get the default bus
-            self._bus = _dbus.Bus()
+            bus = _dbus.Bus()
+
+        # see if this name is already defined, return it if so
+        if name in bus._bus_names:
+            return bus._bus_names[name]
+
+        # otherwise register the name
+        retval = dbus_bindings.bus_request_name(bus.get_connection(), name)
+        print retval
+        # TODO: more intelligent tracking of bus name states?
+        if retval == dbus_bindings.REQUEST_NAME_REPLY_PRIMARY_OWNER:
+            pass
+        elif retval == dbus_bindings.REQUEST_NAME_REPLY_IN_QUEUE:
+            # you can't arrive at this state via the high-level bindings
+            # because you can't put flags in, but... who knows?
+            print "joined queue for %s" % name
+            pass
+        elif retval == dbus_bindings.REQUEST_NAME_REPLY_EXISTS:
+            raise dbus_bindings.DBusException('requested name %s already exists' % name)
+        elif retval == dbus_bindings.REQUEST_NAME_REPLY_ALREADY_OWNER:
+            # if this is a shared bus which is being used by someone
+            # else in this process, this can happen legitimately
+            print "already owner of %s" % name
+            pass
         else:
-            self._bus = bus
+            raise dbus_bindings.DBusException('requesting name %s returned unexpected value %s' % (name, retval))
 
-        dbus_bindings.bus_request_name(self._bus.get_connection(), named_service)
+        # and create the object
+        bus_name = object.__new__(cls)
+        bus_name._bus = bus
+        bus_name._name = name
+
+        # cache instance
+        bus._bus_names[name] = bus_name
+
+        return bus_name
+
+    # do nothing because this is called whether or not the bus name
+    # object was retrieved from the cache or created new
+    def __init__(self, *args, **keywords):
+        pass
+
+    # we can delete the low-level name here because these objects
+    # are guaranteed to exist only once for each bus name
+    def __del__(self):
+        # FIXME: we don't have this function yet :)
+        #dbus_bindings.bus_release_name(self._bus.get_connection(), self._named_service)
+        pass
 
     def get_bus(self):
         """Get the Bus this Service is on"""
@@ -26,10 +67,10 @@ class BusName:
 
     def get_name(self):
         """Get the name of this service"""
-        return self._named_service
+        return self._name
 
     def __repr__(self):
-        return '<dbus.service.BusName %s on %r at %#x>' % (self._named_service, self._bus, id(self))
+        return '<dbus.service.BusName %s on %r at %#x>' % (self._name, self._bus, id(self))
     __str__ = __repr__
 
 
@@ -322,3 +363,4 @@ class Object(Interface):
     def __repr__(self):
         return '<dbus.service.Object %s on %r at %#x>' % (self._object_path, self._name, id(self))
     __str__ = __repr__
+
diff --git a/test/python/test-client.py b/test/python/test-client.py
index 8c09b67e..26ce375c 100755
--- a/test/python/test-client.py
+++ b/test/python/test-client.py
@@ -14,6 +14,7 @@ import dbus
 import dbus_bindings
 import gobject
 import dbus.glib
+import dbus.service
 
 if not dbus.__file__.startswith(pydir):
     raise Exception("DBus modules are not being picked up from the package")
@@ -169,6 +170,65 @@ class TestDBusBindings(unittest.TestCase):
                 print val, ret
                 self.assert_(val == ret)
 
+    def testBusInstanceCaching(self):
+        print "\n********* Testing dbus.Bus instance sharing *********"
+
+        # unfortunately we can't test the system bus here
+        # but the codepaths are the same
+        for (cls, type, func) in ((dbus.SessionBus, dbus.Bus.TYPE_SESSION, dbus.Bus.get_session), (dbus.StarterBus, dbus.Bus.TYPE_STARTER, dbus.Bus.get_starter)):
+            print "\nTesting %s:" % cls.__name__
+
+            share_cls = cls()
+            share_type = dbus.Bus(bus_type=type)
+            share_func = func()
+
+            private_cls = cls(private=True)
+            private_type = dbus.Bus(bus_type=type, private=True)
+            private_func = func(private=True)
+
+            print " - checking shared instances are the same..."
+            self.assert_(share_cls == share_type, '%s should equal %s' % (share_cls, share_type))
+            self.assert_(share_type == share_func, '%s should equal %s' % (share_type, share_func))
+
+            print " - checking private instances are distinct from the shared instance..."
+            self.assert_(share_cls != private_cls, '%s should not equal %s' % (share_cls, private_cls))
+            self.assert_(share_type != private_type, '%s should not equal %s' % (share_type, private_type))
+            self.assert_(share_func != private_func, '%s should not equal %s' % (share_func, private_func))
+
+            print " - checking private instances are distinct from each other..."
+            self.assert_(private_cls != private_type, '%s should not equal %s' % (private_cls, private_type))
+            self.assert_(private_type != private_func, '%s should not equal %s' % (private_type, private_func))
+            self.assert_(private_func != private_cls, '%s should not equal %s' % (private_func, private_cls))
+
+    def testBusNameCreation(self):
+        print '\n******** Testing BusName creation ********'
+        test = [('org.freedesktop.DBus.Python.TestName', True),
+                ('org.freedesktop.DBus.Python.TestName', True),
+                ('org.freedesktop.DBus.Python.InvalidName&^*%$', False),
+                ('org.freedesktop.DBus.TestSuitePythonService', False)]
+        # For some reason this actually succeeds
+        # ('org.freedesktop.DBus', False)]
+
+        # make a method call to ensure the test service is active
+        self.iface.Echo("foo")
+
+        names = {}
+        for (name, succeed) in test:
+            try:
+                print "requesting %s" % name
+                busname = dbus.service.BusName(name)
+            except Exception, e:
+                print "%s:\n%s" % (e.__class__, e)
+                self.assert_(not succeed, 'did not expect registering bus name %s to fail' % name)
+            else:
+                print busname
+                self.assert_(succeed, 'expected registering bus name %s to fail'% name)
+                if name in names:
+                    self.assert_(names[name] == busname, 'got a new instance for same name %s' % name)
+                    print "instance of %s re-used, good!" % name
+                else:
+                    names[name] = busname
+
 class TestDBusPythonToGLibBindings(unittest.TestCase):
     def setUp(self):
         self.bus = dbus.SessionBus()