Hi Brad,

This is a nice improvement.

merge-conditional

-Edwin

>=== modified file 'lib/lp/registry/browser/product.py'
>--- lib/lp/registry/browser/product.py	2009-11-23 14:52:25 +0000
>+++ lib/lp/registry/browser/product.py	2009-11-25 18:30:38 +0000
>@@ -474,9 +474,9 @@
>
> def _sort_distros(a, b):


Since this function doesn't actually sort a & b, it should be
called _compare_distros.


>     """Put Ubuntu first, otherwise in alpha order."""
>-    if a['name'] == 'ubuntu':
>+    if a == 'ubuntu':
>         return -1

This only works if 'ubuntu' is always passed in as the first
parameter. There needs to be a corresponding if-statement for
b=='ubuntu'.


>-    return cmp(a['name'], b['name'])
>+    return cmp(a, b)
>
>
> class ProductSetBreadcrumb(Breadcrumb):
>@@ -947,29 +947,33 @@
>         title, and an attribute "packagings" which is a list of the relevant
>         packagings for this distro and product.
>         """
>-        distros = {}
>-        # first get a list of all relevant packagings
>+        # First get a list of all relevant packagings.
>         all_packagings = []
>         for series in self.context.series:
>             for packaging in series.packagings:
>                 all_packagings.append(packaging)
>-        # we sort it so that the packagings will always be displayed in the
>-        # distroseries version, then productseries name order
>+        # We sort it so that the packagings will always be displayed in the
>+        # distroseries version, then productseries name order.
>         all_packagings.sort(key=lambda a: (a.distroseries.version,
>             a.productseries.name, a.id))
>+
>+        distros = {}
>         for packaging in all_packagings:
>-            if distros.has_key(packaging.distroseries.distribution.name):
>-                distro = distros[packaging.distroseries.distribution.name]
>+            distribution = packaging.distroseries.distribution
>+            if distribution.name in distros:
>+                distro = distros[distribution.name]
>             else:
>-                distro = {}
>-                distro['distribution'] = packaging.distroseries.distribution
>-                distro['packagings'] = []
>+                # Create a dictionary for the distribution.
>+                distro = dict(
>+                    distribution=packaging.distroseries.distribution,


Since you created the new distribution variable, this could be shortened to:
  distribution=distribution


>+                    packagings=[])
>                 distros[packaging.distroseries.distribution.name] = distro


The index could be shortened to just distribution.name.
Alternatively, the entire if-else-statement could be replaced with:
    distro = distros.setdefault(
        distribution.name, dict(distribution=distribution, packaging=[]))
but that is up to you, since you might find it less readable.


>             distro['packagings'].append(packaging)
>-        # now we sort the resulting set of "distro" objects, and return that
>-        result = distros.values()
>-        result.sort(cmp=_sort_distros)
>-        return result
>+        # Now we sort the resulting list of "distro" objects, and return that.
>+        distro_names = distros.keys()
>+        distro_names.sort(cmp=_sort_distros)
>+        results = [distros[name] for name in distro_names]
>+        return results
> 
> 
> class ProductDownloadFilesView(LaunchpadView,
>