[Merge] lp:~software-updates-spec/update-manager/group-by-applications into lp:update-manager

[Michael Vogt]

On Tue, Jan 22, 2013 at 06:25:25PM -0000, Michael Terry wrote:
> Michael Terry has proposed merging lp:~software-updates-spec/update-manager/group-by-applications into lp:update-manager.
> 
> Requested reviews:
>   Ubuntu Core Development Team (ubuntu-core-dev)
> 
> For more details, see:
> https://code.launchpad.net/~software-updates-spec/update-manager/group-by-applications/+merge/144362
> 
> This is a continuation of https://code.launchpad.net/~dylanmccall/update-manager/group-by-applications/+merge/112678
> 
> This implements the 'Available Updates' description pane in the Software Updates spec:
> https://wiki.ubuntu.com/SoftwareUpdates#Expanded_presentation_of_updates

Thanks for this branch! I haven't really had a chance to test-run it
yet, my bandwidth right now is not very good. From looking at the diff
it looks good, I like the new tests and the new
"aptroot-group-testing"!

Some things I noticed during reading the diff, nothing that really
needs fixing, just tiny nit-picking. 

[..]
> +    def __init__(self, parent, dist=None):
> +        self.dist = dist
> +        if self.dist is None:
> +            try:
> +                self.dist = subprocess.check_output(
> +                    ["lsb_release", "-c", "-s"],
> +                    universal_newlines=True).strip()
> +            except subprocess.CalledProcessError as e:
> +                print("Error in lsb_release: %s" % e)
[..]

As a tiny nit-pick, this above could be written also via:

import platform
self.dist = platform.dist()[2]

[..]
> +import xml.sax.saxutils
..
> +def get_package_label(pkg):
> +    """ this takes a package synopsis and uppercases the first word's
> +        first letter
> +    """
> +    import xml.sax.saxutils
> +    name = xml.sax.saxutils.escape(getattr(pkg.candidate, "summary",
> ""))
> +    return capitalize_first_word(name)

Looks like one of the imports can be skipped. Or (probably better) we use 
GLib.markup_escape_text() instead.

> +        if len(cells) != 3 or \
> +           not isinstance(cells[0], Gtk.CellRendererToggle) or \
> +           not isinstance(cells[1], Gtk.CellRendererPixbuf) or \
> +           not isinstance(cells[2], Gtk.CellRendererText):
> +            return

Most of the other python code seems to put "(" around ")" the multi
line if statements, either is fine with me, just wanted to mention
it.


Cheers,
 Michael

-- 
https://code.launchpad.net/~software-updates-spec/update-manager/group-by-applications/+merge/144362
Your team Ubuntu Core Development Team is requested to review the proposed merge of lp:~software-updates-spec/update-manager/group-by-applications into lp:update-manager.