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

[Sascha Silbe]

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.

[...]

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


[...]
> +    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?


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


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


[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

-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 500 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20110702/badf5941/attachment.pgp>