Merge ubiquity:ubuntu-drivers-busy-indicator into ubiquity:master

[Łukasz Zemczak]

Provide some minimal user feedback on the UI when ubuntu-drivers is busy while running in the background.

Provide some minimal user feedback on the UI when ubuntu-drivers is busy while running in the background.

First of all: please forgive me if some of this UI-related code is not of the highest quality - it's been a while + it's the first time I'm touching anything GUI related in ubiquity.

For the busy-indication I have used:
 * For GTK: the main widget always had a hidden "Connecting..." indicator we are using when connecting to a wireless network. I wanted to re-use that UI element, so now it's used for both indicating connection and ubuntu-drivers processing.
 * For KDE: the wireless connecting status is done a bit differently, but I also re-used some parts. Had to add a new widget to the prepare screen, but re-using the nmwidgets ProgressIndicator. So the spinner appears now in the middle below all the user selections, which seems to look nice in my eyes.

Feedback appreciated!

[Jean-Baptiste Lallement (jibel)]

Thanks for the patch. It looks good, however on Kubuntu there is a small graphical artefact on the left of the label, a small darker grey line https://imgur.com/a/CVh9QzC

Can you see if you can remove it please?

review: Needs Fixing

[Łukasz Zemczak (sil2100)]

Oh, hmm! Darn, this is a bug that I saw previously and thought I fixed it - this is actually the spinner, but when I'm enabling it wrong it just draws all the frames one after another (which looks like a line). Let me check what's up with that! Thanks!

288cdf9... by Łukasz Zemczak on 2020-12-09

The Spinner() class was busted, probably something changed in Qt recently.

[Łukasz Zemczak (sil2100)]

Ok, found the issue. I have no idea how this worked for me at one point. Apparently the Spinner object that I was using here didn't quite work (so for the wireless screen as well!) - as per the docs the pixmap() call takes a size argument, either N x N for one argument or N x M if two. In this case they were using the 'size' variable, which means the pixmap was allocated to be of maximum size x size dimensions. And this pixmap is actually a set of frames for the spinner animation.

Anyway, now it should all work. Please re-check!

d5ab6da... by Łukasz Zemczak on 2020-12-09

Fix PEP8

[Jean-Baptiste Lallement (jibel)]

LGTM. tested on hirsute desktop ubuntu & kubuntu. builds properly once rebased.

Thanks!

review: Approve

diff --git a/debian/changelog b/debian/changelog
index 88c3881..175265d 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,4 @@
+<<<<<<< debian/changelog
 ubiquity (21.04.3) hirsute; urgency=medium
 
   [ Mauricio Faria de Oliveira ]
@@ -22,6 +23,14 @@ ubiquity (21.04.1) hirsute; urgency=medium
     default (jumping by two releases as we forgot this step for groovy).
 
  -- Łukasz 'sil2100' Zemczak <[e-mail]>  Mon, 16 Nov 2020 18:56:41 +0100
+=======
+ubiquity (21.04.1) UNRELEASED; urgency=medium
+
+  * Provide some minimal user feedback on the UI when ubuntu-drivers is busy
+    while running in the background. (LP: #1824905)
+
+ -- Łukasz 'sil2100' Zemczak <[e-mail]>  Fri, 30 Oct 2020 11:40:05 +0100
+>>>>>>> debian/changelog
 
 ubiquity (20.10.13) groovy; urgency=medium
 
diff --git a/debian/ubiquity.templates b/debian/ubiquity.templates
index fef62c4..552d6d9 100644
--- a/debian/ubiquity.templates
+++ b/debian/ubiquity.templates
@@ -10,6 +10,10 @@ Type: boolean
 Default: true
 Description: enable frame buffer console on boot
 
+Template: ubiquity/text/preparing_ud_label
+Type: text
+_Description: Preparing ubuntu-drivers...
+
 Template: ubiquity/text/connecting_label
 Type: text
 _Description: Connecting...
diff --git a/gui/gtk/ubiquity.ui b/gui/gtk/ubiquity.ui
index ec35580..552a1e9 100644
--- a/gui/gtk/ubiquity.ui
+++ b/gui/gtk/ubiquity.ui
@@ -740,12 +740,12 @@
                                     <property name="visible">True</property>
                                     <property name="can_focus">False</property>
                                     <child>
-                                      <object class="GtkBox" id="hbox_connecting">
+                                      <object class="GtkBox" id="hbox_status">
                                         <property name="visible">True</property>
                                         <property name="can_focus">False</property>
                                         <property name="spacing">6</property>
                                         <child>
-                                          <object class="GtkSpinner" id="connecting_spinner">
+                                          <object class="GtkSpinner" id="status_spinner">
                                             <property name="can_focus">False</property>
                                           </object>
                                           <packing>
@@ -755,9 +755,9 @@
                                           </packing>
                                         </child>
                                         <child>
-                                          <object class="GtkLabel" id="connecting_label">
+                                          <object class="GtkLabel" id="status_label">
                                             <property name="can_focus">False</property>
-                                            <property name="label" translatable="yes">Connecting...</property>
+                                            <property name="label"></property>
                                           </object>
                                           <packing>
                                             <property name="expand">True</property>
diff --git a/gui/qt/stepPrepare.ui b/gui/qt/stepPrepare.ui
index cc6827f..38cee1d 100644
--- a/gui/qt/stepPrepare.ui
+++ b/gui/qt/stepPrepare.ui
@@ -321,6 +321,19 @@
     </layout>
    </item>
    <item>
+    <widget class="QWidget" name="progress_container" native="true">
+     <property name="enabled">
+      <bool>true</bool>
+     </property>
+     <property name="sizePolicy">
+      <sizepolicy hsizetype="Expanding" vsizetype="Expanding">
+       <horstretch>0</horstretch>
+       <verstretch>0</verstretch>
+      </sizepolicy>
+     </property>
+    </widget>
+   </item>
+   <item>
     <spacer name="verticalSpacer">
      <property name="orientation">
       <enum>Qt::Vertical</enum>
diff --git a/tests/test_gtkui.py b/tests/test_gtkui.py
index 60cbfd3..8c605be 100644
--- a/tests/test_gtkui.py
+++ b/tests/test_gtkui.py
@@ -141,6 +141,9 @@ class TestFrontend(unittest.TestCase):
                 'disable_secureboot', 'prepare_foss_disclaimer',
                 'label_free_space', 'label_required_space',
                 'label_download_updates',
+                # This one is set by either the wireless and prepare plugins
+                # to indicate status
+                'status_label',
             ]
             deb_host_arch = subprocess.Popen(
                 ['dpkg-architecture', '-qDEB_HOST_ARCH'],
diff --git a/ubiquity/frontend/kde_components/Spinner.py b/ubiquity/frontend/kde_components/Spinner.py
index 03e02d0..e5e9069 100644
--- a/ubiquity/frontend/kde_components/Spinner.py
+++ b/ubiquity/frontend/kde_components/Spinner.py
@@ -33,7 +33,14 @@ class Spinner(QtWidgets.QLabel):
         size = 22
         self.setFixedSize(size, size)
 
-        pixmap = QtGui.QIcon.fromTheme('process-working').pixmap(size)
+        # XXX: pixmap() needs specification of the size of the pixmap to be
+        # exported and, per documentation, the resulting returned pixmap will
+        # have dimentions smaller or equal (but never bigger) to those values.
+        # Since this pixmap is a set of images we don't want to hard-code
+        # the number of frames (as this comes from an external theme), so to
+        # make sure we fit all of them we select some big number as the
+        # height.
+        pixmap = QtGui.QIcon.fromTheme('process-working').pixmap(size, 512)
         self.pixes = []
         for y in range(0, pixmap.height(), size):
             pix = pixmap.copy(0, y, size, size)
diff --git a/ubiquity/frontend/kde_ui.py b/ubiquity/frontend/kde_ui.py
index b085750..2adeb26 100644
--- a/ubiquity/frontend/kde_ui.py
+++ b/ubiquity/frontend/kde_ui.py
@@ -1074,6 +1074,12 @@ class Wizard(BaseFrontend):
             return
 
         self.allow_change_step(False)
+        ui = self.pages[self.pagesindex].ui
+        if hasattr(ui, 'plugin_on_next_clicked'):
+            if ui.plugin_on_next_clicked():
+                # Stop processing and return to the page.
+                self.allow_change_step(True)
+                return
 
         if self.dbfilter is not None:
             self.dbfilter.ok_handler()
diff --git a/ubiquity/plugins/ubi-prepare.py b/ubiquity/plugins/ubi-prepare.py
index 8702646..8339803 100644
--- a/ubiquity/plugins/ubi-prepare.py
+++ b/ubiquity/plugins/ubi-prepare.py
@@ -73,6 +73,12 @@ class PreparePageBase(plugin.PluginUI):
         self.rst_title_text = i18n.get_string('rst_header', lang)
         return
 
+    def show_ubuntu_drivers_spinner(self):
+        return
+
+    def hide_ubuntu_drivers_spinner(self):
+        return
+
 
 class PageGtk(PreparePageBase):
     restricted_package_name = 'ubuntu-restricted-addons'
@@ -153,8 +159,22 @@ class PageGtk(PreparePageBase):
         self.controller.go_to_page(self.current_page)
         return True
 
+    def show_ubuntu_drivers_spinner(self):
+        frontend = self.controller._wizard
+        frontend.status_spinner.show()
+        frontend.status_spinner.start()
+        frontend.status_label.show()
+
+    def hide_ubuntu_drivers_spinner(self):
+        frontend = self.controller._wizard
+        frontend.status_spinner.hide()
+        frontend.status_spinner.stop()
+        frontend.status_label.hide()
+
     def plugin_on_next_clicked(self):
         if self.current_page != self.rst_page:
+            if self.get_use_nonfree():
+                self.show_ubuntu_drivers_spinner()
             return
 
         self.controller._wizard.do_reboot()
@@ -208,6 +228,11 @@ class PageGtk(PreparePageBase):
 
     def plugin_translate(self, lang):
         PreparePageBase.plugin_translate(self, lang)
+
+        frontend = self.controller._wizard
+        frontend.status_label.set_text(self.controller.get_string(
+            'ubiquity/text/preparing_ud_label', lang))
+
         release = misc.get_release()
 
         from gi.repository import Gtk
@@ -298,7 +323,10 @@ class PageKde(PreparePageBase):
         self.controller = controller
         try:
             from PyQt5 import uic
-            from PyQt5 import QtGui
+            from PyQt5 import QtGui, QtWidgets
+            # No worries, this has nothing to do with NM, we just want the
+            # generic progress indicator from there
+            from ubiquity.frontend.kde_components.nmwidgets import ProgressIndicator
             self.page = uic.loadUi('/usr/share/ubiquity/qt/stepPrepare.ui')
             self.prepare_minimal_install = self.page.prepare_minimal_install
             self.qt_label_minimal_install = self.page.qt_label_minimal_install
@@ -314,6 +342,10 @@ class PageKde(PreparePageBase):
             self.badPassword = self.page.badPassword
             self.badPassword.setPixmap(QtGui.QPixmap(
                 "/usr/share/icons/oxygen/16x16/status/dialog-warning.png"))
+            self.progress_indicator = ProgressIndicator()
+            self.progress_indicator.hide()
+            layout = QtWidgets.QHBoxLayout(self.page.progress_container)
+            layout.addWidget(self.progress_indicator)
             # TODO we should set these up and tear them down while on this
             # page.
             try:
@@ -342,6 +374,18 @@ class PageKde(PreparePageBase):
     def show_rst_page(self):
         return False
 
+    def show_ubuntu_drivers_spinner(self):
+        self.progress_indicator.show()
+        self.progress_indicator.setSpinnerVisible(True)
+
+    def hide_ubuntu_drivers_spinner(self):
+        self.progress_indicator.setSpinnerVisible(False)
+        self.progress_indicator.hide()
+
+    def plugin_on_next_clicked(self):
+        if self.get_use_nonfree():
+            self.show_ubuntu_drivers_spinner()
+
     def show_insufficient_space_page(self, required, free):
         from PyQt5 import QtWidgets
         QtWidgets.QMessageBox.critical(self.page,
@@ -417,6 +461,9 @@ class PageKde(PreparePageBase):
 
     def plugin_translate(self, lang):
         PreparePageBase.plugin_translate(self, lang)
+        # Translate the progress label
+        self.progress_indicator.setText(self.controller.get_string(
+            'ubiquity/text/preparing_ud_label', lang))
         # gtk does the ${RELEASE} replace for the title in gtk_ui but we do
         # it per plugin because our title widget is per plugin
         release = misc.get_release()
@@ -543,3 +590,7 @@ class Page(plugin.Plugin):
                         'ubiquity/nonfree_package',
                         self.ui.restricted_package_name)
         plugin.Plugin.ok_handler(self)
+
+    def cleanup(self):
+        self.ui.hide_ubuntu_drivers_spinner()
+        plugin.Plugin.cleanup(self)
diff --git a/ubiquity/plugins/ubi-wireless.py b/ubiquity/plugins/ubi-wireless.py
index b3045cd..b2d2ed8 100644
--- a/ubiquity/plugins/ubi-wireless.py
+++ b/ubiquity/plugins/ubi-wireless.py
@@ -105,6 +105,8 @@ class PageGtk(WirelessPageBase):
             frontend.next.set_label(self.connect_text)
         if not self.back_normal:
             frontend.back.set_label(self.stop_text)
+        frontend.status_label.set_text(self.controller.get_string(
+            'ubiquity/text/connecting_label', lang))
 
     def selection_changed(self, unused):
         from ubiquity import nm
@@ -132,9 +134,9 @@ class PageGtk(WirelessPageBase):
                 self.nmwidget.select_usable_row()
             self.state_changed(None, self.state)
         else:
-            frontend.connecting_spinner.hide()
-            frontend.connecting_spinner.stop()
-            frontend.connecting_label.hide()
+            frontend.status_spinner.hide()
+            frontend.status_spinner.stop()
+            frontend.status_label.hide()
             frontend.translate_widget(frontend.next)
             self.next_normal = True
             self.controller.allow_go_forward(True)
@@ -145,9 +147,9 @@ class PageGtk(WirelessPageBase):
             self.nmwidget.disconnect_from_ap()
             return True
         else:
-            frontend.connecting_spinner.hide()
-            frontend.connecting_spinner.stop()
-            frontend.connecting_label.hide()
+            frontend.status_spinner.hide()
+            frontend.status_spinner.stop()
+            frontend.status_label.hide()
             self.no_wireless.set_active(True)
             return False
 
@@ -157,9 +159,9 @@ class PageGtk(WirelessPageBase):
             self.nmwidget.connect_to_ap()
             return True
         else:
-            frontend.connecting_spinner.hide()
-            frontend.connecting_spinner.stop()
-            frontend.connecting_label.hide()
+            frontend.status_spinner.hide()
+            frontend.status_spinner.stop()
+            frontend.status_label.hide()
             return False
 
     def state_changed(self, unused, state):
@@ -170,17 +172,17 @@ class PageGtk(WirelessPageBase):
         if not self.use_wireless.get_active():
             return
         if state != nm.NM_STATE_CONNECTING:
-            frontend.connecting_spinner.hide()
-            frontend.connecting_spinner.stop()
-            frontend.connecting_label.hide()
+            frontend.status_spinner.hide()
+            frontend.status_spinner.stop()
+            frontend.status_label.hide()
 
             frontend.translate_widget(frontend.back)
             self.back_normal = True
             frontend.back.set_sensitive(True)
         else:
-            frontend.connecting_spinner.show()
-            frontend.connecting_spinner.start()
-            frontend.connecting_label.show()
+            frontend.status_spinner.show()
+            frontend.status_spinner.start()
+            frontend.status_label.show()
 
             self.next_normal = True