[Sugar-devel] [PATCH sugar] Group members of a shared activity around it OLPC #10675

[Simon Schampijer]

On 07/02/2011 06:05 PM, Sascha Silbe wrote:
> Excerpts from Simon Schampijer's message of Sun Jun 26 14:30:06 +0200 2011:
>
>> This patch removes as well the dependency in the shell on the
>> neighborhood model which was a hack to determine the color of
>> a shared activity.
>
> s/removes as well/also removes/ ? But maybe it just sounds weird to my
> non-native speaker ears. And just a minor nitpick anyway.

Done.

> [...]
>
> Thanks for working on this!
>
>
> [src/jarabe/model/neighborhood.py]
>> @@ -375,6 +379,29 @@ class _Account(gobject.GObject):
> [...]
>> +    def __active_activity_changed_cb(self, model, home_activity):
>> +        room_handle = 0
>> +        home_activity_id = home_activity.get_activity_id()
>> +        for handle, activity_id in self._activity_handles.items():
>> +            if home_activity_id == activity_id:
>> +                room_handle = handle
>> +                break
>> +        if room_handle == 0:
>> +            activity_id = ''
>
> There are some subtleties involved with using the loop variable from
> outside the loop that I'd prefer to avoid. How about this:
>
>          for handle, activity_id in self._activity_handles.items():
>              if home_activity_id == activity_id:
>                  room_handle = handle
>                  break
>          else:
>              home_activity_id = ''
>
> And then use home_activity_id instead of activity_id as parameter to
> connection.SetCurrentActivity().

Done, but I did leave the 'if room_handle == 0:' in because I find the 
'else' clause confusing, had to try it out before I was clear what it is 
supposed to do.

> [...]
>> +    def __set_current_activity_error_cb(self, error):
>> +        logging.debug('_Account.__set_current_activity__error_cb %r', error)
>
> I wonder whether this should be a warning instead. Do we expect
> connection.SetCurrentActivity() to fail during normal operation?

Made it a warning, yes we do not think SetCurrentActivity should fail.

> [...]
>> @@ -433,6 +462,18 @@ class _Account(gobject.GObject):
> [...]
>> +
>> +                if buddy_handle == self._self_handle:
>> +                    home_model = shell.get_model()
>> +                    act = home_model.get_active_activity()
>> +                    if act.get_activity_id() == activity_id:
>
> We can spell out activity without hitting any length limitations AFAICS.

Done.

> [...]
>> @@ -443,23 +484,24 @@ class _Account(gobject.GObject):
> [...]
>> -                # Sometimes we'll get CurrentActivityChanged before we get to
>> -                # know about the activity so we miss the event. In that case,
>> -                # request again the current activity for this buddy.
>> -                connection = self._connection[CONNECTION_INTERFACE_BUDDY_INFO]
>> -                connection.GetCurrentActivity(
>> -                    buddy_handle,
>> -                    reply_handler=partial(self.__get_current_activity_cb,
>> -                                          buddy_handle),
>> -                    error_handler=partial(self.__error_handler_cb,
>> -                                          'BuddyInfo.GetCurrentActivity'))
>> +                if buddy_handle != self._self_handle:
>> +                    # Sometimes we'll get CurrentActivityChanged before we get to
>> +                    # know about the activity so we miss the event. In that case,
>> +                    # request again the current activity for this buddy.
>> +                    connection = self._connection[CONNECTION_INTERFACE_BUDDY_INFO]
>> +                    connection.GetCurrentActivity(
>> +                        buddy_handle,
>> +                        reply_handler=partial(self.__get_current_activity_cb,
>> +                                              buddy_handle),
>> +                        error_handler=partial(self.__error_handler_cb,
>> +                                              'BuddyInfo.GetCurrentActivity'))
>
> We should refactor _update_buddy_activities() in master to reduce the
> number of indentations and avoid the nested line continuations.

Yes, but first I would like to fix the outstanding bugs in this area so 
I don't have to rebase patches.

> [src/jarabe/model/shell.py]
> [...]
>> +    def add_shared_activity(self, activity_id, color):
>> +        self._shared_activities[activity_id] = color
>> +
>> +    def remove_shared_activity(self, activity_id):
>> +        del self._shared_activities[activity_id]
>
> Mixing up Collaboration with window management makes a real mess. We
> should let sugar-toolkit set up the icon the regular (X11) way and just
> display the client-provided icon in the Frame. Short term your patch is
> fine, though.
>
> Sascha
>

Thanks for the review,
    Simon