[sugar] Merging sugar-toolkit changes from tomeu repository

[Tomeu Vizoso]

On Fri, Mar 28, 2008 at 6:46 PM, Marco Pesenti Gritti
<mpgritti at gmail.com> wrote:
> On Fri, Mar 28, 2008 at 6:19 PM, Tomeu Vizoso <tomeu at tomeuvizoso.net> wrote:
>  > This one cleans up the code in *ToolButton quite a bit, following
>  >  Marco's suggestions.
>
>  +        self.keep = ToolButton('document-save', tooltip=_('Keep'))
>  +        self.keep.props.accelerator = '<Ctrl>S'
>
>  I'll just point out that I prefer if properties are all passed to the
>  constructor. If you still feel your bad code tastes are superior
>  though, you should feel free to ignore me :P

Ignored :)

>  +        accel_group = gtk.AccelGroup()
>  +        self.set_data('accel-group', accel_group)
>  +        self.add_accel_group(accel_group)
>
>  I think we should probably use sugar-accel-group here.

Done

>  +        self._label = gtk.AccelLabel('')
>  +        self._label.set_size_request(-1, style.zoom(style.GRID_CELL_SIZE) -
>  +                                         2 * self.get_border_width())
>
>  You are just changing this code but... bonus if you fix it up. We
>  should not use set_size_request here, if you change the font you are
>  screwed (it won't fit into the container anymore). Instead we should
>  use borders/padding/spacing and do the calculations to make our picky
>  designer happy in style (there are some examples of it there).

Heh, this gets delayed for U3 :P

>  +        requisition.width = max(requisition.width, label_width)
>  +
>          requisition.width = max(requisition.width, self._full_request[0])
>
>  We can pass all the 3 values to max, should be cleaner.

Done

>  +    def set_tooltip(self, tooltip):
>  +        if self._tooltip != tooltip:
>  +            self._tooltip = tooltip
>  +            if tooltip and self.palette is None:
>  +                self.palette = Palette(tooltip)
>  +            elif self.palette:
>  +                self.palette.set_primary_text(tooltip)
>
>  Is the if at the top really necessary? I can't think of use cases
>  where we would be hitting it.
>  Also I'd replace the palette if you set_tooltip and a Palette already exist.

How so?

>  +    def set_accelerator(self, accelerator):
>  +        if self._accelerator != accelerator:
>  +            self._accelerator = accelerator
>
>  Same as above, the if seem unnecessary to me.
>
>  +    accelerator = gobject.property(type=str, setter=set_accelerator,
>  +            getter=get_accelerator)
>
>  NITPICK! I'd rather align getter under type. I've seen other similar cases.
>
>  +def _really_set_accelerator(tool_button):
>
>  I'd rename to _add_accellerator.

Done, typo included.

Thanks,

Tomeu