[Merge] lp:~azzar1/software-properties/add-canonical-livepatch into lp:software-properties

[Brian Murray]

I'm not sure I'm the best person to evaluate all the gtk code so I've identified some other things mostly nit-picks but I do think using python3-distro-info would quite a good improvement.

Diff comments:

> === added file 'data/gtkbuilder/dialog-auth.ui'
> --- data/gtkbuilder/dialog-auth.ui	1970-01-01 00:00:00 +0000
> +++ data/gtkbuilder/dialog-auth.ui	2018-03-16 13:02:28 +0000
> @@ -0,0 +1,134 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!-- Generated with glade 3.20.4 -->
> +<interface>
> +  <requires lib="gtk+" version="3.10"/>
> +  <object class="GtkDialog" id="dialog_auth">
> +    <property name="can_focus">False</property>
> +    <property name="resizable">False</property>
> +    <property name="modal">True</property>
> +    <property name="destroy_with_parent">True</property>
> +    <property name="type_hint">dialog</property>
> +    <property name="deletable">False</property>
> +    <child internal-child="vbox">
> +      <object class="GtkBox" id="box_dialog">
> +        <property name="can_focus">False</property>
> +        <property name="orientation">vertical</property>
> +        <property name="spacing">2</property>
> +        <child internal-child="action_area">
> +          <object class="GtkButtonBox" id="dialog-action_area1">
> +            <property name="can_focus">False</property>
> +          </object>
> +          <packing>
> +            <property name="expand">False</property>
> +            <property name="fill">False</property>
> +            <property name="position">0</property>
> +          </packing>
> +        </child>
> +        <child>
> +          <object class="GtkGrid" id="main_grid">
> +            <property name="visible">True</property>
> +            <property name="can_focus">False</property>
> +            <property name="border_width">12</property>
> +            <property name="row_spacing">12</property>
> +            <property name="column_spacing">12</property>
> +            <child>
> +              <object class="GtkLabel" id="label_title">
> +                <property name="visible">True</property>
> +                <property name="can_focus">False</property>
> +                <property name="hexpand">True</property>
> +                <property name="label" translatable="yes">To enable Livepatch choose an Ubuntu Single Sign-on account.</property>
> +                <property name="wrap">True</property>
> +                <property name="xalign">0</property>
> +              </object>
> +              <packing>
> +                <property name="left_attach">0</property>
> +                <property name="top_attach">0</property>
> +              </packing>
> +            </child>
> +            <child>
> +              <object class="GtkButton" id="button_settings">
> +                <property name="visible">True</property>
> +                <property name="can_focus">True</property>
> +                <property name="receives_default">True</property>
> +                <property name="tooltip_text" translatable="yes">Open online account settings</property>
> +                <property name="valign">start</property>
> +                <child>
> +                  <object class="GtkImage" id="settings_button_image">
> +                    <property name="visible">True</property>
> +                    <property name="can_focus">False</property>
> +                    <property name="icon_name">emblem-system-symbolic</property>
> +                  </object>
> +                </child>
> +              </object>
> +              <packing>
> +                <property name="left_attach">1</property>
> +                <property name="top_attach">0</property>
> +              </packing>
> +            </child>
> +            <child>
> +              <object class="GtkFrame" id="main_frame">
> +                <property name="visible">True</property>
> +                <property name="can_focus">False</property>
> +                <property name="label_xalign">0</property>
> +                <child>
> +                  <object class="GtkListBox" id="listbox_accounts">
> +                    <property name="visible">True</property>
> +                    <property name="can_focus">False</property>
> +                    <property name="selection_mode">none</property>
> +                    <child>
> +                      <object class="GtkListBoxRow" id="listboxrow_new_account">
> +                        <property name="visible">True</property>
> +                        <property name="can_focus">False</property>
> +                        <child>
> +                          <object class="GtkLabel" id="label_new_account">
> +                            <property name="height_request">48</property>
> +                            <property name="visible">True</property>
> +                            <property name="can_focus">False</property>
> +                            <property name="halign">center</property>
> +                            <property name="valign">center</property>
> +                            <property name="label" translatable="yes"><b>Use another account...</b></property>
> +                            <property name="use_markup">True</property>
> +                            <property name="justify">center</property>
> +                          </object>
> +                        </child>
> +                      </object>
> +                    </child>
> +                  </object>
> +                </child>
> +              </object>
> +              <packing>
> +                <property name="left_attach">0</property>
> +                <property name="top_attach">1</property>
> +                <property name="width">2</property>
> +              </packing>
> +            </child>
> +          </object>
> +          <packing>
> +            <property name="expand">False</property>
> +            <property name="fill">True</property>
> +            <property name="position">1</property>
> +          </packing>
> +        </child>
> +      </object>
> +    </child>
> +    <child type="titlebar">
> +      <object class="GtkHeaderBar">
> +        <property name="visible">True</property>
> +        <property name="can_focus">False</property>
> +        <property name="title">Choose an account</property>

I'm not very familiar with gtk but it is possible to make title translatable?

> +        <child>
> +          <object class="GtkButton" id="button_cancel">
> +            <property name="label">gtk-cancel</property>
> +            <property name="visible">True</property>
> +            <property name="can_focus">True</property>
> +            <property name="receives_default">True</property>
> +            <property name="use_stock">True</property>
> +          </object>
> +        </child>
> +      </object>
> +    </child>
> +    <action-widgets>
> +      <action-widget response="-6">button_cancel</action-widget>
> +    </action-widgets>
> +  </object>
> +</interface>
> 
> === modified file 'debian/changelog'
> --- debian/changelog	2018-03-06 16:06:38 +0000
> +++ debian/changelog	2018-03-16 13:02:28 +0000
> @@ -1,3 +1,17 @@
> +software-properties (0.96.24.25) UNRELEASED; urgency=medium
> +
> +  * DialogAuth.py: Implement a dialog to choose between an ubuntu sso account or
> +    login into a new one. This interacts with gnome-online-accounts.
> +  * DialogLivepatchError: Implement a dialog to show livepatch reletated errors.

The word "related" has a typo in it.

> +  * GoaAuth.py: Implement an utility function to store manage livepatch credentials.
> +  * SoftwareProperties.py: Implement functions used by the dbus API to
> +    enable/disable livepatch.
> +  * SoftwarePropertiesDbus.py: Add SetLivepatchEnabled to the dbus API.
> +  * SoftwarePropertiesGtk.py: Add a livepatch switch.
> +  * debian/control: Depends on gir1.2-goa-1.0, gir1.2-snapd-1 and gir1.2-secret-1.
> +
> + -- Andrea Azzarone <andrea.azzarone at canonical.com>  Wed, 14 Mar 2018 20:24:31 +0100
> +
>  software-properties (0.96.24.23) bionic; urgency=medium
>  
>    * SoftwarePropertiesGtk.py: After installing proprietary drivers provide the
> 
> === modified file 'softwareproperties/SoftwareProperties.py'
> --- softwareproperties/SoftwareProperties.py	2017-03-01 17:35:37 +0000
> +++ softwareproperties/SoftwareProperties.py	2018-03-16 13:02:28 +0000
> @@ -858,6 +866,128 @@
>      except:
>        return False
>  
> +  #
> +  # Livepatch
> +  #
> +  def init_snapd(self):
> +      self.snapd_client = Snapd.Client()
> +
> +  def get_livepatch_snap_async(self, callback):
> +      assert self.snapd_client
> +      self.snapd_client.list_one_async('canonical-livepatch',
> +                                        self.cancellable,
> +                                        self.on_list_one_ready_cb,
> +                                        callback)
> +
> +  def on_list_one_ready_cb(self, source_object, result, user_data):
> +      callback = user_data
> +      try:
> +          snap = source_object.list_one_finish(result)
> +      except:
> +          snap = None
> +      if snap:
> +          if callback: callback(snap)
> +          return
> +      else:
> +          assert self.snapd_client
> +          self.snapd_client.find_async(Snapd.FindFlags.MATCH_NAME,
> +                                       'canonical-livepatch',
> +                                        self.cancellable,
> +                                        self.on_find_ready_cb,
> +                                        callback)
> +
> +  def on_find_ready_cb(self, source_object, result, user_data):
> +      callback = user_data
> +      try:
> +          snaps = source_object.find_finish(result)[0]
> +      except:
> +          snaps = list()
> +      snap = snaps[0] if len(snaps) else None
> +      if callback: callback(snap)
> +
> +  def get_livepatch_snap_status(self, snap):
> +      if snap is None:
> +          return Snapd.SnapStatus.UNKNOWN
> +      return snap.get_status()
> +
> +  # glib-snapd does not keep track of the status of the snap. Use this decorator
> +  # to make it easy to write async functions that will always have an updated
> +  # snap object.
> +  def require_livepatch_snap(func):
> +      def get_livepatch_snap_and_call(*args, **kwargs):
> +          return args[0].get_livepatch_snap_async(lambda snap: func(snap=snap, *args, **kwargs))
> +      return get_livepatch_snap_and_call
> +
> +  def is_livepatch_enabled(self):
> +    file = Gio.File.new_for_path(path='/var/snap/canonical-livepatch/common/machine-token')
> +    return file.query_exists(None)
> +
> +  @require_livepatch_snap
> +  def set_livepatch_enabled_async(self, enabled, token, callback, snap=None):
> +      status = self.get_livepatch_snap_status(snap)
> +      if status == Snapd.SnapStatus.UNKNOWN:
> +          if callback: callback(True, _("canonical-livepatch cannot be installed"))
> +      elif status == Snapd.SnapStatus.ACTIVE:
> +          if enabled:
> +              error = self.enable_livepatch_service(token)
> +          else:
> +              error = self.disable_livepatch_service()
> +          if callback: callback(len(error) > 0, error)
> +      elif status == Snapd.SnapStatus.INSTALLED:
> +          if enabled:
> +              self.snapd_client.enable_async(name='canonical-livepatch',
> +                                             cancellable=self.cancellable,
> +                                             callback=self.livepatch_enable_snap_cb,
> +                                             user_data=(callback, token))
> +          else:
> +              if callback: callback(False, "")
> +      elif status == Snapd.SnapStatus.AVAILABLE:
> +          if enabled:
> +              self.snapd_client.install_async(name='canonical-livepatch',
> +                                              channel='edge', # Remove this once bionic is officialy supported.
> +                                              cancellable=self.cancellable,
> +                                              callback=self.livepatch_install_snap_cb,
> +                                              user_data=(callback, token))
> +          else:
> +              if callback: callback(False, "")
> +
> +  def livepatch_enable_snap_cb(self, source_object, result, user_data): 
> +      (callback, token) = user_data
> +      try:
> +          if source_object.enable_finish(result):
> +              error = self.enable_livepatch_service(token)
> +              if callback: callback(len(error) > 0, error)
> +      except Exception:
> +          if callback: callback(True, _("canonical-livepatch cannot be installed"))
> +
> +  def livepatch_install_snap_cb(self, source_object, result, user_data):
> +      (callback, token) = user_data
> +      try:
> +          if source_object.install_finish(result):
> +              error = self.enable_livepatch_service(token)
> +              if callback: callback(len(error) > 0, error)
> +      except Exception:
> +          if callback: callback(True, _("canonical-livepatch cannot be installed"))
> +
> +  def enable_livepatch_service(self, token):
> +    if self.is_livepatch_enabled():

The indenting from here on is 2 spaces instead of four, please make so that it is all 4 spaces.

> +      return ""
> +
> +    try:
> +      subprocess.check_output(['/snap/bin/canonical-livepatch', 'enable', token], stderr=subprocess.STDOUT)
> +      return ""
> +    except subprocess.CalledProcessError as e:
> +      return e.output if e.output else ""
> +
> +  def disable_livepatch_service(self):
> +    if not self.is_livepatch_enabled():
> +      return ""
> +
> +    try:
> +      subprocess.check_output(['/snap/bin/canonical-livepatch', 'disable'], stderr=subprocess.STDOUT)
> +      return ""
> +    except subprocess.CalledProcessError as e:
> +      return e.output if e.output else ""
>  
>  def shortcut_handler(shortcut):
>      for factory in _SHORTCUT_FACTORIES:
> 
> === modified file 'softwareproperties/gtk/SoftwarePropertiesGtk.py'
> --- softwareproperties/gtk/SoftwarePropertiesGtk.py	2018-03-06 15:47:27 +0000
> +++ softwareproperties/gtk/SoftwarePropertiesGtk.py	2018-03-16 13:02:28 +0000
> @@ -1446,3 +1459,138 @@
>                                                 % {'count': self.nonfree_drivers})
>          else:
>              self.label_driver_action.set_label(_("No proprietary drivers are in use."))
> +
> +    #
> +    # Livepatch
> +    #
> +    def init_livepatch(self):
> +        self.goa_auth = GoaAuth()
> +        self.waiting_livepatch_response = False
> +        self.quit_when_livepatch_responds = False
> +
> +        if not self.is_livepatch_supported():
> +            self.grid_livepatch.set_visible(False)
> +            return
> +
> +        self.checkbutton_livepatch.set_active(self.is_livepatch_enabled())
> +        self.on_goa_auth_changed()
> +
> +        # hacky way to monitor if livepatch is enabled or not
> +        file = Gio.File.new_for_path(path='/var/snap/canonical-livepatch/common/machine-token')
> +        self.lp_monitor = file.monitor_file(Gio.FileMonitorFlags.NONE)
> +
> +        # connect to signals
> +        self.handlers[self.goa_auth] = \
> +            self.goa_auth.connect('notify::logged', lambda o, p: self.on_goa_auth_changed())
> +        self.handlers[self.checkbutton_livepatch] = \
> +            self.checkbutton_livepatch.connect('toggled', self.on_checkbutton_livepatch_toggled)
> +        self.handlers[self.button_ubuntuone] =  \
> +            self.button_ubuntuone.connect('clicked', self.on_button_ubuntuone_clicked)
> +        self.handlers[self.lp_monitor] = \
> +            self.lp_monitor.connect('changed', self.on_livepatch_status_changed)
> +
> +    def is_livepatch_supported(self):
> +        supported_releases = ['trusty', 'xenial', 'bionic']

Instead of hard coding releases I think adding a dependency on and using python3-distro-info would be better, this would also remove the need for an SRU when bionic becomes supported. These may help:

di = distro_info.UbuntuDistroInfo()
di.is_lts('bionic')
di.supported(datetime.now().date())
di.devel()

> +        return any([self.distro.is_codename(d) for d in supported_releases])
> +
> +    def on_goa_auth_changed(self):
> +        if self.goa_auth.logged:
> +            self.button_ubuntuone.set_label(_('Sign Out'))
> +
> +            if self.goa_auth.token:
> +                self.checkbutton_livepatch.set_sensitive(True)
> +                self.label_livepatch_login.set_label(_('Signed in as %s' % self.goa_auth.username))
> +            else:
> +                self.checkbutton_livepatch.set_sensitive(False)
> +                text = _('%s isn\'t authorized to use Livepatch.' % self.goa_auth.username)
> +                text = "<span color='red'>" +  text + "</span>"
> +                self.label_livepatch_login.set_markup(text) 
> +        else:
> +            self.checkbutton_livepatch.set_sensitive(False)
> +            self.button_ubuntuone.set_label(_('Sign In…'))

Somewhere else "Sign In" is capitalized like "Sign in", I've no preference but think it should be consistent.

> +            self.label_livepatch_login.set_label(_('To use Livepatch you need to sign in.'))
> +
> +    def on_livepatch_status_changed(self, file_monitor, file, other_file, event_type):
> +        if not self.waiting_livepatch_response:
> +            self.checkbutton_livepatch.set_active(self.is_livepatch_enabled())
> +
> +    def on_button_ubuntuone_clicked(self, button):
> +        if self.goa_auth.logged:
> +            self.do_logout()
> +        else:
> +            self.do_login()
> +
> +    def do_login(self):
> +        try:
> +            # Show login dialog!
> +            dialog = DialogAuth(self.window_main, self.datadir)
> +            response = dialog.run()
> +        except Exception as e:
> +            logging.error(e)
> +            error(self.window_main,
> +                  _("Error enabling Canonical Livepatch"),
> +                  _("Please check your Internet connection."))
> +        else:
> +            if response == Gtk.ResponseType.OK:
> +                self.goa_auth.login(dialog.account)
> +                if self.goa_auth.logged:
> +                    self.checkbutton_livepatch.set_active(True)
> +
> +    def do_logout(self):
> +        self.goa_auth.logout()
> +        self.checkbutton_livepatch.set_active(False)
> +
> +    def on_checkbutton_livepatch_toggled(self, checkbutton):
> +        if self.waiting_livepatch_response:
> +            return
> +
> +        self.waiting_livepatch_response = True
> +
> +        if self.checkbutton_livepatch.get_active():
> +            self.backend.SetLivepatchEnabled(True, self.goa_auth.token if self.goa_auth.token else '',
> +                                             reply_handler=self.livepatch_enabled_reply_handler,
> +                                             error_handler=self.livepatch_enabled_error_handler,
> +                                             timeout=INFINIT_DBUS_TIMEOUT)
> +        else:
> +            self.backend.SetLivepatchEnabled(False, '',
> +                                             reply_handler=self.livepatch_enabled_reply_handler,
> +                                             error_handler=self.livepatch_enabled_error_handler,
> +                                             timeout=INFINIT_DBUS_TIMEOUT)
> +
> +    def livepatch_enabled_reply_handler(self, is_error, prompt):
> +        self.sync_checkbutton_livepatch(is_error, prompt)
> +
> +    def livepatch_enabled_error_handler(self, e):
> +        if e._dbus_error_name == 'com.ubuntu.SoftwareProperties.PermissionDeniedByPolicy':
> +            logging.error("Authentication canceled, changes have not been saved")
> +            self.sync_checkbutton_livepatch(is_error=True, prompt=None)
> +        else:
> +            self.sync_checkbutton_livepatch(is_error=True, prompt=str(e))
> +
> +    def sync_checkbutton_livepatch(self, is_error, prompt):
> +        if is_error:
> +            self.waiting_livepatch_response = False
> +            self.checkbutton_livepatch.handler_block(self.handlers[self.checkbutton_livepatch])
> +            self.checkbutton_livepatch.set_active(self.is_livepatch_enabled())
> +            self.checkbutton_livepatch.handler_unblock(self.handlers[self.checkbutton_livepatch])
> +
> +            if prompt:
> +                dialog = DialogLivepatchError(self.window_main, self.datadir)
> +                response = dialog.run(prompt)
> +                if response == DialogLivepatchError.RESPONSE_SETTINGS:
> +                    self.window_main.show()
> +                    self.quit_when_livepatch_responds = False
> +        else:
> +            if self.is_livepatch_enabled() and not self.checkbutton_livepatch.get_active():
> +                self.backend.SetLivepatchEnabled(False, '',
> +                                                 reply_handler=self.livepatch_enabled_reply_handler,
> +                                                 error_handler=self.livepatch_enabled_error_handler)
> +            elif not self.is_livepatch_enabled() and self.checkbutton_livepatch.get_active():
> +                self.backend.SetLivepatchEnabled(True, self.goa_auth.token if self.goa_auth.token else '',
> +                                                 reply_handler=self.livepatch_enabled_reply_handler,
> +                                                 error_handler=self.livepatch_enabled_error_handler)
> +            else:
> +                self.waiting_livepatch_response = False
> +
> +        if self.quit_when_livepatch_responds:
> +            self.on_close_button(self.button_close)


-- 
https://code.launchpad.net/~azzar1/software-properties/add-canonical-livepatch/+merge/341427
Your team Ubuntu Core Development Team is requested to review the proposed merge of lp:~azzar1/software-properties/add-canonical-livepatch into lp:software-properties.