Hi Tim,

This is a very nice consolidation of the widget infrastructure! I have a
couple of questions and suggestion for enhancements.

Cheers


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

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)


> +        self.content_box_id = content_box_id
> +
> +        self.mutator_method_name = None
> +        ws_stack = exported_field.queryTaggedValue(LAZR_WEBSERVICE_EXPORTED)
> +        if ws_stack is None:
> +            # The field may be a copy, or similarly named to one we care
> +            # about.
> +            self.api_attribute = self.attribute_name
> +        else:
> +            self.api_attribute = ws_stack['as']
> +            mutator_info = ws_stack.get('mutator_annotations')
> +            if mutator_info is not None:
> +                mutator_method, mutator_extra = mutator_info
> +                self.mutator_method_name = mutator_method.__name__

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.

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

> +    @property
> +    def json_resource_uri(self):
> +        return simplejson.dumps(self.resource_uri)
> +
> +    @classmethod
> +    def _generate_id(cls):
> +        """Return a presumably unique id for this widget."""
> +        cls.last_id += 1
> +        return '%s-id-%d' % (cls.widget_type, cls.last_id)
> +
> +    @property
> +    def can_write(self):
> +        if canWrite(self.context, self.attribute_name):
> +            return True
> +        elif self.mutator_method_name is not None:
> +            # The user may not have write access on the attribute itself, but
> +            # the REST API may have a mutator method configured, such as
> +            # transitionToAssignee.
> +            return canAccess(self.context, self.mutator_method_name)
> +        else:
> +            return False

Hmm, I see why you need to look at the mutator then. Might want to add a
comment to explain it.

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

> +        self.edit_url = edit_url
> +        # TODO: check the exported_field to determine accept_empty
> +        self.accept_empty = simplejson.dumps(accept_empty)
> +        self.title = title
> +        self.json_attribute = simplejson.dumps(self.api_attribute)
> +        self.widget_css_selector = simplejson.dumps('#' + self.content_box_id)
> +
> +    @property
> +    def json_attribute_uri(self):
> +        return simplejson.dumps(self.resource_uri + '/' + self.api_attribute)
> +
> +
> +class TextLineEditorWidget(TextWidgetBase):
> +    """Wrapper for the lazr-js inlineedit/editor.js widget."""
> +
> +    __call__ = ViewPageTemplateFile('templates/text-line-editor.pt')
> +
> +    def __init__(self, context, exported_field, content_box_id=None,
> +                 title="Edit",
> +                 tag='h1', accept_empty=False, edit_view="+edit", edit_url=None, 
>                   default_text=None, initial_value_override=None, width=None):
>          """Create a widget wrapper.


> === modified file 'lib/lp/app/javascript/picker.js'
> --- lib/lp/app/javascript/picker.js	2011-01-20 21:26:07 +0000
> +++ lib/lp/app/javascript/picker.js	2011-01-27 06:08:34 +0000
> @@ -39,6 +39,7 @@
>      var remove_button_text = 'Remove';
>      var null_display_value = 'None';
>      var show_search_box = true;
> +    resource_uri = LP.client.normalize_uri(resource_uri)

Aren't you missing a var here?

>      var full_resource_uri = LP.client.get_absolute_uri(resource_uri);
>      var current_context_uri = LP.client.cache['context']['self_link'];
>      var editing_main_context = (full_resource_uri == current_context_uri);

> === modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
> --- lib/lp/code/templates/branchmergeproposal-index.pt	2010-10-10 21:54:16 +0000
> +++ lib/lp/code/templates/branchmergeproposal-index.pt	2011-01-27 06:08:34 +0000
> @@ -95,45 +95,23 @@
>    </div>
>  
>    <div id="commit-message" class="yui-g">
> +    <div tal:define="link menu/set_commit_message"
> +         tal:condition="link/enabled"
> +         tal:content="structure link/render"
> +         tal:attributes="class view/edit_commit_message_link_class">
> +      Set commit message
> +    </div>
> +    <tal:widget replace="structure view/commit_message_html"/>
>    </div>
>  
>    <div id="description" class="yui-g">
> +    <div tal:define="link menu/set_description"
> +         tal:condition="link/enabled"
> +         tal:content="structure link/render"
> +         tal:attributes="class view/edit_description_link_class">
> +      Set description
> +    </div>
> +    <tal:widget replace="structure view/description_html"/>
>    </div>
>  
>    <div class="yui-g" tal:condition="view/has_bug_or_spec">
> 

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?