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

[Tomeu Vizoso]

r+

On Mon, Apr 21, 2008 at 10:54 PM, Eben Eliason <eben.eliason at gmail.com> wrote:
> On Mon, Apr 21, 2008 at 10:01 AM, Tomeu Vizoso <tomeu at tomeuvizoso.net> wrote:
>  > 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).
>
>  Done.
>
>
>  >  >  >  +        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.
>
>  I connected to the activate signal on the widget, as suggested.
>
>
>  >
>  >  >  >  +            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()?
>
>  Done.
>
>
>  >
>  >  >  >  +    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.
>
>  There is now an _apply_title_change and a _cancel_title_change.  The
>  condition in question simply calls cancel_title_change to clearly
>  indicate what's happening.
>
>
>  >
>  >  >  >              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.
>
>  I ignored this problem for this patch.  I'll create a new patch to
>  clean this up.
>
>
>  Thanks!
>
>  - Eben
>