[Merge] ~osomon/software-properties:updates-refresh into software-properties:ubuntu/master

Olivier Tilloy olivier.tilloy at canonical.com
Tue Feb 18 20:54:35 UTC 2020


Thanks for the review, much appreciated!
I have fixed the "what if we only have updates" case, thanks very much for catching it! I have also applied some suggested changes, and replied inline otherwise.

Diff comments:

> diff --git a/softwareproperties/gtk/SoftwarePropertiesGtk.py b/softwareproperties/gtk/SoftwarePropertiesGtk.py
> index ae35ec0..8e59ae3 100644
> --- a/softwareproperties/gtk/SoftwarePropertiesGtk.py
> +++ b/softwareproperties/gtk/SoftwarePropertiesGtk.py
> @@ -83,6 +84,27 @@ RESPONSE_ADD = 2
>      STORE_VISIBLE
>  ) = list(range(5))
>  
> +# rows for the updates subscription combobox
> +(
> +    UPDATES_ALL,
> +    UPDATES_RECOMMENDED,
> +    UPDATES_SECURITY,
> +    UPDATES_CUSTOM,
> +) = range(4)
> +
> +# names of the pockets
> +POCKET_SECURITY = "security"
> +POCKET_UPDATES = "updates"
> +POCKET_BACKPORTS = "backports"
> +
> +
> +def maybe_log_authentication_canceled_error(dbus_exception):

Good suggestion. Done!

> +    if dbus_exception._dbus_error_name == \
> +      'com.ubuntu.SoftwareProperties.PermissionDeniedByPolicy':
> +        logging.error("Authentication canceled, changes have not been saved")
> +        return True
> +    return False
> +
>  
>  def error(parent_window, summary, msg):
>      """ show a error dialog """
> @@ -347,29 +369,28 @@ class SoftwarePropertiesGtk(SoftwareProperties, SimpleGtkbuilderApp):
>              self.vbox_dist_comps.add(checkbox)
>              checkbox.show()
>  
> -        # Setup the checkbuttons for the child repos / updates
> -        for checkbutton in self.vbox_updates.get_children():
> -            self.vbox_updates.remove(checkbutton)
> +        # Setup the checkbuttons for the child repos
>          for checkbutton in self.dev_box.get_children():
>              self.dev_box.remove(checkbutton)
> -        if len(self.distro.source_template.children) < 1:
> -            self.frame_children.hide()
>          for template in self.distro.source_template.children:
>              # Do not show source entries in there
>              if template.type == "deb-src":
>                  continue
> -
> -            checkbox = Gtk.CheckButton(label="%s (%s)" % (template.description,
> -                                                          template.name))
> -            checkbox.template = template
> -            self.handlers[checkbox] = checkbox.connect("toggled",
> -                                                   self.on_checkbutton_child_toggled,
> -                                                   template)
>              if "proposed" in template.name:
> +                checkbox = Gtk.CheckButton(label="%s (%s)" % (template.description,
> +                                                              template.name))
> +                checkbox.template = template
> +                self.handlers[checkbox] = \
> +                    checkbox.connect("toggled",
> +                                     self.on_checkbutton_child_toggled,
> +                                     template)

Yeah, in terms of style consistency this codebase is all over the place. I would be good to fix this later on, but not in the merge request, as it would introduce a lot of noise.

>                  self.dev_box.add(checkbox)
> -            else:
> -                self.vbox_updates.add(checkbox)
> -            checkbox.show()
> +                checkbox.show()
> +
> +        # Setup the combo box for updates subscriptions
> +        combobox = self.combobox_updates_subscription
> +        self.handlers[combobox] = combobox.connect(

This is used by the block_handlers() and unblock_handlers() methods, which are called when populating pages to avoid getting in an inconsistent state.
No explicit disconnection is ever performed.

> +            "changed", self.on_combobox_updates_subscription_changed)
>  
>          # setup the server chooser
>          cell = Gtk.CellRendererText()
> @@ -416,14 +431,54 @@ class SoftwarePropertiesGtk(SoftwareProperties, SimpleGtkbuilderApp):
>              checkbox.set_inconsistent(inconsistent)
>              checkbox.set_active(active)
>  
> +        # Refresh the combo box for updates subscriptions
> +        security_subscription = False
> +        updates_subscription = False
> +        backports_subscription = False
> +        other_subscriptions = False
> +        for template in self.distro.source_template.children:
> +            # Do not show source entries in there
> +            if template.type == "deb-src":
> +                continue
> +            pocket = template.name.split('-')[-1]
> +            (active, inconsistent) = self.get_comp_child_state(template)
> +            if active:
> +                if pocket == POCKET_SECURITY:
> +                    security_subscription = True
> +                elif pocket == POCKET_UPDATES:
> +                    updates_subscription = True
> +                elif pocket == POCKET_BACKPORTS:
> +                    backports_subscription = True
> +                else:
> +                    other_subscriptions = True
> +        combobox = self.combobox_updates_subscription
> +        model = combobox.get_model()
> +        if other_subscriptions:
> +            try:
> +                model.get_iter_from_string(str(UPDATES_CUSTOM))

The documentation appears to lie… In my tests if the index can't be found in the model, get_iter_from_string raises a ValueError, it doesn't return False.

> +            except ValueError:
> +                model.append([_("Custom")])
> +            combobox.set_active(UPDATES_CUSTOM)
> +        else:
> +            try:
> +                model.remove(model.get_iter_from_string(str(UPDATES_CUSTOM)))
> +            except ValueError:
> +                pass
> +            if security_subscription and updates_subscription and backports_subscription:

Very good catch! In those cases, the selected entry should be "Custom", of course. This should be fixed now.

> +                combobox.set_active(UPDATES_ALL)
> +            elif security_subscription and updates_subscription:
> +                combobox.set_active(UPDATES_RECOMMENDED)
> +            elif security_subscription:
> +                combobox.set_active(UPDATES_SECURITY)
> +
>          # If no components are enabled there will be no need for updates
>          # and source code
>          if len(self.distro.enabled_comps) < 1:
> -            self.vbox_updates.set_sensitive(False)
> +            self.combobox_updates_subscription.set_sensitive(False)
>              self.dev_box.set_sensitive(False)
>              self.checkbutton_source_code.set_sensitive(False)
>          else:
> -            self.vbox_updates.set_sensitive(True)
> +            self.combobox_updates_subscription.set_sensitive(True)
>              self.dev_box.set_sensitive(True)
>              self.checkbutton_source_code.set_sensitive(True)
>  
> @@ -537,15 +590,52 @@ class SoftwarePropertiesGtk(SoftwareProperties, SimpleGtkbuilderApp):
>          try:
>              self.backend.SetReleaseUpgradesPolicy(i)
>          except dbus.DBusException as e:
> -            if e._dbus_error_name == 'com.ubuntu.SoftwareProperties.PermissionDeniedByPolicy':
> -                logging.error("Authentication canceled, changes have not been saved")
> -
> +            if maybe_log_authentication_canceled_error(e):
>                  combo_handler = self.handlers[self.combobox_release_upgrades]
>                  self.combobox_release_upgrades.handler_block(combo_handler)
>                  i = self.get_release_upgrades_policy()
>                  self.combobox_release_upgrades.set_active(i)
>                  self.combobox_release_upgrades.handler_unblock(combo_handler)
>  
> +    def on_combobox_updates_subscription_changed(self, combobox):
> +        index = combobox.get_active()
> +        if index != UPDATES_CUSTOM:
> +            model = combobox.get_model()
> +            try:
> +                model.remove(model.get_iter_from_string(str(UPDATES_CUSTOM)))
> +            except ValueError:
> +                pass
> +
> +        def toggle_child_source(pocket, enabled):

Agreed, prefixing with an underscore makes it slightly more obvious that it's private/local. Done.

> +            source = "{}-{}".format(self.distro.codename, pocket)
> +            if enabled:
> +                logging.info("enabling {}".format(source))
> +                self.backend.EnableChildSource(source)
> +            else:

I'd agree if there was a preferred/more frequent code path, but in that case both branches are equally valid, so I think an if/else statement is suited.

> +                logging.info("disabling {}".format(source))
> +                self.backend.DisableChildSource(source)
> +
> +        try:
> +            if index == UPDATES_ALL:
> +                toggle_child_source(POCKET_SECURITY, True)
> +                toggle_child_source(POCKET_UPDATES, True)
> +                toggle_child_source(POCKET_BACKPORTS, True)
> +            elif index == UPDATES_RECOMMENDED:
> +                toggle_child_source(POCKET_SECURITY, True)
> +                toggle_child_source(POCKET_UPDATES, True)
> +                toggle_child_source(POCKET_BACKPORTS, False)
> +            elif index == UPDATES_SECURITY:
> +                toggle_child_source(POCKET_SECURITY, True)
> +                toggle_child_source(POCKET_UPDATES, False)
> +                toggle_child_source(POCKET_BACKPORTS, False)
> +            for template in self.distro.source_template.children:
> +                if template.type == "deb-src":
> +                    continue
> +                pocket = template.name.split('-')[-1]
> +                if pocket not in (POCKET_SECURITY, POCKET_UPDATES, POCKET_BACKPORTS):
> +                    toggle_child_source(pocket, False)
> +        except dbus.DBusException as e:
> +            maybe_log_authentication_canceled_error(e)
>  
>      def on_combobox_server_changed(self, combobox):
>          """


-- 
https://code.launchpad.net/~osomon/software-properties/+git/software-properties/+merge/379345
Your team Ubuntu Core Development Team is subscribed to branch software-properties:ubuntu/master.



More information about the Ubuntu-reviews mailing list