[sugar] [PATCH] Add support for inline renaming of Journal entries

[Eben Eliason]

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