[Sugar-devel] [PATCH sugar-toolkit-gtk3] Reimplement Palettes for GTK3

[Benjamin Berg]

Hello,

looks good to me! Just a few small comments, nothing big.

On Thu, 2011-12-15 at 01:53 +0000, Daniel Drake wrote:
> [SNIP] 
>      def _update_separators(self):
> -        visible = self.menu.get_children() or  \
> -                  self._content.get_children()
> +        visible = self._content.get_children()
>          self._separator.props.visible = visible
>  
> -        visible = self.menu.get_children() and  \
> -                  self._content.get_children()
> -        self._menu_content_separator.props.visible = visible

Hm, this might be not quite correct in the menu case. But it could be
that the menu is simply broken in this regard (ie. primary/secondary
palette) currently.

> [SNIP]
> +        # The remaining challenge is tracking when the mouse enters or leaves
> +        # the invoker area. While the appropriate GtkMenu grab is active,
> +        # we do get informed of such events, however these events will only
> +        # arrive if the user has entered the menu. If the user hovers over
> +        # the invoker and then leaves the invoker without entering the palette,
> +        # we get no enter/leave event.
> +        # We work around this by tracking mouse motion events. When the mouse
> +        # moves, we compare the mouse coordinates to the region occupied by the
> +        # invoker, and this lets us track enter/leave for the invoker widget.

This mouse enter/leave thing for the invoker totally surprised me too. I
am kind of suspecting some bug inside GDK here (in conjunction with
client side windows maybe).

Other than that, the idea of just listening to all enter/leave events on
all menus seems like a pretty simple solution.

> [SNIP]
> +class _PaletteWindowWidget(Gtk.Window):
> [SNIP]
> +
> +    def do_button_release_event(self, event):
> +        alloc = self.get_allocation()
> +        x, y = self.get_window().get_position()
> +
> +        in_window = event.x_root >= x and event.x_root < x + alloc.width and \
> +          event.y_root >= y and event.y_root < y + alloc.height
> +
> +        if not in_window:
> +            self.popdown()
> +            return True

This doesn't seem to make much sense to me. I do not see any code to
grab the input in the normal window case. So the event should not be
fired if the mouse is outside of the window.

> [SNIP] 
> @@ -234,12 +233,12 @@ class _ToolbarPalette(PaletteWindow):
>          PaletteWindow.on_invoker_leave(self)
>          self._set_focus(False)
>  
> -    def on_enter(self, event):
> -        PaletteWindow.on_enter(self, event)
> +    def on_enter(self):
> +        PaletteWindow.on_enter(self)
>          self._set_focus(True)
>  
> -    def on_leave(self, event):
> -        PaletteWindow.on_enter(self, event)
> +    def on_leave(self):
> +        PaletteWindow.on_enter(self)
>          self._set_focus(False)

Hm, are you sure, that on_enter is correct here? (yeah, it was there
before ...)

Benjamin