Merge lp:~thumper/launchpad/recipe-syntax into lp:launchpad

[Tim Penhey]

[r=sinzui][ui=none][bug=670475] Add a help link to the recipe text description when adding a new recipe.

Show a help link in the description of the recipe text widget, and have that bring up some meaningful help.

As discussed on the list I'm using an optional field modifier that gets the launchpad form infrastructure
to render the widget's description using the structured text, so html is rendered un-escaped.

tests:
   lp.app.browser.tests.test_launchpadform

[Curtis Hovey (sinzui)]

Hi Tim.

Your implementation looks good. I have a few concerns about markup and text that you may want to consider before you land.

@lib/lp/app/templates/launchpad-form.pt

launchpad-form does not look very DRY. I wonder if we could define a metal macro before the launchpad form macro based on the first definition and use it.

  <metal:help-hint metal:define-macro="help_hint">
    <tal:widget-help
        define="use_structure context/query:has-structured-doc"
        condition="context/hint">
     <p class="formHelp" tal:content="context/hint"
         tal:condition="not: use_structure">
        Some Help Text
      </p>
      <p class="formHelp" tal:content="structure context/hint"
         tal:condition="use_structure">
        Some Structured Help Text
      </p>
    </tal:widget-help>
  </metal:help-hint>

then

  <metal:help-hint use-macro="widget/@@launchpad_form/help_hint">

Writing macros is somewhat dodgy. Maybe we should report a bug in launchpad-web and set someone the task of cleaning up the rules. The clean up might reveal inconsistencies in the file.

@lib/lp/code/browser/sourcepackagerecipe.py

Some users will see odd leading and trailing underlines on hover because there is white space between the anchor tags and content. While HTML 3.0+ requires renders to strip the leanding and trailing space, few browsers have implemented the rule.

                <a href="/+help/recipe-syntax.html" target="help"
                  >Syntax help&nbsp;
                  <span class="sprite maybe">
                    <span class="invisible-link">Help</span>
                  </span></a>

@lib/lp/code/help/recipe-syntax.html

I believe we are using an > to indicate the user will leave the site/open a new window.
    <a href="https://help.launchpad.net/Place" target="_blank">place &gt;</a>

s/ndash/mdash/ ndash is for a range of numbers, mdash it used to construct a clause

review: Approve (code)

[Tim Penhey (thumper)]

Thanks Curtis.

I've updated the code following your recommendations.

=== modified file 'Makefile'
--- Makefile	2010-11-25 02:28:35 +0000
+++ Makefile	2010-11-26 02:05:36 +0000
@@ -139,6 +139,9 @@
 	touch $(CODEHOSTING_ROOT)/config/launchpad-lookup.txt
 
 build: compile apidoc jsbuild css_combine
+	mkdir -p $(CODEHOSTING_ROOT)/mirrors
+	touch $(CODEHOSTING_ROOT)/rewrite.log
+	chmod 777 $(CODEHOSTING_ROOT)/rewrite.log
 
 css_combine: sprite_css bin/combine-css
 	${SHHH} bin/combine-css

=== modified file 'lib/lp/app/browser/configure.zcml'
--- lib/lp/app/browser/configure.zcml	2010-11-09 04:24:09 +0000
+++ lib/lp/app/browser/configure.zcml	2010-11-26 02:05:36 +0000
@@ -551,6 +551,14 @@
       name="fmt"
       />
 
+  <!-- TALES query: namespace -->
+  <adapter
+      for="zope.app.form.interfaces.IWidget"
+      provides="zope.traversing.interfaces.IPathAdapter"
+      factory="lp.app.browser.launchpadform.WidgetHasStructuredDoc"
+      name="query"
+      />
+
   <adapter
       factory="lp.app.browser.tales.LaunchpadLayerToMainTemplateAdapter"
       />

=== modified file 'lib/lp/app/browser/launchpadform.py'
--- lib/lp/app/browser/launchpadform.py	2010-11-23 23:22:27 +0000
+++ lib/lp/app/browser/launchpadform.py	2010-11-26 02:05:36 +0000
@@ -9,6 +9,7 @@
 __all__ = [
     'action',
     'custom_widget',
+    'has_structured_doc',
     'LaunchpadEditFormView',
     'LaunchpadFormView',
     'ReturnToReferrerMixin',
@@ -32,9 +33,12 @@
 from zope.formlib.form import action
 from zope.interface import (
     classImplements,
+    implements,
     providedBy,
     )
 from zope.interface.advice import addClassAdvisor
+from zope.traversing.interfaces import ITraversable, TraversalError
+
 
 from canonical.launchpad.webapp.interfaces import (
     IAlwaysSubmittedWidget,
@@ -511,3 +515,31 @@
 
     next_url = _return_url
     cancel_url = _return_url
+
+
+def has_structured_doc(field):
+    """Set an annotation to mark that the field's doc should be structured."""
+    field.setTaggedValue('has_structured_doc', True)
+    return field
+
+
+class WidgetHasStructuredDoc:
+    """Check if widget has structured doc.
+
+    Example usage::
+        tal:condition="widget/query:has-structured-doc"
+    """
+
+    implements(ITraversable)
+
+    def __init__(self, widget):
+        self.widget = widget
+
+    def traverse(self, name, furtherPath):
+        if name != 'has-structured-doc':
+            raise TraversalError("Unknown query %r" % name)
+        if len(furtherPath) > 0:
+            raise TraversalError(
+                "There should be no further path segments after "
+                "query:has-structured-doc")
+        return self.widget.context.queryTaggedValue('has_structured_doc')

=== added file 'lib/lp/app/browser/tests/test_launchpadform.py'
--- lib/lp/app/browser/tests/test_launchpadform.py	1970-01-01 00:00:00 +0000
+++ lib/lp/app/browser/tests/test_launchpadform.py	2010-11-26 02:05:36 +0000
@@ -0,0 +1,66 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the lp.app.browser.launchpadform module."""
+
+__metaclass__ = type
+
+from zope.interface import Interface
+from zope.schema import Text
+
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.browser.launchpadform import (
+    has_structured_doc,
+    LaunchpadFormView,
+    )
+from lp.testing import TestCase, test_tales
+
+
+class TestInterface(Interface):
+    """Test interface for the view below."""
+
+    normal = Text(title=u'normal', description=u'plain text')
+
+    structured = has_structured_doc(
+        Text(title=u'structured',
+             description=u'<strong>structured text</strong'))
+
+
+class TestView(LaunchpadFormView):
+    """A trivial view using the TestInterface."""
+
+    schema = TestInterface
+
+
+class TestHasStructuredDoc(TestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def _widget_annotation(self, widget):
+        return widget.context.queryTaggedValue('has_structured_doc')
+
+    def test_has_structured_doc_sets_attribute(self):
+        # Test that has_structured_doc sets the field annotation.
+        request = LaunchpadTestRequest()
+        view = TestView(None, request)
+        view.initialize()
+        normal_widget, structured_widget = list(view.widgets)
+        self.assertIs(None, self._widget_annotation(normal_widget))
+        self.assertTrue(self._widget_annotation(structured_widget))
+
+
+class TestQueryTalesForHasStructuredDoc(TestCase):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_query_tales(self):
+        # Test that query:has-structured-doc gets sets the field annotation.
+        request = LaunchpadTestRequest()
+        view = TestView(None, request)
+        view.initialize()
+        normal_widget, structured_widget = list(view.widgets)
+        self.assertIs(None, test_tales(
+                'widget/query:has-structured-doc', widget=normal_widget))
+        self.assertTrue(test_tales(
+                'widget/query:has-structured-doc', widget=structured_widget))

=== modified file 'lib/lp/app/templates/launchpad-form.pt'
--- lib/lp/app/templates/launchpad-form.pt	2010-10-15 01:27:04 +0000
+++ lib/lp/app/templates/launchpad-form.pt	2010-11-26 02:05:36 +0000
@@ -123,10 +123,7 @@
           </div>
           <div class="message" tal:condition="error"
                tal:content="structure error">Error message</div>
-          <p class="formHelp"
-             tal:condition="widget/hint"
-             tal:content="widget/hint">Some Help Text
-          </p>
+          <metal:help-hint use-macro="widget/@@launchpad_form/help_hint" />
       </div>
         </td>
       </tr>
@@ -152,10 +149,7 @@
             <div tal:content="structure widget">
               <input type="text" />
             </div>
-            <p class="formHelp"
-               tal:condition="widget/hint"
-               tal:content="widget/hint">Some Help Text
-            </p>
+            <metal:help-hint use-macro="widget/@@launchpad_form/help_hint" />
             </div>
           </td>
         </tr>
@@ -171,10 +165,7 @@
             tal:content="structure error"
             class="message"
           >Error message</div>
-          <p class="formHelp"
-             tal:condition="widget/hint"
-             tal:content="widget/hint">Some Help Text
-          </p>
+          <metal:help-hint use-macro="widget/@@launchpad_form/help_hint" />
         </div>
         </td>
       </tr>
@@ -193,4 +184,19 @@
     </table>
   </metal:macro>
 
+  <metal:help-hint metal:define-macro="help_hint">
+    <tal:widget-help
+        define="use_structure widget/query:has-structured-doc"
+        condition="widget/hint">
+     <p class="formHelp" tal:content="widget/hint"
+         tal:condition="not: use_structure">
+        Some Help Text
+      </p>
+      <p class="formHelp" tal:content="structure widget/hint"
+         tal:condition="use_structure">
+        Some Structured Help Text
+      </p>
+    </tal:widget-help>
+  </metal:help-hint>
+
 </div>

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2010-11-24 08:05:42 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2010-11-26 02:05:36 +0000
@@ -58,6 +58,7 @@
 from lp.app.browser.launchpadform import (
     action,
     custom_widget,
+    has_structured_doc,
     LaunchpadEditFormView,
     LaunchpadFormView,
     )
@@ -287,9 +288,16 @@
         description=(
             u'If built daily, these are the distribution versions that '
             u'the recipe will be built for.'))
-    recipe_text = Text(
-        title=u'Recipe text', required=True,
-        description=u'The text of the recipe.')
+    recipe_text = has_structured_doc(
+        Text(
+            title=u'Recipe text', required=True,
+            description=u"""The text of the recipe.
+                <a href="/+help/recipe-syntax.html" target="help"
+                  >Syntax help&nbsp;
+                  <span class="sprite maybe">
+                    <span class="invisible-link">Help</span>
+                  </span></a>
+               """))
 
 
 class RecipeTextValidatorMixin:

=== added file 'lib/lp/code/help/recipe-syntax.html'
--- lib/lp/code/help/recipe-syntax.html	1970-01-01 00:00:00 +0000
+++ lib/lp/code/help/recipe-syntax.html	2010-11-26 02:05:36 +0000
@@ -0,0 +1,91 @@
+<html>
+  <head>
+    <title>Source package recipe syntax</title>
+    <link rel="stylesheet" type="text/css"
+          href="/+icing/yui/cssreset/reset.css" />
+    <link rel="stylesheet" type="text/css"
+          href="/+icing/yui/cssfonts/fonts.css" />
+    <link rel="stylesheet" type="text/css"
+          href="/+icing/yui/cssbase/base.css" />
+  </head>
+  <body>
+    <h1>Source package recipe syntax</h1>
+
+    <p>A recipe is just text that starts with a line such as:</p>
+    <dl>
+      <dd>
+        <tt># bzr-builder format 0.2 deb-version {debupstream}-0~{revno}</tt>
+      </dd>
+    </dl>
+
+    <p>
+      The format specifier is there to allow the syntax to be changed in later
+      versions, and the meaning of "deb-version" will be explained below.
+    </p>
+
+    <p>
+      The next line specifies the base branch, this is the branch that will
+      be used to define the initial files and directory structure.
+    </p>
+
+    <p>
+      Next comes any number of lines of other branches to be merged in, but
+      using a slightly different format. To merge a branch in to the base
+      specify something like:
+    </p>
+
+    <dl>
+      <dd>
+        <tt>merge packaging lp:~foo-dev/foo/packaging</tt>
+      </dd>
+    </dl>
+
+    <p>
+      which specifies we are merging a branch we will refer to as "packaging",
+      which can be found at the given URI. The name you give to the branch as
+      the second item doesn't have to match anything else, it's just an
+      identifier specific to the recipe.
+    </p>
+
+    <p>
+      <a href="https://help.launchpad.net/Packaging/SourceBuilds/Recipes"
+         target="_blank">Read more &gt;</a> on recipe syntax for other commands and
+         specifying revisions for the branches.
+    </p>
+
+    <h2>deb-version</h2>
+
+    <p>
+      To build Debian source package that you desire you should make sure that
+      "deb-version" is set to an appropriate value on the first line of your
+      recipe. This will be used as the version number of the package. The
+      value you put there also allows for substitution of values in to it
+      based on various things when the recipe is processed:
+    </p>
+
+    <ul class="bulleted">
+      <li><tt>{date}</tt> &mdash; will be substituted with just the current
+        date, such as <tt>20090819</tt></li>
+      <li><tt>{debupstream}</tt> &mdash; will be replaced by the upstream
+        portion of the version number taken from debian/changelog in the final
+        tree. If when the tree is built the top of debian/changelog has a
+        version number of "<tt>1.0-1</tt>" then this would evaluate to
+        "<tt>1.0</tt>".
+      </li>
+      <li><tt>{revno}</tt> &mdash; will be the revno of the base branch (the
+        first specified)</li>
+      <li><tt>{revno:&lt;branch name&gt;}</tt> &mdash; will be substituted
+        with the revno for the branch named &lt;branch name&gt; in the
+        recipe</li>
+      <li><tt>{time}</tt> &mdash; will be substituted with the current date
+        and time, such as <tt>200908191512</tt></li>
+    </ul>
+
+    <p>
+      <a href="https://help.launchpad.net/Packaging/SourceBuilds/Recipes"
+         target="_blank">Read more &gt;</a> on recipe syntax for other commands and
+         specifying revisions for the branches.
+    </p>
+
+  </body>
+</html>

=== modified file 'lib/lp/code/templates/sourcepackagerecipe-new.pt'
--- lib/lp/code/templates/sourcepackagerecipe-new.pt	2010-11-03 01:20:17 +0000
+++ lib/lp/code/templates/sourcepackagerecipe-new.pt	2010-11-26 02:05:36 +0000
@@ -24,6 +24,7 @@
       </div>
 
       <div metal:use-macro="context/@@launchpad_form/form" />
+
     </div>
   </body>
 </html>