launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/pickers-pickers-everywhere into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/pickers-pickers-everywhere/+merge/64602

Summary
=======
Per the linked bug, we have a ton of repeated javascript as a result of multiple pickers on the advanced bug search page (and anywhere else that uses) the form macros for the picker.

This branch blocks off repetitions to the extent possible; it puts creation the make_picker function behind a guard so that it is only added to the template for the first picker that needs it, and restructure make_picker so that it can be used by the setup code for subsequent pickers on a page. Ideally we would just move make_picker to a js file, but the form-macros are somewhat fragile and this was the more expedient fix.

Restructuring can always be done on form macros as part of work on later branches.

Pre-implementation notes
========================
Spoke with Curtis Hovey about the idea of putting a guard on parts of the rendered template.

Implementation details
======================
lib/lp/app/widgets/popup.py
---------------------------
Created a widget_rendered property on the pickers that only returns true for the first use of a given popup type. This is used to provide a guard in the template.

lib/lp/app/widgets/templates/form-picker-macros.pt
--------------------------------------------------
Broke up the javascript for getting and attaching a picker to a text input into two parts; the first creates the make_picker function, and the second wires it into the input. The part creating a picker has been restructured so it can be used for multiple widgets, and is behind the widget_rendered guards so the macro only emits it once in a template.

lib/lp/app/widgets/tests/test_popup.py
--------------------------------------
Tests.

Tests
=====
bin/test -vvcm lp.app.widgets

Q/A
===
You should only see one instance of make_picker on the link provided in the bug; additionally, the pickers should work as expected on the bug.

Lint
====
Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/widgets/templates/form-picker-macros.pt
  lib/lp/app/widgets/tests/test_popup.py
  lib/lp/app/widgets/popup.py

./lib/lp/app/widgets/popup.py
      64: redefinition of function 'widget_rendered' from line 47
    
This is a result of the property syntax in python 2.6; I'm not sure the linter has been updated to deal with this. If we have a standard against this sort of property, I'll change the code.
-- 
https://code.launchpad.net/~jcsackett/launchpad/pickers-pickers-everywhere/+merge/64602
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/pickers-pickers-everywhere into lp:launchpad.

=== modified file 'lib/lp/app/widgets/popup.py'
--- lib/lp/app/widgets/popup.py	2011-06-01 19:13:54 +0000
+++ lib/lp/app/widgets/popup.py	2011-06-14 20:02:18 +0000
@@ -43,6 +43,28 @@
     # Defaults to self.vocabulary.displayname.
     header = None
 
+    @property
+    def widget_rendered(self):
+        # We manually create the widget name since we don't want different
+        # popups colliding, and we have to put the attr on the request so all
+        # instances of the popup on this request can check the attr.
+        attr = self.__class__.__name__ + '_rendered'
+        if hasattr(self.request, attr):
+            return getattr(self.request, attr)
+        else:
+            # The widget has not rendered before this. We return False, but
+            # set the attribute to true, so future checks will see that it
+            # has rendered. It would be better to set this in __init__ or
+            # similar, but then this first check also sees True, and we never
+            # render the part of the template this is used to guard.
+            setattr(self.request, attr, True)
+            return False
+
+    @widget_rendered.setter
+    def widget_rendered(self, val):
+        attr = self.__class__.__name__ + '_rendered'
+        setattr(self.request, attr, val)
+
     @cachedproperty
     def matches(self):
         """Return a list of matches (as ITokenizedTerm) to whatever the

=== modified file 'lib/lp/app/widgets/templates/form-picker-macros.pt'
--- lib/lp/app/widgets/templates/form-picker-macros.pt	2011-06-03 18:23:23 +0000
+++ lib/lp/app/widgets/templates/form-picker-macros.pt	2011-06-14 20:02:18 +0000
@@ -29,21 +29,19 @@
     </tal:search_results>
 
     <tal:chooseLink replace="structure view/chooseLink" />
-    <script tal:content="structure string:
+
+    <script
+      tal:condition="not: view/widget_rendered"
+      tal:content="structure string:
     LPS.use('node', 'lp.app.picker', 'plugin', function(Y) {
         if (Y.UA.ie) {
             return;
         }
-
+        namespace = Y.namespace('lp.macros');
         // The vocabulary picker, created when used for the first time.
-        function make_picker() {
-            var config = {
-                header: ${view/header_text},
-                step_title: ${view/step_title_text},
-                extra_no_results_message: ${view/extra_no_results_message},
-                picker_type: '${view/picker_type}'
-            };
-            var picker = Y.lp.app.picker.create('${view/vocabulary_name}', config);
+        function make_picker(config) {
+            var vocab_name = config.vocabulary_name;
+            var picker = Y.lp.app.picker.create(vocab_name, config);
             if (config.extra_no_results_message !== null) {
                 picker.before('resultsChange', function (e) {
                     var new_results = e.details[0].newVal;
@@ -57,10 +55,27 @@
                 });
             }
             picker.plug(Y.lazr.TextFieldPickerPlugin,
-                        {input_element: '[id=\x22${view/input_id}\x22]'});
+                        {input_element: '[id=\x22' + config.input_id + '\x22]'});
             return picker;
         }
+        namespace.make_picker = make_picker;
+    });
+    "/>
+
+    <script tal:content="structure string:
+    LPS.use('node', 'lp.app.picker', 'plugin', function(Y) {
+        if (Y.UA.ie) {
+            return;
+        }
         var picker = null;
+        var config = {
+            vocabulary_name: '${view/vocabulary_name}',
+            header: ${view/header_text},
+            step_title: ${view/step_title_text},
+            extra_no_results_message: ${view/extra_no_results_message},
+            picker_type: '${view/picker_type}',
+            input_id: '${view/input_id}'
+        };
         Y.on('domready', function(e) {
             // Sort out the Choose... link.
             var show_widget_node = Y.one('#${view/show_widget_id}');
@@ -70,7 +85,7 @@
             show_widget_node.get('parentNode').removeClass('unseen');
             show_widget_node.on('click', function (e) {
                 if (picker === null) {
-                    picker = make_picker();
+                    picker = Y.lp.macros.make_picker(config);
                 }
                 picker.show();
                 e.preventDefault();

=== modified file 'lib/lp/app/widgets/tests/test_popup.py'
--- lib/lp/app/widgets/tests/test_popup.py	2011-06-02 16:12:05 +0000
+++ lib/lp/app/widgets/tests/test_popup.py	2011-06-14 20:02:18 +0000
@@ -24,6 +24,8 @@
             "test_invalid_chars+":
             Choice(vocabulary='ValidTeamOwner'),
             "test_valid.item":
+            Choice(vocabulary='ValidTeamOwner'),
+            "test_valid.second_item":
             Choice(vocabulary='ValidTeamOwner')}
         super(TestMetaClass, self).__init__(
             name, bases=bases, attrs=attrs, __doc__=__doc__,
@@ -47,6 +49,24 @@
             self.context, 'ValidTeamOwner')
         self.request = LaunchpadTestRequest()
 
+    def test_widget_rendered_one(self):
+        field = ITest['test_valid.item']
+        bound_field = field.bind(self.context)
+        picker_widget = VocabularyPickerWidget(
+            bound_field, self.vocabulary, self.request)
+        self.assertFalse(picker_widget.widget_rendered)
+        self.assertTrue(self.request.VocabularyPickerWidget_rendered)
+
+    def test_widget_rendered_multiple(self):
+        fields = (ITest['test_valid.item'], ITest['test_valid.second_item'])
+        bound_fields = [field.bind(self.context) for field in fields]
+        picker_widgets = [VocabularyPickerWidget(
+            bound_field, self.vocabulary, self.request)
+            for bound_field in bound_fields]
+        self.assertFalse(picker_widgets[0].widget_rendered)
+        self.assertTrue(self.request.VocabularyPickerWidget_rendered)
+        self.assertTrue(picker_widgets[1].widget_rendered)
+
     def test_widget_template_properties(self):
         # Check the picker widget is correctly set up for a field which has a
         # name containing only valid HTML ID characters.