[sugar] Ticket #1289, patch review

[Dan Williams]

On Sat, 2007-04-07 at 00:34 -0400, Mike C. Fletcher wrote:
> Marco Pesenti Gritti wrote:
> > First of all this is really useful, thanks!
> >   
> Most welcome.
> > +        # XXX this creates an uncollectable object, should be handled
> > +        # using a weakref callback...
> >          gobject.source_remove(self._launch_timeout_id)
> >          self._launch_timeout_id = 0
> >
> > I don't understand this. To which object are you referring?
> >   
> The current object, in this instance, a HomeActivity.  When you define a 
> __del__ method you prevent Python's garbage collection system from 
> collecting cycles that happen to include this object.  Generally 
> speaking, you shouldn't use __del__ for this kind of cleanup work, 
> instead use a weakref.ref with a callable that will clean up after the 
> object (without any reference to the attributes of the object).  It 
> takes more work (because by the time it is called the object is already 
> gone), but in complex systems it tends to produce fewer hard-to-debug 
> memory leaks.
> 
>  >>> import weakref
>  >>> class x(object):
> ...     def __init__( self ):
> ...             x = 2
> ...             def closer( ref ):
> ...                     print x
> ...             self._closer = weakref.ref( self, closer )
> ...
>  >>> y = x()
>  >>> del y
> 2
> 
> Not a critical issue unless we introduce a reference cycle at some 
> point, but that might happen some day when we aren't thinking about this 
> method.
> > This code should go away at some point I think, we should just use the
> > dbus timeout.
> >   
> Fair enough.
> > +        """Retrieve the "activity_id" passed in to our constructor
> > +        
> > +        XXX What is the activity id, really?
> >
> > Each activity instance has a unique id, so that it can be resumed and
> > shared on the network.
> >   
> Yes, but what *is* it, would be nice to have a sample of what one looks 
> like (so programmers recognise them), rules on how they are constructed, 
> how unique they are, how we deal with collisions, etceteras.  Is this a 
> GUID?  Is it a URL-like string?  Is it some large unique number?  Is it 
> an SHA hash of something?  The XXX was a note to ourselves that we need 
> to describe these things somewhere in full.

It's pretty much an implementation detail.  Activities should never have
to know about how it gets constructed at all.  They may have to know how
large it is for storage of some kind, but assuming we define it to be
large enough that's not a problem.  If an activity actually cares about
the implementation details, we must ask why it does so.

Activitiy IDs are large, globally unique strings that are opaque to
everything but the sugar shell.  They mean nothing.  And we will have to
find some facility for handling collisions.

> >      def get_window(self):
> > +        """Retrieve the X-windows root window of this application
> > +        
> > +        This was stored by the set_window method.
> > +        
> > +        XXX What called that?
> > +        """
> >
> > Activities are added to HomeModel just after starting them, before the
> > window actually appears, so that we can do startup notification. So
> > HomeModel listen for new windows creation and when the one associated
> > with an HomeActivity appears, it set_window on it.
> >   
> Alright, have updated the doc string to reflect that.
> > +    Note: there seem to be logic issues in this function
> >   
> ...
> > Haven't seen the function but if you want to refactor it and post a
> > patch that would be useful. Dan might have more insights on why it does
> > what it does.
> >   
> Okay, will look into that.

You've caught a real bug, thanks :)  That function is wrong in exactly
the way you suggest.

Dan

> > +    This object uses a dbus method on the ActivityFactory
> > +    service to create the new activity.  It generates 
> > +    GTK events in response to the success/failure of
> > +    activity startup using callbacks to the service's 
> > +    create call.
> >
> > s/GTK events/GObject events
> >   
> Okay.
> > +        XXX where is the ActivityFactoryService specific to this
> > +        instance created?  That is, what's launching the 
> > +        activity-factory scripts?  Is it the 
> > +        com.redhat.Sugar.ActivityFactory service?  If so, where
> > +        is that defined/registered/created?
> >
> > DBus activation. When you do a call on a dbus service and the service is
> > not active DBus automatically launch the process. We generate the
> > service information DBus uses to do this dinamically in
> > sugar/activity/bundleregistry.py.
> >   
> Ah, that's the key piece I was missing.  Thanks.  I've added some docs 
> to bundleregistry._ServiceManager to explain some of that.
> > +        object_id -- XXX what is this?
> >   
> ...
> > +        uri -- XXX what is this?
> >   
> Updated, thanks.
> > +"""Metadata description of a given application/activity
> > +
> > +The bundle's metadata is normally read from an activity.info 
> > +file in order to describe a given bundle (directory).
> > +"""
> >
> > The spec should be referenced there:
> > http://wiki.laptop.org/go/Activity_bundles
> >   
> Okay, have added the link to the spec.
> > +        At the moment this requires that something somewhere
> > +        have registered a dbus service with the XID of the 
> > +        new window that is used to bind the requested activity 
> > +        to the window.
> >
> > In the future the dbus service will not be strictly required. I want to
> > be able to launch simple windows in the shell for debugging purposes.
> > Still the service(s) will be required by proper activities.
> > It would be probably possible to increase compatibility further but
> > that's not planned at the moment.
> >
> > Do you have write access to the sugar git module? Otherwise can you send
> > mail to Ivan about it and cc me?
> >   
> Nope, don't have access yet.  Will email Ivan.
> 
> Have fun,
> Mike
>