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

[Simon Schampijer]

On 02/12/2011 05:27 PM, martin.langhoff at gmail.com wrote:
> From: Martin Langhoff<martin at laptop.org>
>
> Now activities have a stack of windows (Activity._windows). The
> lowest still-valid window in the stack is considered the main window.
>
> When a window is closed, the shell finds what activity had that window,
> tells it to remove it from its stack. If that was the last window
> in the activities' stack, the activity is marked as closed.
>
> With this patch, activities can switch from one window to another
> safely, without sugar shell losing track of them. Activities based
> on AdobeAIR have been seen to switch windows (perhaps when they navigate
> from one SWF to another).
>
> This replaces the one-window = one-activity model, where the moment
> an activity closed one window (perhaps one of many) it was considered
> closed by the shell.
> ---
> Amended version, with a fixup in the call to wm.get_sugar_window_type().

Hi Martin,

thanks for your patch and the detailed description. The case you 
describe is valid and I know that you have been testing your patch 
thoroughly. I like the general approach - just a few small comments inline.

Regards,
    Simon

>   src/jarabe/model/shell.py |   81 +++++++++++++++++++++++++++++---------------
>   1 files changed, 53 insertions(+), 28 deletions(-)
>
> diff --git a/src/jarabe/model/shell.py b/src/jarabe/model/shell.py
> index 661e370..135d1a1 100644
> --- a/src/jarabe/model/shell.py
> +++ b/src/jarabe/model/shell.py
> @@ -62,11 +62,12 @@ class Activity(gobject.GObject):
>               the "type" of activity being created.
>           activity_id -- unique identifier for this instance
>               of the activity type
> -        window -- Main WnckWindow of the activity
> +        _windows -- WnckWindows registered for the activity. The lowest
> +                    one in the stack is the main window.
>           """
>           gobject.GObject.__init__(self)
>
> -        self._window = None
> +        self._windows = []
>           self._service = None
>           self._activity_id = activity_id
>           self._activity_info = activity_info
> @@ -74,7 +75,7 @@ class Activity(gobject.GObject):
>           self._launch_status = Activity.LAUNCHING
>
>           if window is not None:
> -            self.set_window(window)
> +            self.add_window(window)
>
>           self._retrieve_service()
>
> @@ -96,15 +97,20 @@ class Activity(gobject.GObject):
>
>       launch_status = gobject.property(getter=get_launch_status)
>
> -    def set_window(self, window):
> -        """Set the window for the activity
> -
> -        We allow resetting the window for an activity so that we
> -        can replace the launcher once we get its real window.
> -        """
> +    def add_window(self, window):
> +        """Add a window to the windows stack."""
>           if not window:
>               raise ValueError('window must be valid')
> -        self._window = window
> +        self._windows.append(window)
> +
> +    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.

> +                self._windows.remove(wnd)
> +                return True
> +        return False
>
>       def get_service(self):
>           """Get the activity service
> @@ -118,8 +124,8 @@ class Activity(gobject.GObject):
>
>       def get_title(self):
>           """Retrieve the application's root window's suggested title"""
> -        if self._window:
> -            return self._window.get_name()
> +        if self._windows:
> +            return self._windows[0].get_name()
>           else:
>               return ''
>
> @@ -171,31 +177,44 @@ class Activity(gobject.GObject):
>
>       def get_xid(self):
>           """Retrieve the X-windows ID of our root window"""
> -        if self._window is not None:
> -            return self._window.get_xid()
> +        if self._windows:
> +            return self._windows[0].get_xid()
>           else:
>               return None
>
> +    def has_xid(self, xid):
> +        """Retrieve the X-windows ID of our root window"""
> +        if self._windows:
> +            for wnd in self._windows:
> +                if wnd.get_xid() == xid:
> +                    return True
> +        return None

We should return False here if we do not find a window and for the 
docstring I would recommend something like "Check if an X-window with 
the given xid is in the windows stack".

>       def get_window(self):
>           """Retrieve the X-windows root window of this application
>
> -        This was stored by the set_window method, which was
> -        called by HomeModel._add_activity, which was called
> +        This was stored by the add_window method, which was
> +        called by HomeModel._add_activity, which was called
>           via a callback that looks for all 'window-opened'
>           events.
>
> +        We keep a stack of the windows. The lowest window in the
> +        stack that is still valid we consider the main one.
> +
>           HomeModel currently uses a dbus service query on the
>           activity to determine to which HomeActivity the newly
>           launched window belongs.
>           """
> -        return self._window
> +	if self._windows:
> +            return self._windows[0]
> +        return None
>
>       def get_type(self):
>           """Retrieve the activity bundle id for future reference"""
> -        if self._window is None:
> +        if not self._windows:
>               return None
>           else:
> -            return wm.get_bundle_id(self._window)
> +            return wm.get_bundle_id(self._windows[0])
>
>       def is_journal(self):
>           """Returns boolean if the activity is of type JournalActivity"""
> @@ -211,7 +230,7 @@ class Activity(gobject.GObject):
>
>       def get_pid(self):
>           """Returns the activity's PID"""
> -        return self._window.get_pid()
> +        return self._windows[0].get_pid()

Not introduced by your patch, but I would do a check here as well if we 
have a window already (we do for all the other calls).

if not self._windows:
     return None

>       def get_bundle_path(self):
>           """Returns the activity's bundle directory"""
> @@ -230,8 +249,8 @@ class Activity(gobject.GObject):
>       def equals(self, activity):
>           if self._activity_id and activity.get_activity_id():
>               return self._activity_id == activity.get_activity_id()
> -        if self._window.get_xid() and activity.get_xid():
> -            return self._window.get_xid() == activity.get_xid()
> +        if self._windows[0].get_xid() and activity.get_xid():
> +            return self._windows[0].get_xid() == activity.get_xid()
>           return False
>
>       def _get_service_name(self):
> @@ -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.

> -            if wm.get_sugar_window_type(window) != 'launcher':
> +            if wm.get_sugar_window_type(window) != 'launcher' \
> +                    and home_activity.get_launch_status() == Activity.LAUNCHING:
>                   self.emit('launch-completed', home_activity)

>                   startup_time = time.time() - home_activity.get_launch_time()
>                   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, we 
fail to get the type of the main window. The window stack contains the 
launcher window and the main window at this particular time, and as we 
call 'self._windows[0]' in 'get_type' it will return None as we do try 
to get the type of the launcher window.

> @@ -524,12 +544,17 @@ class ShellModel(gobject.GObject):
>
>       def _window_closed_cb(self, screen, window):
>           if window.get_window_type() == wnck.WINDOW_NORMAL:
> -            if self._get_activity_by_xid(window.get_xid()) is not None:
> -                self._remove_activity_by_xid(window.get_xid())

"self._remove_activity_by_xid" is not used anymore after your patch 
"self._remove_activity(activity)" is now - so we can remove that method.

> +            xid = window.get_xid()
> +            activity = self._get_activity_by_xid(xid)
> +            if activity is not None:
> +                activity.remove_window_by_xid(xid)
> +                if activity.get_window() is None:
> +                    logging.debug('last window gone - remove activity %s' % activity)
> +                    self._remove_activity(activity)
>
>       def _get_activity_by_xid(self, xid):
>           for home_activity in self._activities:
> -            if home_activity.get_xid() == xid:
> +            if home_activity.has_xid(xid):
>                   return home_activity
>           return None
>