On Sat, Apr 19, 2008 at 6:41 AM, Tomeu Vizoso <[EMAIL PROTECTED]> wrote: > On Wed, Apr 16, 2008 at 6:30 PM, Eben Eliason <[EMAIL PROTECTED]> wrote: > > The former patch cut some comments that should have been cut (and now > > have been) in the initial visual patch. This eliminates those parts > > of the patch. > > + self._title_entry.props.widget.connect('focus-out-event', > + > self._title_entry_focus_out_event_cb) > > I'd indent the second line just two additional tabs to the right.
I started there. The problem is that it's currently tabbed over as far as it can go before breaking the 80 char boundary. Which style convention should we break? > + self._title_entry.connect('key-press-event', > + self._title_entry_key_press_event_cb) > > This as well as the above would go inside _create_title_entry(). Makes sense. > + if event.key == hippo.KEY_RETURN: > + self._set_title(entry.props.text) > + self._title_entry.set_visible(False) > + self._title.set_visible(True) > + elif event.key == hippo.KEY_ESCAPE: > + entry.props.text = self._title.props.text > + self._title_entry.set_visible(False) > + self._title.set_visible(True) > > I wonder if hardcoding the return and escape keys are really needed, > or if gtk has a better way of doing this. Hmm, I'm not sure. I'll happily change this if someone has a better way. > + self._title_entry.set_visible(False) > + self._title.set_visible(True) > > These two lines are repeated after every time we end editing the > title, can this duplication be removed? Sure. Any thoughts on an appropriate function name? _restore_title_label, _title_edit_completed ? > + def _set_title(self, title): > + if title == '': > + self._title_entry.props.text = self._title.props.text > > I guess that you don't want to let the user remove completely the > title of an entry, can we make it more explicit? Indeed, that's the idea. Do you mean I should add a comment to this effect above the conditional, or is there a way the code could read more clearly itself? > self._title.props.text = self._format_title() + _(' Activity') > + self._title_entry.props.text = self._format_title() + _(' > Activity') > > We have a big problem here: https://dev.laptop.org/ticket/6875 . > "Activity" should be a formatting thing, shouldn't get into the > datastore. Yes, I noticed this problem while making this patch. I'm happy to see there is already a ticket, as it was on my list to enter one for it anyway. I think I'd actually recommend cutting the title formatting completely, and simply depending upon the unique visual treatment to identify the bundle for now. I'll think about it, and perhaps provide a separate patch for it. Do you think it should go in along with this patch? Thanks for the reviews, Tomeu! - Eben _______________________________________________ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar