[Sugar-devel] review of the new toolbars implementation

[Tomeu Vizoso]

Hi,

have been looking at the code and have these comments:

+    def set_toolbar_box(self, toolbar_box):
+        # make more consistent using ToolbarBox instead of Toolbox
+        self.set_toolbox(toolbar_box)

Maybe mark set_toolbox as DEPRECATED?

+        ToolButton.__init__(self, 'activity-stop',
+                tooltip=_('Stop'),
+                accelerator='<Ctrl>Q',
+                **kwargs)

If you wanted to make me a happier person, you would set the
properties that don't fit in a single line in their own, like
self.props.tooltip = _('Stop') But is fine as-is.

+        self.__private = RadioToolButton(

Why two underscores? (There are other instances of this)

+        activity.connect('shared', self.__update_share)

Maybe name it self.__update_share_cb instead?

+        if shared_activity:

Should be "is not None"? There are other instances of this.

+        self.paste = ToolButton('edit-paste')

Why not using PasteButton?

+        button.__palette_label = label

Accessing a private member of another class. (There are other instances of this)

+from sugar.graphics.palette import _PopupAnimation, _PopdownAnimation

Those are privates, we may need to add API to do what you want, or
move a class into s.g.palette. Maybe you need an invoker?

+    toolbar = property(get_toolbar)

Would that be a ToolbarBox? Then maybe should we called toolbar_box?

+        return bool(self.toolbar) and bool(self._page) and \
+                self.toolbar._expanded_page() == self._page

More castings from None to False, specially dangerous as _page could
be a container that evaluates to False even if it's not None.

+            self.toolbar._shrink_page(self._page)

Access to private member of another class. There are other instances
around this one.

+        expanded = self.toolbar._expanded_page()

Better use nouns for variables, such as expanded_page.

+        page_no = self.__notebook.page_num(widget._page)

Abbreviation, can we get something more verbose like page_num or page_number?

+        self.__notebook = gtk.Notebook()

Why do we need a notebook?

+class _Palette(gtk.Window):

The palette class is very tricky and is a frequent source of bugs, we
shouldn't duplicate it. Either we add what you need to the existing
Palette, or we split it out in a BasePalette and Palette and then
inherit from BasePalette for this.

+def _align(box_class, widget):

Can we name it _align_center, _align_bottom, etc? As this isn't a
generic alignment func.

-        self.connect('clicked', self.__button_clicked_cb)

This is very likely to cause problems in activities that override
do_clicked without knowing what the base class does there. Consider
listening to the signal and calling a method that can be overridden
from there instead.

Thanks!

Tomeu