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

[Martin Langhoff]

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.

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

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

cheers,



m
-- 
 martin.langhoff at gmail.com
 martin at laptop.org -- Software Architect - OLPC
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff