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

[Tomeu Vizoso]

On Sat, Apr 19, 2008 at 8:11 PM, Eben Eliason <eben.eliason at gmail.com> wrote:
> 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?

Sorry, I explained badly. I meant this:

self._title_entry.props.widget.connect('focus-out-event',
        self._title_entry_focus_out_event_cb)

If you cannot make the indentation beautiful, then fall back to the
simplest rule: two extra tabs (one may be confusing because of code
blocks).

>  >  +        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.

I think we want to apply the changes when the 'activate' and
'focus-out' signals are called. About when to undo the changes, seems
like we need to listen for the escape key as you are doing.

>  >  +            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 ?

Can it be merged somehow inside _set_title()? And perhaps this method
should be called instead _apply_title_change()?

>  >  +    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?

Hopefully the later. You are not doing anything specially complicated,
so I would prefer if we could avoid the extra comment by making the
intentions of the code more explicit.

>  >              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?

I think it could got in this patch if it's better for you, but
attaching a more focused patch to the ticket may be better process.

Thanks,

Tomeu