Software Updater

Merge lp:~dylanmccall/update-manager/group-by-applications into lp:update-manager

group-by-applications
Merge into main

Proposed by Dylan McCall on 2012-06-29

Status:	Merged
Merged at revision:	2582
Proposed branch:	lp:~dylanmccall/update-manager/group-by-applications
Merge into:	lp:update-manager
Diff against target:	859 lines (+405/-239) 2 files modified UpdateManager/Core/UpdateList.py (+175/-100) UpdateManager/UpdatesAvailable.py (+230/-139)
To merge this branch:	bzr merge lp:~dylanmccall/update-manager/group-by-applications
Related bugs:	Link a bug report
Related blueprints:	Software Updater in 13.04 (Low)

Reviewer	Date Requested	Status
Dylan McCall (community)		Needs Resubmitting on 2012-11-10
Barry Warsaw (community)	2012-06-29	Needs Fixing on 2012-07-17
Matthew Paul Thomas	2012-07-17	Pending
Review via email: mp+112678@code.launchpad.net

Description of the change

This is a first step towards grouping updates according to the specification at https://wiki.ubuntu.com/SoftwareUpdates.

There are some details still to be worked out, but the feature so far is functional :)

Changes:
- Changed the list of updates to a treeview, where updates are collapsed under application names.
- UpdateList links updated packages to applications and groups them according to dependencies.
- Restart required indicator is shown for packages with "XB-Restart-Required: system." There is a temporary override for linux-meta for testing purposes.
- Update origins are not displayed.

Known issues, to be resolved with future work:
- Indentation in the update tree view is presently hard coded and probably belongs (at least partly) in the light-themes package.
- The 'Ubuntu base' update group should list specific core packages, not 'everything else.' The specification is unclear on what to do with everything else, so it's like this for the time being.
- The Updates Required icon does not have its corresponding UI element below the download size text.
- The Download column does not yet show a check mark for downloaded packages.
- "Terminal" has an expander, with "gnome-terminal" as a child. Instead, group headers should simply be packages instead of their own things.

So, it isn't _finished_, but it does seem to be in a working state. The reason I'm interested in merging with trunk is because I want to keep our code in sync. Most of the crazy merge conflicts are probably done with now (especially since we seem to be standardized on 4 space indents). However, my diff spans _a lot_ of UpdateList.py and UpdatesAvailable.py, so any conflicts can be a real pain to deal with.

lp:~dylanmccall/update-manager/group-by-applications updated on 2012-06-29

2442. By Dylan McCall on 2012-06-29: Attempt to load a symbolic icon for 'restart required'.
Don't select second item in update list.

Revision history for this message

Barry Warsaw (barry) wrote on 2012-07-17:

See detailed review sent separately.

review: Needs Fixing

Revision history for this message

Barry Warsaw (barry) wrote on 2012-07-17:

Download full text (43.2 KiB)

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, wri...

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 <mvo@debian.org>
> -#
> -# 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 = "%s" % 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(['<big>%s</big>' % 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%s" % (summary, name)
> - else:
> - contents = "%s\n%s" % (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%s %s" % (contents, version,
> - size)
> - else:
> - contents = "%s %s" % (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):

lp:~dylanmccall/update-manager/group-by-applications updated on 2012-08-20

2443. By Dylan McCall on 2012-08-18: Merged changes from trunk.
Removed unnecessary TODO notes in UpdateList.py
2444. By Dylan McCall on 2012-08-20: Fixed unintended formatting changes.
2445. By Dylan McCall on 2012-08-20: Merged changes from trunk.
2446. By Dylan McCall on 2012-08-20: Fixed some code style errors in UpdateList.

Revision history for this message

Dylan McCall (dylanmccall) wrote on 2012-08-20:

Okay, I'm finally working on this and I'll let you know when it's all done. (Couple days, I hope). A lot of the weirder issues are due to me manually merging changes, apparently to the wrong version of UpdateList.py. Unfortunate waste of time, so thank you for being so patient, Barry! :)

lp:~dylanmccall/update-manager/group-by-applications updated on 2012-11-10

2447. By Dylan McCall on 2012-08-21: Fixed lines longer than 79 characters in UpdateList.py.
2448. By Dylan McCall on 2012-11-10: Fixed style and commenting issues in UpdatesAvailable.py.
2449. By Dylan McCall on 2012-11-10: Merged changes from trunk

Revision history for this message

Dylan McCall (dylanmccall) wrote on 2012-11-10:

There, I think that covers the immediate problems. Those bizarre whitespace changes were because mterry had cleaned stuff up beautifully (along with the Python 3 update) between me starting and finishing this, and then I merged changes in the worst possible way. I'm sorry for taking so very long to get back to you.

Replacing pkg with package in UpdateList.py:
I'd like to leave "pkg" be in this branch, if that's okay, just because pkg seems to appear consistently across the application and in the previous version of UpdateList.py (even in the apt.pkg library we import). I agree it would be lovely to clean up, though. I honestly do cringe every time I abbreviate something :)

You were wondering about that weird function called _rate_application_for_package. Originally I had expected to need some fancier heuristics, but we seem to be getting along fine with what we have here. I agree with you, and I'll deal with that soon.

get_application_for_package:
GIO has a lovely function to parse a .desktop file, spitting out a class with a (localized) name, icon and such. The important bit is it's the same code we use in places like the Unity dash or the Applications menu in Gnome Panel, so it's all nice and familiar. Adding some comments to explain the purpose...

APP_INSTALL_PATH: Right now that is effectively the same thing that happens in Software Centre. I'm happy to turn it into a config parameter :)

I have some refactoring to do (and some new designs to implement!), so I think I will do that, and I will try to do tests and documentation alongside that work. I will resubmit then.

review: Needs Resubmitting

Revision history for this message

Marco Biscaro (marcobiscaro2112) wrote on 2012-12-17:

I tried to merge trunk and got conflicts. You should merge trunk again and fix them (in UpdateManager/Core/UpdateList.py).

Some details:

=== modified file 'UpdateManager/Core/UpdateList.py'
--- UpdateManager/Core/UpdateList.py 2012-08-20 15:32:45 +0000
+++ UpdateManager/Core/UpdateList.py 2012-11-10 06:29:19 +0000
@@ -1,7 +1,7 @@
# UpdateList.py
# -*- Mode: Python; indent-tabs-mode: nil; tab-width: 4; coding: utf-8 -*-
#
-# Copyright (c) 2004-2012 Canonical
+# Copyright (c) 2004-2008 Canonical

I think this shouldn't be modified.

#
# Author: Michael Vogt <email address hidden>
#

You modified this file a lot, I think your name should be here too. :)

@@ -22,53 +22,103 @@

from __future__ import print_function

+import warnings
+warnings.filterwarnings("ignore", "Accessed deprecated property",
+ DeprecationWarning)
+
from gettext import gettext as _
-
-import apt
-import logging
import operator
-import random
+import itertools
import subprocess
+import os
+import glob
import sys

What about alphabetical order here?

Revision history for this message

Michael Terry (mterry) wrote on 2012-12-17:

Dylan, I wanted to help push this over the hump. I've got a merged-with-trunk branch here: lp:~mterry/update-manager/group-by-application

I'll look into adding some unittests next and fixing the single-package headers next. Is there a team we could put a shared branch under?

Revision history for this message

Dylan McCall (dylanmccall) wrote on 2012-12-17:

Cool! There is not one at the moment, but feel free to add me to one. (I'll
be able to do that in about 8 hours).

I have a few local changes that I'm aiming to finish up and push by Sunday,
so we might want to chat about those over email or IRC or something, too.
They could make things a lot easier (once they are not broken),
particularly with single packages and packages that aren't applications;)

On Mon, Dec 17, 2012 at 11:04 AM, Michael Terry <<email address hidden>
> wrote:

> Dylan, I wanted to help push this over the hump. I've got a
> merged-with-trunk branch here:
> lp:~mterry/update-manager/group-by-application
>
> I'll look into adding some unittests next and fixing the single-package
> headers next. Is there a team we could put a shared branch under?
> --
>
> https://code.launchpad.net/~dylanmccall/update-manager/group-by-applications/+merge/112678
> You are the owner of lp:~dylanmccall/update-manager/group-by-applications.
>

Revision history for this message

Marco Biscaro (marcobiscaro2112) wrote on 2012-12-17:

I can help too!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Brian Murray

Cassandra Prentiss

Dylan McCall

Marco Biscaro

Michael Vogt

Nathan Stratton Treadway

paul janssen

to status/vote changes:

Phearret

 === modified file 'UpdateManager/Core/UpdateList.py'
 --- UpdateManager/Core/UpdateList.py	2012-08-20 15:32:45 +0000
 +++ UpdateManager/Core/UpdateList.py	2012-11-10 06:29:19 +0000
@@ -1,7 +1,7 @@
  # UpdateList.py
  # -*- Mode: Python; indent-tabs-mode: nil; tab-width: 4; coding: utf-8 -*-
+ #
--#  Copyright (c) 2004-2012 Canonical
++#  Copyright (c) 2004-2008 Canonical
+ #
  #  Author: Michael Vogt <mvo@debian.org>
+ #
@@ -22,53 +22,103 @@
  from __future__ import print_function
++import warnings
++warnings.filterwarnings("ignore", "Accessed deprecated property",
++                        DeprecationWarning)
++
  from gettext import gettext as _
--
--import apt
--import logging
  import operator
--import random
++import itertools
  import subprocess
++import os
++import glob
  import sys
--
--class UpdateOrigin(object):
++from gi.repository import Gio
++
++APP_INSTALL_PATH = "/usr/share/app-install/desktop"
++
++class UpdateGroup():
++    def __init__(self, core_pkgs, name, icon):
++        self._core_pkgs = set(core_pkgs)
++        self._extra_pkgs = set()
++        self.name = name
++        self.icon = icon
++
++    @property
++    def pkgs(self):
++        all_pkgs = []
++        all_pkgs.extend(self._core_pkgs)
++        all_pkgs.extend(self._extra_pkgs)
++        return sorted(all_pkgs, key=operator.attrgetter('name'))
++
++    def add_extra(self, extra_pkg):
++        if self._contains(extra_pkg):
++            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):
++        for core_pkg in self._core_pkgs:
++            candidate = core_pkg.candidate
++            dependencies = candidate.get_dependencies('Depends', 'Recommends')
++            for dependency_pkg in itertools.chain.from_iterable(dependencies):
++                if dependency_pkg.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))
++
++    def get_total_size(self):
++        size = 0
++        for pkg in self.pkgs:
++            size += getattr(pkg.candidate, "size", 0)
++        return size
++
++
++class UpdateApplicationGroup(UpdateGroup):
++    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():
      def __init__(self, desc, importance):
          self.packages = []
          self.importance = importance
          self.description = desc
--class OriginsImportance:
--    # filed in by us
--    SECURITY = 10
--    UPDATES = 9
--    PROPOSED = 8
--    BACKPORTS = 7
--    ARCHIVE = 6
--    # this is filed in by MyCache
--    OTHER = 0
--    # this is used by us
--    OTHER_UNKNOWN = -1
--
--
--class UpdateList(object):
++class UpdateList():
      """
      class that contains the list of available updates in
      self.pkgs[origin] where origin is the user readable string
      """
--    # the key in the debian/control file used to add the phased
--    # updates percentage
--    PHASED_UPDATES_KEY = "Phased-Update-Percentage"
--
--    # the file that contains the uniq machine id
--    UNIQ_MACHINE_ID_FILE = "/var/lib/dbus/machine-id"
--
--    # the configuration key to turn phased-updates always on
--    ALWAYS_INCLUDE_PHASED_UPDATES = (
--        "Update-Manager::Always-Include-Phased-Updates")
--
      def __init__(self, parent):
          # a map of packages under their origin
          try:
@@ -81,59 +131,64 @@
                             "you are using.") % e)
              sys.exit(1)
          self.distUpgradeWouldDelete = 0
--        self.pkgs = {}
++        self.pkg_groups = []
          self.num_updates = 0
--        self.matcher = self.initMatcher(dist)
--        self.random = random.Random()
--        # a stable machine uniq id
--        with open(self.UNIQ_MACHINE_ID_FILE) as f:
--            self.machine_uniq_id = f.read()
--
--    def initMatcher(self, dist):
--        # (origin, archive, description, importance)
--        matcher_templates = [
--            ("%s-security" % dist, "Ubuntu", _("Important security updates"),
--             OriginsImportance.SECURITY),
--            ("%s-updates" % dist, "Ubuntu", _("Recommended updates"),
--             OriginsImportance.UPDATES),
--            ("%s-proposed" % dist, "Ubuntu", _("Proposed updates"),
--             OriginsImportance.PROPOSED),
--            ("%s-backports" % dist, "Ubuntu", _("Backports"),
--             OriginsImportance.BACKPORTS),
--            (dist, "Ubuntu", _("Distribution updates"),
--             OriginsImportance.ARCHIVE),
--            (None, None, _("Other updates"), OriginsImportance.OTHER_UNKNOWN)
--        ]
--        matcher = {}
--        for (origin, archive, desc, importance) in matcher_templates:
--            matcher[(origin, archive)] = UpdateOrigin(desc, importance)
--        return matcher
--
--    def is_ignored_phased_update(self, pkg):
--        """ This will test if the pkg is a phased updated and if
--            it needs to get installed or ignored.
--
--            :return: True if the updates should be ignored
--        """
--        # allow the admin to override this
--        if apt.apt_pkg.config.find_b(
--            self.ALWAYS_INCLUDE_PHASED_UPDATES, False):
--            return False
--
--        if self.PHASED_UPDATES_KEY in pkg.candidate.record:
--            # its important that we always get the same result on
--            # multiple runs of the update-manager, so we need to
--            # feed a seed that is a combination of the pkg/ver/machine
--            self.random.seed("%s-%s-%s" % (
--                    pkg.name, pkg.candidate.version,
--                    self.machine_uniq_id))
--            threshold = pkg.candidate.record[self.PHASED_UPDATES_KEY]
--            percentage = self.random.randint(0, 100)
--            if percentage > int(threshold):
--                logging.info("holding back phased update (%s < %s)" % (
--                        threshold, percentage))
--                return True
--        return False
++        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
++
++    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
++
++        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))
++
++        rated_applications.sort(key=lambda app: app[0], reverse=True)
++        if len(rated_applications) > 0:
++            return rated_applications[0][1]
++        else:
++            return None
      def update(self, cache):
          self.held_back = []
@@ -141,9 +196,11 @@
          # 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:
@@ -152,22 +209,40 @@
                      print("WARNING: upgradable but no candidate.origins?!?: ",
                            pkg.name)
                      continue
--                # check where the package belongs
--                origin_node = cache.match_package_origin(pkg, self.matcher)
--
--                # see if its a phased update and *not* a security update
--                # or shadowing a security update
--                if (origin_node.importance != OriginsImportance.SECURITY and
--                    self.is_ignored_phased_update(pkg)):
--                    continue
--
--                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
++            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 is not None:
++                    app_group = UpdateApplicationGroup([pkg], app)
++                    self.pkg_groups.append(app_group)
++                else:
++                    ungrouped_pkgs.append(pkg)
++        self.pkg_groups.sort(key=operator.attrgetter('name'))
++
++        # 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
++        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-10-10 08:13:13 +0000
 +++ UpdateManager/UpdatesAvailable.py	2012-11-10 06:29:19 +0000
@@ -70,10 +70,12 @@
  # FIXME:
  # - kill "all_changes" and move the changes into the "Update" class
++# - screen reader does not read update toggle state
++# - screen reader does not say "Downloaded" for downloaded updates
  # 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 is a tuple of (is_toplevel, group, pkg)
  # NetworkManager enums
  from .Core.roam import NetworkManagerHelper
@@ -134,38 +136,71 @@
          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)
++
++        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)
++
++        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)
++        # 1.0/1.2 == PANGO.Scale.SMALL. Constant is not (yet) introspected.
++        size_renderer.set_property("scale", 1.0/1.2)
++        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.on_treeview_button_press)
++        # Line up row details under expanders.
++        # FIXME: This is arbitrary and should happen in the Gtk theme itself
++        self.treeview_update.set_level_indentation(12)
          # setup the help viewer and disable the help button if there
          # is no viewer available
@@ -210,26 +245,83 @@
              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.
++        # 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 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)
++
++        if is_toplevel:
++            gicon = group.icon
++        else:
++            gicon = Gio.ThemedIcon.new("package")
++
++        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)
++
++        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
++
++        renderer.set_property("markup", markup)
      def set_changes_buffer(self, changes_buffer, text, name, srcpkg):
          changes_buffer.set_text("")
@@ -264,7 +356,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()
@@ -350,7 +442,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
          """
@@ -361,10 +453,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 +468,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):
@@ -417,7 +512,17 @@
          while Gtk.events_pending():
              Gtk.main_iteration()
--    def refresh_updates_count(self):
++    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):
          self.button_install.set_sensitive(self.cache.install_count)
          try:
              inst_count = self.cache.install_count
@@ -458,10 +563,15 @@
              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
          text_header = None
          text_desc = None
@@ -571,14 +681,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
@@ -588,7 +698,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()
@@ -611,45 +721,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:
@@ -666,8 +746,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):
@@ -675,7 +755,28 @@
          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) """
@@ -704,46 +805,36 @@
          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", ""))
--                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])
++
++        # add update groups and packages to self.store. Each row contains:
++        # row label (for screen reader),
++        # update data tuple (is_toplevel, group object, package object),
++        # update size,
++        # update selection state
++        for group in self.list.pkg_groups:
++            group_row = [
++                group.name,
++                (True, group, None),
++                humanize_size(group.get_total_size()),
++                True
++                ]
++            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
++                    ]
++                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):

Software Updater

Merge lp:~dylanmccall/update-manager/group-by-applications into lp:update-manager

Commit message

Description of the change

Preview Diff

Subscribers