[sugar] Ticket #1289, patch review

[Mike C. Fletcher]

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.
>      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.
> +    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

-- 
________________________________________________
  Mike C. Fletcher
  Designer, VR Plumber, Coder
  http://www.vrplumber.com
  http://blog.vrplumber.com