Code review comment for lp:~thumper/launchpad/refactor-lazrjs-text-widgets

On Fri, 28 Jan 2011 05:43:28 Francis J. Lacoste wrote:
> > === modified file 'lib/canonical/widgets/lazrjs.py'
> > +class WidgetBase:
> > + """Useful methods for all widgets."""
> >
> > + def __init__(self, context, exported_field, content_box_id):
> > + self.context = context
> > + self.exported_field = exported_field
> > +
> > + self.request = get_current_browser_request()
>
> That makes us depend on global state. To unit test widget python code, it
> will require setting the interaction.

The widgets have always depended on global state as they call the
canonical_url method. This just makes that more dependency explicit.

> Maybe it's not that much of a problem...
>
> > + self.attribute_name = exported_field.__name__
> > +
> > + if content_box_id is None:
> > + content_box_id = "edit-%s" % self.attribute_name
>
> We need to cater for the use-case when we'll have multiple edit widget on
> the page, so I'd suggest putting in a counter there. (That's why we were
> using _generate_id() before)

As we discussed, the _generate_id method was never called from any call-site
as they almost all used "edit-foo" where foo was the attribute being edited.
I updated the default to do this, and removed the extra parameter from the
call sites to make them simpler.

If for some reason there are multiple widgets on a page for editing a single
attribute name (perhaps like bug task assignees), then the call site will need
to specify the id.

> Why do we need to care about the mutator method name? It should be
> transparent: you simply PATCH the field to the new value and lazr.restful
> will call the mutator appropriately automatically. The 'as' stuff is
> required though.

Comment added.

> > + @property
> > + def resource_uri(self):
> > + return canonical_url(self.context, force_local_path=True)
> > +
>
> Why force_local_path? (Subtle hint to add a comment there.)

Docstring added.

> > +class TextWidgetBase(WidgetBase):
> > +
> > + def __init__(self, context, exported_field, content_box_id,
> > + accept_empty, title, edit_view, edit_url):
> > + super(TextWidgetBase, self).__init__(
> > + context, exported_field, content_box_id)
> > + if edit_url is None:
> > + edit_url = canonical_url(self.context, view_name=edit_view)
>
> If edit_url can be None, why not make it an optional argument? I see that
> you made it so in the descendant class.

No I don't want it optional here. TextWidgetBase is an abstract base class
for the text editor widgets. Even though they have the paramter as optional,
it is explicitly passed through to the base class constructor.

> > + resource_uri = LP.client.normalize_uri(resource_uri)
>
> Aren't you missing a var here?

Nope. It is altering the function parameter.

> WHy do we have 'Set description' links in addition to the widget? If it's
> for graceful degradation, could we make that part of the widget API? Like
> it is for the inline textline widget?

This is due to the behaviour on the merge proposal page for empty text areas
not showing, and a link appearing. As we discussed it may be preferable to
have that behaviour added to the widget itself, but I'm not doing this just
now.