Hi Dylan,

Thanks for your contribution to Update Manager, and to Ubuntu!

I've provided some detailed comments on the code below.

One thing I'd really like to see is some unittests, especially for the new
code in UpdateList.py.  Would you like to take a crack at adding a few for
some of the new methods and classes you're adding?  Have you run the existing
unittests to see if your changes have broken any of them?  While Update
Manager doesn't have a fantastic test suite, I'm not crazy about making things
worse by adding more untested code.

Someone should also review the UI changes before this branch lands, probably
mpt or someone else on the design team.  I'll request a review from mpt to
start with.

I did notice one UI problem when I ran your branch on Quantal.  I see an
update for Ubuntu base (which I gather is a catch-all for anything that
doesn't fit into any other category).  However, when I expand the arrow for
that group, I'm still left with a very short window, one row that contains the
"Ubuntu base" string, and a very long and mostly unusable scroll bar.  Maybe
the window should expand vertically to fit the sub-package contents better, or
maybe the details window should be taller to start with.

Other than that, I think the groupings are a nice refactoring of the
information presented by Update Manager!

I'll mark the branch Needs Fixing for now, but I will be happy to re-review it
if you make the suggested changes.

=== modified file 'UpdateManager/Core/UpdateList.py'
--- UpdateManager/Core/UpdateList.py	2012-06-28 00:10:23 +0000
+++ UpdateManager/Core/UpdateList.py	2012-07-17 18:03:22 +0000
> @@ -1,25 +1,29 @@
> -# UpdateList.py
> +# UpdateList.py 
>  # -*- Mode: Python; indent-tabs-mode: nil; tab-width: 4; coding: utf-8 -*-
> -#
> +#  

You need to be really careful about adding extra whitespace, as your patch
does here.  There are several reason for this, but probably the most important
one is that such whitespace-only changes are just noise that distract from the
substantive changes, and reviews thereof.  There are many options for ensuring
"whitespace normalization" in Python code, some of which can be accomplished
by your editor.   E.g. I have nice functions for Emacs:

http://mail.python.org/pipermail/python-dev/2007-April/072773.html

>  #  Copyright (c) 2004-2008 Canonical
> -#
> +#  
>  #  Author: Michael Vogt <email address hidden>
> -#
> -#  This program is free software; you can redistribute it and/or
> -#  modify it under the terms of the GNU General Public License as
> +# 
> +#  This program is free software; you can redistribute it and/or 
> +#  modify it under the terms of the GNU General Public License as 
>  #  published by the Free Software Foundation; either version 2 of the
>  #  License, or (at your option) any later version.
> -#
> +# 
>  #  This program is distributed in the hope that it will be useful,
>  #  but WITHOUT ANY WARRANTY; without even the implied warranty of
>  #  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>  #  GNU General Public License for more details.
> -#
> +# 
>  #  You should have received a copy of the GNU General Public License
>  #  along with this program; if not, write to the Free Software
>  #  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
>  #  USA
>  
> +# TODO: Add mainPackage property to UpdateGroup. (Package owning application .desktop file, or None)
> +# TODO: Extend UpdateGroup with immediate dependencies of mainPackage and friends.
> +# TODO: Sort packages in UpdateGroup

You also need to be careful about lines that are too long.  As a general rule,
all lines should be shorter than 79 characters wide.

http://www.python.org/dev/peps/pep-0008/#maximum-line-length

I highly suggest reading PEP 8, which defines some of the fundamental coding
guidelines we try to adhere to.

> +
>  from __future__ import print_function
>  
>  import warnings
> @@ -27,10 +31,79 @@
>                          DeprecationWarning)
>  
>  from gettext import gettext as _
> -import operator
> -import subprocess
> +import os
> +import glob
>  import sys
> -
> +import itertools

Thanks for deleting the unused imports of operator and subprocess.  However,
it looks like itertools is also not used, so that should be removed too.

BTW, a great tool for finding simple coding problems like this is pyflakes,
which of course is available in Ubuntu, and is often fairly easy to integrate
with your editor.  For example, when I'm editing a file in Emacs, any line
that pyflakes complains about is highlighted with a red background, and you
can see the totally number of problems in the mode line.

> +
> +from gi.repository import Gio
> +
> +APP_INSTALL_PATH = "/usr/share/app-install/desktop"

Do you think it makes sense to either parametrize this, or provide for some
other way to override this location?  Not doing so makes the code harder to
test.

> +
> +class UpdateGroup(object):

Update Manager is now a Python 3 application, so subclassing from object is no
longer necessary.  Python 3 only has "new style" classes.

> +    @property
> +    def pkgs(self):
> +        all_pkgs = []
> +        all_pkgs.extend(self._core_pkgs)
> +        all_pkgs.extend(self._extra_pkgs)
> +        all_pkgs.sort(key=lambda pkg: pkg.name)
> +        return all_pkgs

Use of lambdas is generally frowned upon if you don't need it.  In this case,
operator.attrgetter('name') is probably better.  You also don't need to sort
in place, since the sorted() built-in will do the trick.

Finally, PEP 8 discourages the use of abbreviations, since they are less
readable.  I know that most of the code is using abbreviations, and you
definitely should not change that file-wide, at least not in this branch, so
you could argue that internal consistency is more important than PEP 8
compliance.  I'd offer that a good compromise is that in any new code, or
where the code is private (i.e. not exposed in the API), you should avoid
abbreviations.

I'd probably rewrite this function like so:

    @property
    def packages(self):
        all_packages = list(self._core_pkgs)
        all_packages.extend(self._extra_pkgs)
        return sorted(all_packages, key=operator.attrgetter('name'))

Finally, it's generally good practice to have the __init__() and any other
special methods before "normal" methods, so I'd move this def down below the
constructor.

> +
> +    def __init__(self, core_pkgs, name, icon):
> +        self._core_pkgs = set(core_pkgs)

Again, probably s/pkgs/packages/ all around.

> +        self._extra_pkgs = set()
> +        self.name = name
> +        self.icon = icon
> +
> +    def add_extra(self, extra_pkg):
> +        if self._contains(extra_pkg):

You can probably inline this test with the following:

            if extra_pkg in self._core_pkgs.union(self._extra_pkgs)

I also think it would be worthwhile adding a docstring to this method, and
probably most methods, or at least all public methods.  For example, what are
the semantics of the return value?  I can probably deduce it, but I might be
wrong.  Docstrings go a long way to helping the next person reading this code
to understand the original intent.  Good unittests are another way.

> +            return True
> +        elif self._is_dependency(extra_pkg):
> +            self._extra_pkgs.add(extra_pkg)
> +            return True
> +        else:
> +            return False
> +
> +    def _contains(self, pkg):
> +        return pkg in self._core_pkgs or pkg in self._extra_pkgs
> +
> +    def _is_dependency(self, pkg):
> +        dependency_types = 'Depends', 'Recommends'

You only use dependency_types in one place, so I think a slightly better style
would be to either inline it at the call site (in which case you wouldn't need
the *-args), or to put this in a module global, probably called
DEPENDENCY_TYPES.

> +        for core_pkg in self._core_pkgs:
> +            for dep_list in core_pkg.candidate.get_dependencies(*dependency_types):

Line too long.

> +                for dep in dep_list:
> +                    if dep.name == pkg.name:
> +                        return True
> +        return False
> +
> +    def packages_are_selected(self):
> +        for pkg in self.pkgs:
> +            if pkg.marked_install or pkg.marked_upgrade:
> +                return True
> +        return False
> +
> +    def selection_is_inconsistent(self):
> +        pkgs_installing = [pkg for pkg in self.pkgs
> +                           if pkg.marked_install or pkg.marked_upgrade]
> +        return len(pkgs_installing) > 0 and len(pkgs_installing) < len(self.pkgs)

I wonder if some kind of set comparison would be better here>

> +
> +    def get_total_size(self):
> +        size = 0
> +        for pkg in self.pkgs:
> +            size += getattr(pkg.candidate, "size", 0)
> +        return size
> +
> +class UpdateApplicationGroup(UpdateGroup):

PEP 8 specifies two blank lines between top-level constructs.

> +    def __init__(self, pkgs, application):
> +        name = application.get_display_name()
> +        icon = application.get_icon()
> +        super(UpdateApplicationGroup, self).__init__(pkgs, name, icon)
> +
> +class UpdateSystemGroup(UpdateGroup):
> +    def __init__(self, system_pkgs):
> +        name = _("Ubuntu base")
> +        icon = Gio.ThemedIcon.new("distributor-logo")
> +        super(UpdateSystemGroup, self).__init__(system_pkgs, name, icon)
>  
>  class UpdateOrigin(object):
>      def __init__(self, desc, importance):
> @@ -38,44 +111,84 @@
>          self.importance = importance
>          self.description = desc
>  
> -
>  class UpdateList(object):
>      """
> -    class that contains the list of available updates in
> +    class that contains the list of available updates in 
>      self.pkgs[origin] where origin is the user readable string
>      """
>  
>      def __init__(self, parent):
>          # a map of packages under their origin
>          try:
> -            dist = subprocess.check_output(
> -                ["lsb_release", "-c", "-s"], universal_newlines=True).strip()
> -        except subprocess.CalledProcessError as e:
> +            pipe = os.popen("lsb_release -c -s")
> +            dist = pipe.read().strip()
> +            del pipe
> +        except Exception as e:

Why did you make this change?  Please reconsider, since the original construct
is more Python 3 friendly.  Specifically, the passing of
universal_newlines=True ensures that data is utf-8 encoded.

http://docs.python.org/py3k/library/subprocess.html#frequently-used-arguments

I'll also note that pyflakes complains about the `dist` line since dist isn't
used.

>              print("Error in lsb_release: %s" % e)
>              parent.error(_("Failed to detect distribution"),
>                           _("A error '%s' occurred while checking what system "
> -                           "you are using.") % e)
> +                         "you are using.") % e)

This looks like another improper whitespace change.  The double quotes on the
left hand side should line up.

>              sys.exit(1)
>          self.distUpgradeWouldDelete = 0
> -        self.pkgs = {}
> +        self.pkg_groups = []
>          self.num_updates = 0
> -        self.matcher = self.initMatcher(dist)
> -
> -    def initMatcher(self, dist):
> -        # (origin, archive, description, importance)
> -        matcher_templates = [
> -            ("%s-security" % dist, "Ubuntu", _("Important security updates"),
> -             10),
> -            ("%s-updates" % dist, "Ubuntu", _("Recommended updates"), 9),
> -            ("%s-proposed" % dist, "Ubuntu", _("Proposed updates"), 8),
> -            ("%s-backports" % dist, "Ubuntu", _("Backports"), 7),
> -            (dist, "Ubuntu", _("Distribution updates"), 6)
> -        ]
> -        matcher = {}
> -        for (origin, archive, desc, importance) in matcher_templates:
> -            matcher[(origin, archive)] = UpdateOrigin(desc, importance)
> -        matcher[(None, None)] = UpdateOrigin(_("Other updates"), -1)
> -        return matcher
> +        data_dirs = os.environ['XDG_DATA_DIRS'].split(':')
> +        self.application_dirs = [os.path.join(base, 'applications')
> +                                 for base in data_dirs]
> +        self.current_desktop = os.environ.get('XDG_CURRENT_DESKTOP')
> +
> +    def _file_is_application(self, file_path):
> +        file_path = os.path.abspath(file_path)
> +        is_application = False
> +        for app_dir in self.application_dirs:
> +            is_application = is_application or file_path.startswith(app_dir)
> +        extension = os.path.splitext(file_path)[1]
> +        is_application = is_application and (extension == '.desktop')
> +        return is_application

The logic here is a little bit odd, since if is_application is False, you
don't care about the extension.  I think you also have a subtle bug here.
Let's say file_path is '/foo/bar/app.desktop' and there's an app_dir
containing '/foo/barney'.  You'll get a false match in the first loop.

Here's a thought about a possible improvement.

        file_directory = os.path.dirname(os.path.abspath(file_path))
        if file_directory not in self.application_dirs:
            return False
        return os.path.splitext(file_path)[1] == '.desktop'

This is definitely a method I'd like to see some unittests for, to explore the
corner cases.

> +
> +    def _rate_application_for_package(self, application, pkg):
> +        score = 0
> +        desktop_file = os.path.basename(application.get_filename())
> +        application_id = os.path.splitext(desktop_file)[0]
> +
> +        if application.should_show():
> +            score += 1
> +            
> +            if application_id == pkg.name:
> +                score += 5

Where do these numbers come from, and what is the purpose of the score?

> +        # TODO: Listed in app-install-data?
> +        # TODO: ID == pkg.sourcePackageName?
> +
> +        return score
> +
> +    def _get_application_for_package(self, pkg):
> +        desktop_files = []
> +        rated_applications = []
> +
> +        for installed_file in pkg.installed_files:
> +            if self._file_is_application(installed_file):
> +                desktop_files.append(installed_file)
> +
> +        app_install_pattern = os.path.join(APP_INSTALL_PATH, '%s:*' % pkg.name)
> +        for desktop_file in glob.glob(app_install_pattern):
> +            desktop_files.append(desktop_file)
> +
> +        for desktop_file in desktop_files:
> +            try:
> +                application = Gio.DesktopAppInfo.new_from_filename(desktop_file)
> +                application.set_desktop_env(self.current_desktop)
> +            except Exception as e:
> +                print("Error loading desktop file %s: %s" % installed_file, e)
> +                continue
> +            score = self._rate_application_for_package(application, pkg)
> +            if score > 0:
> +                rated_applications.append((score, application))

Would you consider adding some comments to this method, explaining its
purpose?  I'm not that familiar with the Gio APIs to understand why you are
doing the things you're doing here.  I can read the documentation on
developer.gnome.org, but I still don't understand the purpose of this method.
Some comments would really help.

> +
> +        rated_applications.sort(key=lambda app: app[0], reverse=True)

Similar to above, it's probably better to use operator.itemgetter(0) instead
of the lambda, and in this case, the in-place sorting is probably more
appropriate.

> +        if len(rated_applications) > 0:
> +            return rated_applications[0][1]

It's not clear to me what piece of information it is expecting to return.  A
comment would help (again, as would a unittest).

> +        else:
> +            return None
>  
>      def update(self, cache):
>          self.held_back = []
> @@ -83,26 +196,54 @@
>          # do the upgrade
>          self.distUpgradeWouldDelete = cache.saveDistUpgrade()
>  
> -        #dselect_upgrade_origin = UpdateOrigin(_("Previous selected"), 1)
> +        upgrade_pkgs = []
> +        pkgs_by_source = {}
> +        ungrouped_pkgs = []
>  
> -        # sort by origin
> +        # Find all upgradable packages
>          for pkg in cache:
>              if pkg.is_upgradable or pkg.marked_install:
>                  if getattr(pkg.candidate, "origins", None) is None:
>                      # can happen for e.g. locked packages
>                      # FIXME: do something more sensible here (but what?)
> -                    print("WARNING: upgradable but no candidate.origins?!?: ",
> +                    print("WARNING: upgradable but no canidateOrigin?!?: ",

Why did you change this warning string?  Did you notice the misspelling?
Isn't the original string more accurate?

>                            pkg.name)
>                      continue
> -                # check where the package belongs
> -                origin_node = cache.match_package_origin(pkg, self.matcher)
> -                if origin_node not in self.pkgs:
> -                    self.pkgs[origin_node] = []
> -                self.pkgs[origin_node].append(pkg)
> +
> +                upgrade_pkgs.append(pkg)
>                  self.num_updates = self.num_updates + 1
> +
>              if pkg.is_upgradable and not (pkg.marked_upgrade or
>                                            pkg.marked_install):
>                  self.held_back.append(pkg.name)
> -        for l in self.pkgs.keys():
> -            self.pkgs[l].sort(key=operator.attrgetter("name"))
> +
> +        # Index packages by source package name
> +        for pkg in upgrade_pkgs:
> +            srcpkg = pkg.candidate.source_name
> +            if srcpkg not in pkgs_by_source:
> +                pkgs_by_source[srcpkg] = []
> +            pkgs_by_source[srcpkg].append(pkg)

Do you know about {}.setdefault()?

http://docs.python.org/py3k/library/stdtypes.html#dict.setdefault

You can use this to make this code more succinct (and instantly recognizable
by Pythonistas):

    pkgs_by_source.setdefault(srcpkg, []).append(pkg)

> +    
> +        for srcpkg, pkgs in pkgs_by_source.items():
> +            for pkg in pkgs:
> +                app = self._get_application_for_package(pkg)
> +                if app != None:

Best practice is to always compare None using identity tests, e.g.

                if app is not None:

> +                    app_group = UpdateApplicationGroup([pkg], app)
> +                    self.pkg_groups.append(app_group)
> +                else:
> +                    ungrouped_pkgs.append(pkg)
> +        self.pkg_groups.sort(key=lambda group: group.name)

Another case where operator.attrgetter() is better than a lambda.

> +
> +        # Stick together packages and their immediate dependencies
> +        for pkg in upgrade_pkgs:
> +            for group in self.pkg_groups:
> +                result = group.add_extra(pkg)
> +                if result and pkg in ungrouped_pkgs:
> +                    ungrouped_pkgs.remove(pkg)
> +
> +        # FIXME: system_group should only have packages that are dependencies of ubuntu-meta

The line is too long.

> +        system_group = UpdateSystemGroup(ungrouped_pkgs)
> +        self.pkg_groups.append(system_group)
> +
>          self.keepcount = cache._depcache.keep_count
> +

=== modified file 'UpdateManager/UpdatesAvailable.py'
--- UpdateManager/UpdatesAvailable.py	2012-06-28 00:09:03 +0000
+++ UpdateManager/UpdatesAvailable.py	2012-07-17 18:03:22 +0000
> @@ -72,8 +72,8 @@
>  # - kill "all_changes" and move the changes into the "Update" class
>  
>  # list constants
> -(LIST_CONTENTS, LIST_NAME, LIST_PKG,
> - LIST_ORIGIN, LIST_TOGGLE_CHECKED) = range(5)
> +(LIST_NAME, LIST_UPDATE_DATA, LIST_SIZE, LIST_TOGGLE_ACTIVE) = range(4)
> +#LIST_UPDATE_DATA format: (is_toplevel, group, pkg?)

I don't understand what that comment is saying.  It looks to me like
LIST_UPDATE_DATA will just be the value 1.

>  
>  # NetworkManager enums
>  from .Core.roam import NetworkManagerHelper
> @@ -131,38 +131,64 @@
>          self.update_close_button()
>  
>          # the treeview (move into it's own code!)
> -        self.store = Gtk.ListStore(str, str, GObject.TYPE_PYOBJECT,
> -                                   GObject.TYPE_PYOBJECT, bool)
> +        self.store = Gtk.TreeStore(str, GObject.TYPE_PYOBJECT, str, bool)
>          self.treeview_update.set_model(self.store)
> -        self.treeview_update.set_headers_clickable(True)
> +        
> +        restart_icon_renderer = Gtk.CellRendererPixbuf()
> +        restart_icon_renderer.set_property("xpad", 4)
> +        restart_icon_renderer.set_property("ypad", 2)
> +        restart_icon_renderer.set_property("stock-size", Gtk.IconSize.MENU)
> +        restart_icon_renderer.set_property("follow-state", True)
> +        restart_column = Gtk.TreeViewColumn(None, restart_icon_renderer)
> +        restart_column.set_sizing(Gtk.TreeViewColumnSizing.AUTOSIZE)
> +        self.treeview_update.append_column(restart_column)
> +        restart_column.set_cell_data_func(restart_icon_renderer, self.restart_icon_renderer_data_func)

Line too long.

> +        
> +        pkg_column = Gtk.TreeViewColumn()
> +        pkg_column.set_title(_("Install"))
> +        pkg_column.set_property("spacing", 4)
> +        pkg_column.set_sizing(Gtk.TreeViewColumnSizing.FIXED)
> +        pkg_column.set_expand(True)
> +        self.treeview_update.append_column(pkg_column)
> +        
> +        # FIXME: With pkg_toggle_renderer sharing a column with other cell renderers, ATK never mentions the checkbox

Line too long.  I'll stop mentioning this now, but please do wrap your lines
to less than 79 characters long.

Also, what does this comment mean?  What's `ATK`?

> +        pkg_toggle_renderer = Gtk.CellRendererToggle()
> +        pkg_toggle_renderer.set_property("ypad", 2)
> +        pkg_toggle_renderer.connect("toggled", self.on_update_toggled)
> +        pkg_column.pack_start(pkg_toggle_renderer, False)
> +        pkg_column.add_attribute(pkg_toggle_renderer, 'active', LIST_TOGGLE_ACTIVE)
> +        pkg_column.set_cell_data_func(pkg_toggle_renderer, self.pkg_toggle_renderer_data_func)
> +        
> +        pkg_icon_renderer = Gtk.CellRendererPixbuf()
> +        pkg_icon_renderer.set_property("ypad", 2)
> +        pkg_icon_renderer.set_property("stock-size", Gtk.IconSize.MENU)
> +        pkg_column.pack_start(pkg_icon_renderer, False)
> +        pkg_column.set_cell_data_func(pkg_icon_renderer, self.pkg_icon_renderer_data_func)
> +        
> +        pkg_label_renderer = Gtk.CellRendererText()
> +        pkg_label_renderer.set_property("ypad", 2)
> +        pkg_column.pack_start(pkg_label_renderer, True)
> +        pkg_column.set_cell_data_func(pkg_label_renderer, self.pkg_label_renderer_data_func)
> +        
> +        size_renderer = Gtk.CellRendererText()
> +        size_renderer.set_property("xpad", 6)
> +        size_renderer.set_property("ypad", 0)
> +        size_renderer.set_property("xalign", 1)
> +        size_renderer.set_property("scale", 1.0/1.2) #PANGO.Scale.SMALL. Constant is not (yet) introspected.
> +        size_column = Gtk.TreeViewColumn(_("Download"), size_renderer, text=LIST_SIZE)
> +        size_column.set_sizing(Gtk.TreeViewColumnSizing.AUTOSIZE)
> +        self.treeview_update.append_column(size_column)
> +        
> +        self.treeview_update.set_headers_visible(True)
> +        self.treeview_update.set_headers_clickable(False)
>          self.treeview_update.set_direction(Gtk.TextDirection.LTR)
> -
> -        tr = Gtk.CellRendererText()
> -        tr.set_property("xpad", 6)
> -        tr.set_property("ypad", 6)
> -        cr = Gtk.CellRendererToggle()
> -        cr.set_property("activatable", True)
> -        cr.set_property("xpad", 6)
> -        cr.connect("toggled", self.toggled)
> -
> -        column_install = Gtk.TreeViewColumn(_("Install"), cr,
> -                                            active=LIST_TOGGLE_CHECKED)
> -        column_install.set_cell_data_func(cr, self.install_column_view_func)
> -        column = Gtk.TreeViewColumn(_("Name"), tr, markup=LIST_CONTENTS)
> -        column.set_resizable(True)
> -
> -        column_install.set_sizing(Gtk.TreeViewColumnSizing.FIXED)
> -        column_install.set_fixed_width(30)
> -        column.set_sizing(Gtk.TreeViewColumnSizing.FIXED)
> -        column.set_fixed_width(100)
>          self.treeview_update.set_fixed_height_mode(False)
> -
> -        self.treeview_update.append_column(column_install)
> -        column_install.set_visible(True)
> -        self.treeview_update.append_column(column)
> +        self.treeview_update.set_expander_column(pkg_column)
>          self.treeview_update.set_search_column(LIST_NAME)
> -        self.treeview_update.connect("button-press-event",
> -                                     self.show_context_menu)
> +        self.treeview_update.connect("button-press-event", self.on_treeview_button_press)
> +        # Line up row details under expanders.
> +        # FIXME: This is completely arbitrary and should probably be happening in the Ambiance theme itself.
> +        self.treeview_update.set_level_indentation(12)
>  
>          # setup the help viewer and disable the help button if there
>          # is no viewer available
> @@ -207,26 +233,85 @@
>              self.button_close.set_use_underline(False)
>  
>      def install_all_updates(self, menu, menuitem, data):
> -        self.select_all_updgrades(None)
> +        self.select_all_upgrades(None)
>          self.on_button_install_clicked(None)
>  
> -    def install_column_view_func(self, cell_layout, renderer, model, iter,
> -                                 data):
> -        pkg = model.get_value(iter, LIST_PKG)
> -        if pkg is None:
> -            renderer.set_property("activatable", True)
> -            return
> -        current_state = renderer.get_property("active")
> -        to_install = pkg.marked_install or pkg.marked_upgrade
> -        renderer.set_property("active", to_install)
> -        # we need to update the store as well to ensure orca knowns
> -        # about state changes (it will not read view_func changes)
> -        if to_install != current_state:
> -            self.store[iter][LIST_TOGGLE_CHECKED] = to_install
> -        if pkg.name in self.list.held_back:
> -            renderer.set_property("activatable", False)
> -        else:
> -            renderer.set_property("activatable", True)
> +    def restart_icon_renderer_data_func(self, cell_layout, renderer, model, iter, data):
> +        # List of packages we know require a restart.
> +        # Just an ugly hack to test the feature while packages lack the needed metadata.
> +        # FIXME: Remove this soon.
> +        restart_sourcepkgs = ['linux-meta']
> +        
> +        def pkg_requires_restart(pkg):
> +            restart_condition = pkg.candidate.record.get('XB-Restart-Required')
> +            restart_override = pkg.candidate.source_name in restart_sourcepkgs
> +            return restart_condition == 'system' or restart_override
> +        
> +        is_toplevel, group, pkg = model.get_value(iter, LIST_UPDATE_DATA)
> +        path = model.get_path(iter)
> +        
> +        requires_restart = False
> +        if is_toplevel:
> +            if self.treeview_update.row_expanded(path) == False:

You should almost never compare explicitly against True or False, and
definitely never with equality tests.  This should be rewritten:

            if not self.treeview_update.row_expanded(path):

> +                # A package in the group requires restart
> +                for group_pkg in group.pkgs:
> +                    if pkg_requires_restart(group_pkg):
> +                        requires_restart = True
> +                        break
> +        else:
> +            requires_restart = pkg_requires_restart(pkg)
> +        
> +        # FIXME: Non-standard, incorrect icon name (from app category).
> +        # Theme support for what we want seems to be lacking.
> +        if requires_restart:
> +            restart_icon_names = ['view-refresh-symbolic',
> +                                  'system-restart',
> +                                  'system-reboot']
> +            gicon = Gio.ThemedIcon.new_from_names(restart_icon_names)
> +        else:
> +            gicon = None
> +        renderer.set_property("gicon", gicon)
> +
> +    def pkg_toggle_renderer_data_func(self, cell_layout, renderer, model, iter, data):
> +        is_toplevel, group, pkg = model.get_value(iter, LIST_UPDATE_DATA)
> +        
> +        if is_toplevel:
> +            activatable = True
> +            inconsistent = group.selection_is_inconsistent()
> +        elif pkg:
> +            activatable = pkg.name not in self.list.held_back
> +            inconsistent = False
> +        
> +        # Assumes the "active" attribute is already set, tied directly to LIST_TOGGLE_ACTIVE
> +        renderer.set_property("activatable", activatable)
> +        renderer.set_property("inconsistent", inconsistent)
> +  
> +    def pkg_icon_renderer_data_func(self, cell_layout, renderer, model, iter, data):
> +        is_toplevel, group, pkg = model.get_value(iter, LIST_UPDATE_DATA)
> +        
> +        gicon = None
> +        
> +        if is_toplevel:
> +            gicon = group.icon
> +        else:
> +            gicon = Gio.ThemedIcon.new("package")

In this case, gicon will guaranteed to be set no matter which arm of the
branch is taken.  In that case, you probably shouldn't initialize gicon to
None before the conditional (it's unnecessary).

> +        
> +        renderer.set_property("gicon", gicon)
> +    
> +    def pkg_label_renderer_data_func(self, cell_layout, renderer, model, iter, data):
> +        is_toplevel, group, pkg = model.get_value(iter, LIST_UPDATE_DATA)
> +        markup = None
> +        
> +        if is_toplevel:
> +            markup = "<b>%s</b>" % group.name
> +        else:
> +            pkg_name = xml.sax.saxutils.escape(pkg.name)
> +            if not pkg.is_installed:
> +                markup = _("%s (New install)") % pkg_name
> +            else:
> +                markup = pkg_name

See my comment above, which I think also applies to `markup`.

> +        
> +        renderer.set_property("markup", markup)
>  
>      def set_changes_buffer(self, changes_buffer, text, name, srcpkg):
>          changes_buffer.set_text("")
> @@ -261,7 +346,7 @@
>          iter = model.get_iter(path)
>  
>          # set descr
> -        pkg = model.get_value(iter, LIST_PKG)
> +        is_toplevel, group, pkg = model.get_value(iter, LIST_UPDATE_DATA)
>          if (pkg is None or pkg.candidate is None or
>                  pkg.candidate.description is None):
>              changes_buffer = self.textview_changes.get_buffer()
> @@ -347,7 +432,7 @@
>          if changes:
>              self.set_changes_buffer(changes_buffer, changes, name, srcpkg)
>  
> -    def show_context_menu(self, widget, event):
> +    def on_treeview_button_press(self, widget, event):
>          """
>          Show a context menu if a right click was performed on an update entry
>          """
> @@ -358,13 +443,13 @@
>              self.menu = menu = Gtk.Menu()
>              item_select_none = \
>                  Gtk.MenuItem.new_with_mnemonic(_("_Deselect All"))
> -            item_select_none.connect("activate", self.select_none_updgrades)
> +            item_select_none.connect("activate", self.select_none_upgrades)
>              menu.append(item_select_none)
>              num_updates = self.cache.install_count
>              if num_updates == 0:
>                  item_select_none.set_property("sensitive", False)
>              item_select_all = Gtk.MenuItem.new_with_mnemonic(_("Select _All"))
> -            item_select_all.connect("activate", self.select_all_updgrades)
> +            item_select_all.connect("activate", self.select_all_upgrades)
>              menu.append(item_select_all)
>              menu.show_all()
>              menu.popup_for_device(
> @@ -373,35 +458,35 @@
>              return True
>  
>      # we need this for select all/unselect all
> -    def _toggle_origin_headers(self, new_selection_value):
> -        """ small helper that will set/unset the origin headers
> +    def _toggle_group_headers(self, new_selection_value):
> +        """ small helper that will set/unset the group headers
>          """
>          model = self.treeview_update.get_model()
>          for row in model:
> -            if not model.get_value(row.iter, LIST_PKG):
> -                model.set_value(row.iter, LIST_TOGGLE_CHECKED,
> -                                new_selection_value)
> +            is_toplevel, group, pkg = model.get_value(row.iter, LIST_UPDATE_DATA)
> +            if is_toplevel:
> +                model.set_value(row.iter, LIST_TOGGLE_ACTIVE, new_selection_value)
>  
> -    def select_all_updgrades(self, widget):
> +    def select_all_upgrades(self, widget):
>          """
>          Select all updates
>          """
>          self.setBusy(True)
>          self.cache.saveDistUpgrade()
> -        self._toggle_origin_headers(True)
> +        self._toggle_group_headers(True)
>          self.treeview_update.queue_draw()
> -        self.refresh_updates_count()
> +        self.updates_changed()
>          self.setBusy(False)
>  
> -    def select_none_updgrades(self, widget):
> +    def select_none_upgrades(self, widget):
>          """
>          Select none updates
>          """
>          self.setBusy(True)
>          self.cache.clear()
> -        self._toggle_origin_headers(False)
> +        self._toggle_group_headers(False)
>          self.treeview_update.queue_draw()
> -        self.refresh_updates_count()
> +        self.updates_changed()
>          self.setBusy(False)
>  
>      def setBusy(self, flag):
> @@ -416,8 +501,18 @@
>              self.window_main.get_window().set_cursor(None)
>          while Gtk.events_pending():
>              Gtk.main_iteration()
> +    
> +    def _mark_selected_updates(self):
> +        def foreach_cb(model, path, iter, data):
> +            is_toplevel, group, pkg = model.get_value(iter, LIST_UPDATE_DATA)
> +            if is_toplevel:
> +                active = group.packages_are_selected()
> +            elif pkg:
> +                active = pkg.marked_install or pkg.marked_upgrade
> +            model.set_value(iter, LIST_TOGGLE_ACTIVE, active)
> +        self.store.foreach(foreach_cb, None)
>  
> -    def refresh_updates_count(self):
> +    def _refresh_updates_count(self):
>          self.button_install.set_sensitive(self.cache.install_count)
>          try:
>              inst_count = self.cache.install_count
> @@ -458,10 +553,14 @@
>              self.hbox_downsize.show()
>              self.vbox_alerts.show()
>  
> +    def updates_changed(self):
> +        self._mark_selected_updates()
> +        self._refresh_updates_count()
> +
>      def update_count(self):
>          """activate or disable widgets and show dialog texts correspoding to
>             the number of available updates"""
> -        self.refresh_updates_count()
> +        self.updates_changed()
>          num_updates = self.cache.install_count
>  
>          if num_updates == 0:
> @@ -491,8 +590,6 @@
>              self.notebook_details.set_sensitive(True)
>              self.treeview_update.set_sensitive(True)
>              self.button_install.grab_default()
> -            self.treeview_update.set_cursor(Gtk.TreePath.new_from_string("1"),
> -                                            None, False)
>          self.label_header.set_markup(text_header)
>          return True
>  
> @@ -570,14 +667,14 @@
>          # can deal with dialup connections properly
>          if state in NetworkManagerHelper.NM_STATE_CONNECTING_LIST:
>              self.label_offline.set_text(_("Connecting..."))
> -            self.refresh_updates_count()
> +            self.updates_changed()
>              self.hbox_offline.show()
>              self.vbox_alerts.show()
>              self.connected = False
>          # in doubt (STATE_UNKNOWN), assume connected
>          elif (state in NetworkManagerHelper.NM_STATE_CONNECTED_LIST or
>                state == NetworkManagerHelper.NM_STATE_UNKNOWN):
> -            self.refresh_updates_count()
> +            self.updates_changed()
>              self.hbox_offline.hide()
>              self.connected = True
>              # trigger re-showing the current app to get changelog info (if
> @@ -587,7 +684,7 @@
>              self.connected = False
>              self.label_offline.set_text(_("You may not be able to check for "
>                                            "updates or download new updates."))
> -            self.refresh_updates_count()
> +            self.updates_changed()
>              self.hbox_offline.show()
>              self.vbox_alerts.show()
>  
> @@ -610,45 +707,15 @@
>              self.hbox_on_3g.hide()
>              self.hbox_roaming.hide()
>  
> -    def row_activated(self, treeview, path, column):
> -        iter = self.store.get_iter(path)
> -
> -        pkg = self.store.get_value(iter, LIST_PKG)
> -        origin = self.store.get_value(iter, LIST_ORIGIN)
> -        if pkg is not None:
> -            return
> -        self.toggle_from_origin(pkg, origin, True)
> -
> -    def toggle_from_origin(self, pkg, origin, select_all=True):
> -        self.setBusy(True)
> -        actiongroup = apt_pkg.ActionGroup(self.cache._depcache)
> -        for pkg in self.list.pkgs[origin]:
> -            if pkg.marked_install or pkg.marked_upgrade:
> -                #print("marking keep: ", pkg.name)
> -                pkg.mark_keep()
> -            elif not (pkg.name in self.list.held_back):
> -                #print("marking install: ", pkg.name)
> -                pkg.mark_install(auto_fix=False, auto_inst=False)
> -        # check if we left breakage
> -        if self.cache._depcache.broken_count:
> -            Fix = apt_pkg.ProblemResolver(self.cache._depcache)
> -            Fix.resolve_by_keep()
> -        self.refresh_updates_count()
> -        self.treeview_update.queue_draw()
> -        del actiongroup
> -        self.setBusy(False)
> -
> -    def toggled(self, renderer, path):
> +    def on_update_toggled(self, renderer, path):
>          """ a toggle button in the listview was toggled """
>          iter = self.store.get_iter(path)
> -        pkg = self.store.get_value(iter, LIST_PKG)
> -        origin = self.store.get_value(iter, LIST_ORIGIN)
> +        is_toplevel, group, pkg = self.store.get_value(iter, LIST_UPDATE_DATA)
>          # make sure that we don't allow to toggle deactivated updates
>          # this is needed for the call by the row activation callback
> -        if pkg is None:
> -            toggled_value = not self.store.get_value(iter, LIST_TOGGLE_CHECKED)
> -            self.toggle_from_origin(pkg, origin, toggled_value)
> -            self.store.set_value(iter, LIST_TOGGLE_CHECKED, toggled_value)
> +        if is_toplevel:
> +            self.toggle_from_group(group)
> +            self.updates_changed()
>              self.treeview_update.queue_draw()
>              return
>          if pkg is None or pkg.name in self.list.held_back:
> @@ -665,8 +732,8 @@
>                  pkg.mark_install()
>              except SystemError:
>                  pass
> +        self.updates_changed()
>          self.treeview_update.queue_draw()
> -        self.refresh_updates_count()
>          self.setBusy(False)
>  
>      def on_treeview_update_row_activated(self, treeview, path, column, *args):
> @@ -674,8 +741,29 @@
>          If an update row was activated (by pressing space), toggle the
>          install check box
>          """
> -        self.toggled(None, path)
> +        self.on_update_toggled(None, path)
>  
> +    def toggle_from_group(self, group):
> +        self.setBusy(True)
> +        actiongroup = apt_pkg.ActionGroup(self.cache._depcache)
> +        
> +        # Deselect all updates if any in group are selected
> +        keep_packages = group.packages_are_selected()
> +        for pkg in group.pkgs:
> +            if keep_packages:
> +                pkg.mark_keep()
> +            elif not pkg.name in self.list.held_back:
> +                pkg.mark_install(auto_fix=False, auto_inst=False)
> +        
> +        # check if we left breakage
> +        if self.cache._depcache.broken_count:
> +            Fix = apt_pkg.ProblemResolver(self.cache._depcache)
> +            Fix.resolve_by_keep()
> +        self.updates_changed()
> +        self.treeview_update.queue_draw()
> +        del actiongroup
> +        self.setBusy(False)
> +    
>      def save_state(self):
>          """ save the state  (window-size for now) """
>          if self.expander_details.get_expanded():
> @@ -703,46 +791,30 @@
>          self.dl_size = 0
>  
>          self.scrolledwindow_update.show()
> -        origin_list = sorted(
> -            self.list.pkgs, key=operator.attrgetter("importance"),
> -            reverse=True)
> -        for origin in origin_list:
> -            self.store.append(['<b><big>%s</big></b>' % origin.description,
> -                               origin.description, None, origin, True])
> -            for pkg in self.list.pkgs[origin]:
> -                name = xml.sax.saxutils.escape(pkg.name)
> -                if not pkg.is_installed:
> -                    name += _(" (New install)")
> -                summary = xml.sax.saxutils.escape(getattr(pkg.candidate,
> -                                                          "summary", None))
> -                if self.summary_before_name:
> -                    contents = "%s\n<small>%s</small>" % (summary, name)
> -                else:
> -                    contents = "<b>%s</b>\n<small>%s</small>" % (name, summary)
> -                #TRANSLATORS: the b stands for Bytes
> -                size = _("(Size: %s)") % humanize_size(getattr(pkg.candidate,
> -                                                               "size", 0))
> -                installed_version = getattr(pkg.installed, "version", None)
> -                candidate_version = getattr(pkg.candidate, "version", None)
> -                if installed_version is not None:
> -                    version = _("From version %(old_version)s "
> -                                "to %(new_version)s") % {
> -                                    "old_version": installed_version,
> -                                    "new_version": candidate_version}
> -                else:
> -                    version = _("Version %s") % candidate_version
> -                if self.show_versions:
> -                    contents = "%s\n<small>%s %s</small>" % (contents, version,
> -                                                             size)
> -                else:
> -                    contents = "%s <small>%s</small>" % (contents, size)
> -                self.store.append([contents, pkg.name, pkg, None, True])
> +        for group in self.list.pkg_groups:
> +            group_row = [
> +                group.name,
> +                (True, group, None),
> +                humanize_size(group.get_total_size()),
> +                True
> +            ]

The close-square bracket should be indented 4 spaces, and it might be helpful
to add a comment explaining what the purpose of each item is.

> +            tree_parent = self.store.append(None, group_row)
> +            
> +            for pkg in group.pkgs:
> +                pkg_row = [
> +                    pkg.name,
> +                    (False, group, pkg),
> +                    humanize_size(getattr(pkg.candidate, "size", 0)),
> +                    True
> +                ]

Similar to above.

> +                self.store.append(tree_parent, pkg_row)
> +        
>          self.treeview_update.set_model(self.store)
>          self.update_count()
>          self.setBusy(False)
>          while Gtk.events_pending():
>              Gtk.main_iteration()
> -        self.refresh_updates_count()
> +        self.updates_changed()
>          return False
>  
>      def main(self):