[sugar] [PATCH] Add support for inline renaming of Journal entries
Tomeu Vizoso
tomeu at tomeuvizoso.net
Tue Apr 22 05:40:06 EDT 2008
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
>
More information about the Sugar
mailing list