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

Didier Roche didrocks at ubuntu.com
Tue Feb 18 17:23:07 UTC 2020


Review: Needs Information

Here we go: Note that I didn’t test the ppa yet. I have some comments, some suggestions, but apart from my "what if we only have updates" or so, nothing blocking once this one is answered.

Good work!

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):

It doesn’t seem to be the case for the rest of the project, but I would add a doc string here, explaining in particular the return value (which is ignored in most of the call)

Thanks for factorizing this out btw, looking at the code, this was really needed :)

> +    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)

You have mixed some style (identation) changes and some other ones. Not a biggie, but I’m used to separate those (unsure about this project old rules though). It’s just a FYI :)

(and I was about commenting the "this is not properly aligned" before realizing I was commenting on the "-" that you fixed :p)

>                  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(

I don’t see the rest of the code, but you register it in self.handlers because all handlers are disconnected on page changes? (I think this is similar to the previous self.handlers[checkbox]). Asking as I don’t see it used explicitely anywhere else in this code.

> +            "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))

What if get_iter_from_string returns false (can’t find it in the model) from https://people.gnome.org/~shaunm/girdoc/Python/Gtk.TreeModel.get_iter_from_string.html, shouldn’t we fallback to the manual model.append([_("Custom")]) as well?

> +            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:

So, if I edit manually my sources.list and I enable -security and -backports but not updates, what happens?
Same question if I enable updates and nothing else?

I may miss something, like the combobox has a good default, but I don’t see those cases nor a default "else" here.

> +                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):

Unsure about the styling of this project (again) :p but I tend to have inner closures marked as _toogle_child_source for instance to make it obvious we have a local function here. Just a comment, nothing mandatory as it’s my personal pref :)

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

Same, really personal preference here, but just writing it FTR.

I prefer this style (so that the happy path is mostly left aligned):
if enabled:
    logging.info("enabling {}".format(source))
    self.backend.EnableChildSource(source)
    return
logging.info("disabling {}".format(source))
self.backend.DisableChildSource(source)

Again, if this is not coherent with the rest of the code, feel free to ignore

> +                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