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

[Eben Eliason]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Add-support-for-inline-renaming-of-Journal-entries.patch
Type: text/x-patch
Size: 4979 bytes
Desc: not available
Url : http://lists.laptop.org/pipermail/sugar/attachments/20080421/4d79c7f8/attachment.bin