Merge lp:~salgado/launchpad/real-breadcrumbs into lp:launchpad

[Guilherme Salgado]

[Guilherme Salgado (salgado)]

= Summary =

When I redesigned our breadcrumb infrastructure I made the
BreadcbrumbBuilder abstraction unnecessary, but my branch was already
too big so I left it to be removed later.

== Proposed fix ==

Remove BreadcrumbBuilder by directly adapting objects into IBreadcrumbs,
instead of adapting them into IBreadcrumbBuilder and calling
make_breadcrumb() on it.

In order to actually complete this I had to change all existing adapters
so that they inherit from Breadcrumb rather than BreadcrumbBuilder, but
that brought the diff to 1.4KLOC, so I'll ask for a review of that
later.

== Pre-implementation notes ==

== Implementation details ==

== Tests ==

./bin/test -vvt hierarchical-menu.txt

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/webapp/interfaces.py
  lib/canonical/launchpad/browser/launchpad.py
  lib/canonical/launchpad/webapp/breadcrumb.py
  lib/canonical/launchpad/zcml/personproduct.zcml
  lib/canonical/launchpad/browser/personproduct.py
  lib/canonical/launchpad/doc/hierarchical-menu.txt

== Pyflakes Doctest notices ==

lib/canonical/launchpad/doc/hierarchical-menu.txt
    144: 'canonical_url' imported but unused

This has been removed already.

== Pylint notices ==

lib/canonical/launchpad/browser/launchpad.py
    103: [F0401] Unable to import 'lazr.uri' (No module named uri)

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 status approved

The only think I see that could be improved is in Heirarchy.items, noted
below.

Guilherme Salgado wrote:
> Guilherme Salgado has proposed merging lp:~salgado/launchpad/real-breadcrumbs into lp:launchpad/devel.
>
> Requested reviews:
> Canonical Launchpad Engineering (launchpad)
>
> = Summary =
>
> When I redesigned our breadcrumb infrastructure I made the
> BreadcbrumbBuilder abstraction unnecessary, but my branch was already
> too big so I left it to be removed later.
>
> == Proposed fix ==
>
> Remove BreadcrumbBuilder by directly adapting objects into IBreadcrumbs,
> instead of adapting them into IBreadcrumbBuilder and calling
> make_breadcrumb() on it.
>
> In order to actually complete this I had to change all existing adapters
> so that they inherit from Breadcrumb rather than BreadcrumbBuilder, but
> that brought the diff to 1.4KLOC, so I'll ask for a review of that
> later.
>
> == Pre-implementation notes ==
>
> == Implementation details ==
>
> == Tests ==
>
> ./bin/test -vvt hierarchical-menu.txt
>
> == Demo and Q/A ==
>
>
> = Launchpad lint =
>
> Checking for conflicts. and issues in doctests and templates.
> Running jslint, xmllint, pyflakes, and pylint.
> Using normal rules.
>
> Linting changed files:
> lib/canonical/launchpad/webapp/interfaces.py
> lib/canonical/launchpad/browser/launchpad.py
> lib/canonical/launchpad/webapp/breadcrumb.py
> lib/canonical/launchpad/zcml/personproduct.zcml
> lib/canonical/launchpad/browser/personproduct.py
> lib/canonical/launchpad/doc/hierarchical-menu.txt
>
>
> == Pyflakes Doctest notices ==
>
> lib/canonical/launchpad/doc/hierarchical-menu.txt
> 144: 'canonical_url' imported but unused
>
> This has been removed already.
>
>
> == Pylint notices ==
>
> lib/canonical/launchpad/browser/launchpad.py
> 103: [F0401] Unable to import 'lazr.uri' (No module named uri)
>
>
> === modified file 'lib/canonical/launchpad/browser/launchpad.py'
> --- lib/canonical/launchpad/browser/launchpad.py 2009-08-20 05:06:34 +0000
> +++ lib/canonical/launchpad/browser/launchpad.py 2009-08-24 17:47:34 +0000
> @@ -96,7 +96,7 @@
> StandardLaunchpadFacets, canonical_name, canonical_url, custom_widget,
> stepto)
> from canonical.launchpad.webapp.interfaces import (
> - IBreadcrumbBuilder, ILaunchBag, ILaunchpadRoot, INavigationMenu,
> + IBreadcrumb, ILaunchBag, ILaunchpadRoot, INavigationMenu,
> NotFoundError, POSTToNonCanonicalURL)
> from canonical.launchpad.webapp.publisher import RedirectionView
> from canonical.launchpad.webapp.authorization import check_permission
> @@ -226,39 +226,31 @@
> The list starts with the breadcrumb closest to the hierarchy root.
> """
> breadcrumbs = []
> - for builder in self._getBreadcrumbBuilders():
> - crumb = builder.make_breadcrumb()
> - if crumb is not None:
> - breadcrumbs.append(crumb)
> - return breadcrumbs
> -
> - def _getBreadcrumbBuilders(self):
> - builders = []
> for obj in self.objects:
> - builder = queryAdapter(obj, IBreadcrumbBuilder)
> -...

review: Approve

[Guilherme Salgado (salgado)]

Hi Aaron,

I did the change you suggested, but I need to cast the result of enumerate(breadcrumbs) into a list so that I can feed it to reversed().

Also, https://pastebin.canonical.com/21444/ has the remaining changes. It is huge but has just mechanical changes I did using sed. To test, './bin/test -vvt test_breadcrumbs'.

=== modified file 'lib/canonical/launchpad/browser/launchpad.py'
--- lib/canonical/launchpad/browser/launchpad.py	2009-08-20 05:06:34 +0000
+++ lib/canonical/launchpad/browser/launchpad.py	2009-08-24 17:47:34 +0000
@@ -96,7 +96,7 @@
     StandardLaunchpadFacets, canonical_name, canonical_url, custom_widget,
     stepto)
 from canonical.launchpad.webapp.interfaces import (
-    IBreadcrumbBuilder, ILaunchBag, ILaunchpadRoot, INavigationMenu,
+    IBreadcrumb, ILaunchBag, ILaunchpadRoot, INavigationMenu,
     NotFoundError, POSTToNonCanonicalURL)
 from canonical.launchpad.webapp.publisher import RedirectionView
 from canonical.launchpad.webapp.authorization import check_permission
@@ -226,39 +226,31 @@
         The list starts with the breadcrumb closest to the hierarchy root.
         """
         breadcrumbs = []
-        for builder in self._getBreadcrumbBuilders():
-            crumb = builder.make_breadcrumb()
-            if crumb is not None:
-                breadcrumbs.append(crumb)
-        return breadcrumbs
-
-    def _getBreadcrumbBuilders(self):
-        builders = []
         for obj in self.objects:
-            builder = queryAdapter(obj, IBreadcrumbBuilder)
-            if builder is not None:
-                builders.append(builder)
+            breadcrumb = queryAdapter(obj, IBreadcrumb)
+            if breadcrumb is not None:
+                breadcrumbs.append(breadcrumb)
 
         host = URI(self.request.getURL()).host
-        if (len(builders) == 0
+        if (len(breadcrumbs) == 0
             or host == allvhosts.configs['mainsite'].hostname):
-            return builders
+            return breadcrumbs
 
         # If we got this far it means we have breadcrumbs and we're not on the
         # mainsite, so we'll sneak an extra breadcrumb for the vhost we're on.
         vhost = host.split('.')[0]
 
-        # Iterate over the context of our builders in reverse order and for
+        # Iterate over the context of our breadcrumbs in reverse order and for
         # the first one we find an adapter named after the vhost we're on,
         # generate an extra breadcrumb and insert it in our list.
-        for idx in reversed(xrange(len(builders))):
-            builder = builders[idx]
-            extra_builder = queryAdapter(
-                builder.context, IBreadcrumbBuilder, name=vhost)
-            if extra_builder is not None:
-                builders.insert(idx + 1, extra_builder)
+        for idx in reversed(xrange(len(breadcrumbs))):
+            breadcrumb = breadcrumbs[idx]
+            extra_breadcrumb = queryAdapter(
+                breadcrumb.context, IBreadcrumb, name=vhost)
+            if extra_breadcrumb is not None:
+                breadcrumbs.insert(idx + 1, extra_breadcrumb)
                 break
-        return builders
+        return breadcrumbs
 
     def render(self):
         """Render the hierarchy HTML.

=== modified file 'lib/canonical/launchpad/browser/personproduct.py'
--- lib/canonical/launchpad/browser/personproduct.py	2009-06-25 05:30:52 +0000
+++ lib/canonical/launchpad/browser/personproduct.py	2009-08-24 17:47:34 +0000
@@ -5,7 +5,7 @@
 
 __metaclass__ = type
 __all__ = [
-    'PersonProductBreadcrumbBuilder',
+    'PersonProductBreadcrumb',
     'PersonProductFacets',
     'PersonProductNavigation',
     ]
@@ -17,7 +17,7 @@
 from lp.code.interfaces.branchnamespace import (
     get_branch_namespace)
 from canonical.launchpad.interfaces.personproduct import IPersonProduct
-from canonical.launchpad.webapp.breadcrumb import BreadcrumbBuilder
+from canonical.launchpad.webapp.breadcrumb import Breadcrumb
 from canonical.launchpad.webapp import (
     Link, Navigation, StandardLaunchpadFacets)
 from canonical.launchpad.webapp.interfaces import NotFoundError
@@ -38,8 +38,8 @@
             return branch
 
 
-class PersonProductBreadcrumbBuilder(BreadcrumbBuilder):
-    """Builds a breadcrumb for an `IPersonProduct`."""
+class PersonProductBreadcrumb(Breadcrumb):
+    """Breadcrumb for an `IPersonProduct`."""
 
     @property
     def text(self):

=== modified file 'lib/canonical/launchpad/doc/hierarchical-menu.txt'
--- lib/canonical/launchpad/doc/hierarchical-menu.txt	2009-08-17 19:22:06 +0000
+++ lib/canonical/launchpad/doc/hierarchical-menu.txt	2009-08-24 17:47:34 +0000
@@ -60,7 +60,7 @@
 
 The Hierarchy class builds the breadcrumbs by looking at each object in
 the request.traversed_objects attribute.  If a traversed object can be
-adapted to IBreadcrumbBuilder, then it is added to the breadcrumbs list.
+adapted to IBreadcrumb, then it is added to the breadcrumbs list.
 
 We'll add the objects to the request's list of traversed objects so
 the hierarchy will discover them.
@@ -71,38 +71,35 @@
     ...     [root, cookbook, recipe])
 
 The hierarchy's list of breadcrumbs is empty since none of the objects
-have an IBreadcrumbBuilder adapter.
+have an IBreadcrumb adapter.
 
     >>> hierarchy = getMultiAdapter((recipe, request), name='+hierarchy')
     >>> hierarchy.items()
     []
 
-The ICookbook and IRecipe breadcrumb objects show up in the hierarchy
-after IBreadcrumbBuilder adapters are registered for them.  The
-hierarchy builds a list of breadcrumbs starting with the breadcrumb
-closest to the hierarchy root.
+The ICookbook and IRecipe breadcrumb objects show up in the hierarchy after
+IBreadcrumb adapters are registered for them.  The hierarchy builds a list of
+breadcrumbs starting with the breadcrumb closest to the hierarchy root.
 
-    >>> from canonical.launchpad.webapp.breadcrumb import BreadcrumbBuilder
+    >>> from canonical.launchpad.webapp.breadcrumb import Breadcrumb
 
     # Note that the Hierarchy assigns the breadcrumb's URL, but we need to
     # give it a valid .text attribute.
-    >>> class TextualBreadcrumbBuilder(BreadcrumbBuilder):
+    >>> class TextualBreadcrumb(Breadcrumb):
     ...     @property
     ...     def text(self):
     ...         return self.context.name.capitalize().replace('-', ' ')
 
-    >>> from canonical.launchpad.webapp.interfaces import IBreadcrumbBuilder
+    >>> from canonical.launchpad.webapp.interfaces import IBreadcrumb
 
-    >>> provideAdapter(
-    ...     TextualBreadcrumbBuilder, [ICookbook], IBreadcrumbBuilder)
-    >>> provideAdapter(
-    ...     TextualBreadcrumbBuilder, [IRecipe], IBreadcrumbBuilder)
+    >>> provideAdapter(TextualBreadcrumb, [ICookbook], IBreadcrumb)
+    >>> provideAdapter(TextualBreadcrumb, [IRecipe], IBreadcrumb)
 
     >>> hierarchy.items()
-    [<Breadcrumb
+    [<TextualBreadcrumb
         url='http://launchpad.dev/joy-of-cooking'
         text='Joy of cooking'>,
-     <Breadcrumb
+     <TextualBreadcrumb
         url='http://launchpad.dev/joy-of-cooking/spam'
         text='Spam'>]
 
@@ -114,59 +111,45 @@
     ...     'http://launchpad.dev/+cooker/jamie',
     ...     [root, cooker])
 
-    >>> provideAdapter(
-    ...     TextualBreadcrumbBuilder, [ICooker], IBreadcrumbBuilder)
+    >>> provideAdapter(TextualBreadcrumb, [ICooker], IBreadcrumb)
 
     >>> cooker_hierarchy = getMultiAdapter(
     ...     (cooker, cooker_request), name='+hierarchy')
     >>> cooker_hierarchy.items()
-    [<Breadcrumb url='http://launchpad.dev/+cooker/jamie' text='Jamie'>]
+    [<TextualBreadcrumb url='http://launchpad.dev/+cooker/jamie' text='Jamie'>]
 
 
 == Building IBreadcrumb objects ==
 
-The construction of breadcrumb objects is handled by an
-IBreadcrumbBuilder factory.  The factory adapts a context object and
-produces an IBreadcrumb object for that context.
-
-The builder holds a temporary copy of a breadcrumb until the breadcrumb is in
-a usable state.  We can ask the builder for a finished breadcrumb after both
-the text and the URL have been specified.
+The construction of breadcrumb objects is handled by an IBreadcrumb adapter,
+which adapts a context object and produces an IBreadcrumb object for that
+context.  The default adapter provides the url attribute, but the breadcrumb's
+text must be overriden in subclasses.
 
     >>> from canonical.launchpad.webapp.interfaces import IBreadcrumb
     >>> from zope.interface.verify import verifyObject
-
-    >>> builder = BreadcrumbBuilder(cookbook)
-    >>> verifyObject(IBreadcrumbBuilder, builder)
-    True
-
-    >>> builder.text = 'Joy of cooking'
-
-    >>> breadcrumb = builder.make_breadcrumb()
+    >>> breadcrumb = Breadcrumb(cookbook)
     >>> verifyObject(IBreadcrumb, breadcrumb)
     True
+    >>> print breadcrumb.text
+    None
     >>> breadcrumb
     <Breadcrumb
         url='http://launchpad.dev/joy-of-cooking'
-        text='Joy of cooking'>
+        text='None'>
 
-The breadcrumb's attributes can be overridden with subclassing and
-Python properties.
+As said above, the breadcrumb's attributes can be overridden with subclassing
+and Python properties.
 
     >>> from canonical.launchpad.webapp.publisher import canonical_url
-
-    >>> class DynamicBreadcrumbBuilder(BreadcrumbBuilder):
+    >>> class DynamicBreadcrumb(Breadcrumb):
     ...     @property
     ...     def text(self):
     ...         return self.context.name.capitalize().replace('-', ' ')
-    ...
-    ...     @property
-    ...     def url(self):
-    ...         return canonical_url(self.context)
 
-    >>> builder = DynamicBreadcrumbBuilder(cookbook)
-    >>> builder.make_breadcrumb()
-    <Breadcrumb
+    >>> breadcrumb = DynamicBreadcrumb(cookbook)
+    >>> breadcrumb
+    <DynamicBreadcrumb
         url='http://launchpad.dev/joy-of-cooking'
         text='Joy of cooking'>
 
@@ -186,9 +169,9 @@
     >>> provideAdapter(
     ...     RecipeImageDisplayAPI, [IRecipe], IPathAdapter, 'image')
 
-    >>> builder = DynamicBreadcrumbBuilder(recipe)
-    >>> builder.make_breadcrumb()
-    <Breadcrumb
+    >>> breadcrumb = DynamicBreadcrumb(recipe)
+    >>> breadcrumb
+    <DynamicBreadcrumb
         url='http://launchpad.dev/joy-of-cooking/spam'
         text='Spam'
         icon='<img src="/@@/recipe"/>'>
@@ -204,9 +187,9 @@
     >>> print queryAdapter(cookbook, IPathAdapter, name='image').icon()
     None
 
-    >>> builder = DynamicBreadcrumbBuilder(cookbook)
-    >>> builder.make_breadcrumb()
-    <Breadcrumb
+    >>> breadcrumb = DynamicBreadcrumb(cookbook)
+    >>> breadcrumb
+    <DynamicBreadcrumb
         url='http://launchpad.dev/joy-of-cooking'
         text='Joy of cooking'>
 
@@ -221,7 +204,6 @@
 consistency across the site.
 
     >>> from canonical.launchpad.browser.launchpad import Hierarchy
-
     >>> class CustomHierarchy(Hierarchy):
     ...     @property
     ...     def objects(self):
@@ -229,7 +211,7 @@
 
     >>> spammy_hierarchy = CustomHierarchy(root, request)
     >>> spammy_hierarchy.items()
-    [<Breadcrumb
+    [<TextualBreadcrumb
         url='http://launchpad.dev/joy-of-cooking/spam'
         text='Spam'
         icon='<img src="/@@/recipe"/>'>]
@@ -276,7 +258,7 @@
     >>> breadcrumb_no_icon, breadcrumb_with_icon = hierarchy.items()
 
     >>> breadcrumb_no_icon
-    <Breadcrumb
+    <TextualBreadcrumb
         url='http://launchpad.dev/joy-of-cooking'
         text='Joy of cooking'>
 
@@ -284,7 +266,7 @@
     False
 
     >>> breadcrumb_with_icon
-    <Breadcrumb
+    <TextualBreadcrumb
         url='http://launchpad.dev/joy-of-cooking/spam'
         text='Spam'
         icon='<img src="/@@/recipe"/>'>

=== modified file 'lib/canonical/launchpad/webapp/breadcrumb.py'
--- lib/canonical/launchpad/webapp/breadcrumb.py	2009-08-17 20:20:17 +0000
+++ lib/canonical/launchpad/webapp/breadcrumb.py	2009-08-24 17:47:34 +0000
@@ -7,7 +7,6 @@
 
 __all__ = [
     'Breadcrumb',
-    'BreadcrumbBuilder',
     ]
 
 
@@ -16,38 +15,15 @@
 from zope.interface import implements
 
 from canonical.launchpad.webapp import canonical_url
-from canonical.launchpad.webapp.interfaces import (
-    IBreadcrumb, IBreadcrumbBuilder)
+from canonical.launchpad.webapp.interfaces import IBreadcrumb
 
 
 class Breadcrumb:
-    """See `IBreadcrumb`."""
-    implements(IBreadcrumb)
-
-    def __init__(self, url, text, icon=None):
-        self.url = url
-        self.text = text
-        self.icon = icon
-
-    def __repr__(self):
-        if self.icon is not None:
-            icon_repr = " icon='%s'" % self.icon
-        else:
-            icon_repr = ""
-
-        return "<%s url='%s' text='%s'%s>" % (
-            self.__class__.__name__, self.url, self.text, icon_repr)
-
-
-# XXX: salgado, 2009-08-17: Since this adapter now provides a default
-# value for the 'url' attribute, we could easily convert it into an
-# adapter for IBreadcrumb, just changing the Hierarchy view.
-class BreadcrumbBuilder:
-    """See `IBreadcrumbBuilder`.
+    """See `IBreadcrumb`.
 
     This class is intended for use as an adapter.
     """
-    implements(IBreadcrumbBuilder)
+    implements(IBreadcrumb)
 
     rootsite = 'mainsite'
     text = None
@@ -66,14 +42,11 @@
         return queryAdapter(
             self.context, IPathAdapter, name='image').icon()
 
-    def make_breadcrumb(self):
-        """See `IBreadcrumbBuilder.`"""
-        if self.text is None:
-            raise AssertionError(
-                "The builder has not been given valid text for the "
-                "breadcrumb.")
-        if self.url is None:
-            raise AssertionError(
-               "The builder has not been given a valid breadcrumb URL.")
+    def __repr__(self):
+        if self.icon is not None:
+            icon_repr = " icon='%s'" % self.icon
+        else:
+            icon_repr = ""
 
-        return Breadcrumb(self.url, self.text, icon=self.icon)
+        return "<%s url='%s' text='%s'%s>" % (
+            self.__class__.__name__, self.url, self.text, icon_repr)

=== modified file 'lib/canonical/launchpad/webapp/interfaces.py'
--- lib/canonical/launchpad/webapp/interfaces.py	2009-08-18 14:36:16 +0000
+++ lib/canonical/launchpad/webapp/interfaces.py	2009-08-24 17:47:34 +0000
@@ -259,17 +259,6 @@
     icon = Attribute("An <img> tag showing this breadcrumb's 14x14 icon.")
 
 
-class IBreadcrumbBuilder(IBreadcrumb):
-    """An object that builds `IBreadcrumb` objects."""
-    # We subclass IBreadcrumb to minimize interface drift.
-
-    def make_breadcrumb():
-        """Return an object implementing the `IBreadcrumb` interface.
-
-        If for any reason no IBreadcrumb object can be created, return None.
-        """
-
-
 #
 # Canonical URLs
 #

=== modified file 'lib/canonical/launchpad/zcml/personproduct.zcml'
--- lib/canonical/launchpad/zcml/personproduct.zcml	2009-07-17 00:26:05 +0000
+++ lib/canonical/launchpad/zcml/personproduct.zcml	2009-08-24 17:47:34 +0000
@@ -28,9 +28,9 @@
       />
 
   <adapter
-      provides="canonical.launchpad.webapp.interfaces.IBreadcrumbBuilder"
+      provides="canonical.launchpad.webapp.interfaces.IBreadcrumb"
       for="canonical.launchpad.interfaces.personproduct.IPersonProduct"
-      factory="canonical.launchpad.browser.personproduct.PersonProductBreadcrumbBuilder"
+      factory="canonical.launchpad.browser.personproduct.PersonProductBreadcrumb"
       permission="zope.Public"
       />