[Sugar-devel] [PATCH 1/2] shell: handle activities that cycle through windows dlo#10695

[Simon Schampijer]

On 02/17/2011 01:21 PM, Martin Langhoff wrote:
> On Wed, Feb 16, 2011 at 12:54 PM, Simon Schampijer<simon at schampijer.de>  wrote:
>> the general approach - just a few small comments inline.
>
> Thanks! There's a patch in reply, and a few comments from me here...
>
>>> +    def remove_window_by_xid(self, xid):
>>> +        """Remove a window from the windows stack."""
>>> +        for wnd in self._windows:
>>> +            if wnd.get_xid() == xid:
>>> +                # must break after changing array
>>
>> I think this comment can go away. The code itself should be clear enough.
>
> Sugar developers are smarter than I expect then :-) My concern is
> about someone "expanding" the code inside the loop, and getting bitten
> by my doing something that is not quite kosher.

I think this code is ok without the comment :) Actually there is another 
occasion of the same code that has no comment - if we think it is 
necessary we should have it in both.

>>> @@ -510,11 +529,12 @@ class ShellModel(gobject.GObject):
>>>                   home_activity = Activity(activity_info, activity_id,
>>> window)
>>>                   self._add_activity(home_activity)
>>>               else:
>>> -                home_activity.set_window(window)
>>> +                logging.debug('setting new window for existing activity')
>>> +                home_activity.add_window(window)
>>
>> This code gets called for each window including the launcher. To make that
>> clear maybe you can come up with another message.
>
> I tried to make things clearer with a comment block for the function,
> and more explicit debug lines. HTH.

Yes, looks good now.

>>>                   logging.debug('%s launched in %f seconds.',
>>>                       home_activity.get_type(), startup_time)
>>
>> With the current logic the debug log does not log what we expect,
>
> Here is where we were crashing in race conditions :-/ Anyway, I had
> that as a separate patch (posted together w this one). Now I've merged
> this change in the patch too.

Thanks - looks good now.

Regards,
    Simon