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 _______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel