[Merge] ~nteodosio/software-properties:drivers-hang into software-properties:ubuntu/master

[Marco Trevisan (Treviño)]

Diff comments:

> diff --git a/softwareproperties/gtk/SoftwarePropertiesGtk.py b/softwareproperties/gtk/SoftwarePropertiesGtk.py
> index 0cced00..251f7b4 100644
> --- a/softwareproperties/gtk/SoftwarePropertiesGtk.py
> +++ b/softwareproperties/gtk/SoftwarePropertiesGtk.py
> @@ -283,7 +306,10 @@ class SoftwarePropertiesGtk(SoftwareProperties, SimpleGtkbuilderApp):
>          if page == self.vbox_drivers:
>              self.button_revert.set_visible(False)
>              if not self.detect_called:
> -                GLib.idle_add(lambda: threading.Thread(target=self.detect_drivers).start())
> +                t = threading.Thread(target=self.detect_drivers)
> +                t.daemon = True
> +                self.label_driver_detail.set_text(_("Searching for available drivers..."))
> +                GLib.idle_add(lambda: t.start())

Do we have a reason to delay this?

In general it would be nice to track all the idle's and timeouts, so that they get removed on destruction.

>          else:
>              self.button_revert.set_visible(True)
>  
> @@ -1403,22 +1423,61 @@ class SoftwarePropertiesGtk(SoftwareProperties, SimpleGtkbuilderApp):
>          self.nonfree_drivers = 0
>          self.ui_building = False
>  
> +    def on_detect_drivers_cancel(self, button_cancel):
> +        self.search_process.terminate()
> +        button_cancel.set_sensitive(False)
> +        self.button_detect_drivers_retry.set_sensitive(True)
> +
> +    def on_detect_drivers_retry(self, button_retry):
> +        button_retry.set_sensitive(False)
> +        self.button_detect_drivers_cancel.set_sensitive(True)
> +        self.detect_called = False
> +        self.on_main_notebook_page_switched(None, self.vbox_drivers, None)

Yeah, better to add an utility function if called directly

> +
> +    def wrapper_system_device_drivers(self, queue, cache):
> +        devices = detect.system_device_drivers(cache)
> +        queue.put_nowait(devices)
> +
>      def detect_drivers(self):
>          # WARNING: This is run in a separate thread.
>          self.detect_called = True
> +        queue = multiprocessing.Queue()
> +        self.search_process = Process(
> +            target=self.wrapper_system_device_drivers,
> +            args=(queue, self.apt_cache,)
> +        )
> +        self.search_process.daemon = True
> +        self.search_process.start()
> +        error = None
>          try:
> -            self.devices = detect.system_device_drivers(self.apt_cache)
> -        except:
> +            self.search_process.join()
> +            if self.search_process.exception:
> +                raise(self.search_process.exception)
> +            if self.search_process.exitcode == 0:
> +                self.devices = queue.get_nowait()
> +            elif self.search_process.exitcode == -15:
> +                error = _("The search for drivers was cancelled.")

I feel we should just reset to the default state here, so keeping it blank and having a retry button should be enough.

> +            else:
> +                error = _("An error occurred while searching for drivers.") + \
> +                    _(" Exit status: ") + str(self.search_process.exitcode) + "."

I was forgetting here.

We should not split the translation strings, and use replaceable elements, so use something like:

_("An error occurred while searching for drivers. Exit status was %d.") \
  % self.search_process.exitcode

> +        except Exception as e:
>              # Catch all exceptions and feed them to apport.
> -            GLib.idle_add(self.label_driver_detail.set_text,
> -                          _("An error occurred while searching for drivers."))
> +            error = _("An error occurred while searching for drivers: %s") % e
>  
>              # For apport to catch this exception. See
>              # http://bugs.python.org/issue1230540
>              sys.excepthook(*sys.exc_info())
> +
> +        GLib.idle_add(lambda: self.on_detect_drivers_done(error))

Let's add a comment for future people touching this:
# Using an Idle to ensure the function is called in the main (UI) thread

> +
> +    def on_detect_drivers_done(self, err=None):
> +        if err is None:
> +            self.show_drivers()
>              return
>  
> -        GLib.idle_add(self.show_drivers)
> +        self.label_driver_detail.set_text(err)
> +        self.button_detect_drivers_retry.set_sensitive(True)
> +        self.button_detect_drivers_cancel.set_sensitive(False)
>  
>      def on_driver_selection_changed(self, button, modalias, pkg_name=None):
>          if self.ui_building:


-- 
https://code.launchpad.net/~nteodosio/software-properties/+git/software-properties/+merge/470812
Your team Ubuntu Core Development Team is requested to review the proposed merge of ~nteodosio/software-properties:drivers-hang into software-properties:ubuntu/master.