[sugar] [PATCH] Add support for inline renaming of Journal entries
Eben Eliason
eben.eliason at gmail.com
Sat Apr 19 14:11:51 EDT 2008
On Sat, Apr 19, 2008 at 6:41 AM, Tomeu Vizoso <tomeu at tomeuvizoso.net> wrote:
> On Wed, Apr 16, 2008 at 6:30 PM, Eben Eliason <eben.eliason at gmail.com> 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
More information about the Sugar
mailing list