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

[Simon Schampijer]

On 07/05/2011 09:49 AM, Simon Schampijer wrote:
> 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

Can I land this? This is in the latest OLPC image and tested.

Regards,
    Simon