[sugar] Merging sugar-toolkit changes from tomeu repository
Tomeu Vizoso
tomeu
Fri Mar 28 15:13:43 EDT 2008
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
More information about the Sugar-devel
mailing list