[Sugar-devel] [PATCH] Sugar-toolkit: Pack page in ToolbarButton when is conected to the window - OLPC #10930

[Simon Schampijer]

Hi Gonzalo,

thanks for working on this!

title: s/conected/connected

On 07/11/2011 06:08 PM, godiard at sugarlabs.org wrote:
> From: Gonzalo Odiard<godiard at gmail.com>
>
> To initialize keybindings, the buttons in the toolbars need be attached
> to the activity. This patch pack the page to the toolbarbutton when this is
> added to the activity to enable the buttons keybindings initialization.

I would use the term accelerators here instead of keybindings. Maybe 
like this:

"To add the accelerator to the ToolButton the activity must have set the 
'sugar-accel-group' before. The patch does make the ToolbarButton listen 
to the 'hierarchy-changed' signal and repack itself accordingly. Since 
the ToolButtons of the subtoolbar do listen to 'hierarchy-changed' as 
well to set the accelerator they will set it accordingly.

This fixes the accelerators for new-style-toolbar activities like 
Terminal, TurtleArt and Paint, more info in #10930.
"

>   src/sugar/graphics/toolbarbox.py |    9 +++++++++
>   1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/src/sugar/graphics/toolbarbox.py b/src/sugar/graphics/toolbarbox.py
> index b674e8d..7e6023f 100644
> --- a/src/sugar/graphics/toolbarbox.py
> +++ b/src/sugar/graphics/toolbarbox.py
> @@ -36,6 +36,15 @@ class ToolbarButton(ToolButton):
>           self.connect('clicked',
>                   lambda widget: self.set_expanded(not self.is_expanded()))
>
> +        self.connect('hierarchy-changed', self._hierarchy_changed_cb)

Should be two underscores:  self.__hierarchy_changed_cb

> +    def _hierarchy_changed_cb(self, tool_button, previous_toplevel):
> +        if hasattr(self.parent, 'owner'):
> +            if self.page_widget:
> +                self._unparent()
> +                self.parent.owner.pack_start(self.page_widget)
> +                self.set_expanded(False)
> +
>       def get_toolbar_box(self):
>           if not hasattr(self.parent, 'owner'):
>               return None

The patch does double the "No gtk.AccelGroup in the top level window" 
warnings printed in the logs. I don't think those are really useful for 
anything I would remove that (in a separate patch for master) and if at 
all add a comment there.

Regards,
    Simon