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

Proposed by Łukasz Zemczak
Status: Merged
Merge reported by: Jean-Baptiste Lallement
Merged at revision: d5ab6da405e3acc380c9dded7ebc5d907423a449
Proposed branch: ubiquity:ubuntu-drivers-busy-indicator
Merge into: ubiquity:master
Diff against target: 332 lines (+116/-21) (has conflicts)
9 files modified
debian/changelog (+9/-0)
debian/ubiquity.templates (+4/-0)
gui/gtk/ubiquity.ui (+4/-4)
gui/qt/stepPrepare.ui (+13/-0)
tests/test_gtkui.py (+3/-0)
ubiquity/frontend/kde_components/Spinner.py (+8/-1)
ubiquity/frontend/kde_ui.py (+6/-0)
ubiquity/plugins/ubi-prepare.py (+52/-1)
ubiquity/plugins/ubi-wireless.py (+17/-15)
Conflict in debian/changelog
Reviewer Review Type Date Requested Status
Jean-Baptiste Lallement Approve
Review via email: mp+393329@code.launchpad.net

Commit message

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

Description of the change

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!

To post a comment you must log in.
Revision history for this message
Jean-Baptiste Lallement (jibel) wrote :

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
Revision history for this message
Łukasz Zemczak (sil2100) wrote :

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

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

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

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

Fix PEP8

Revision history for this message
Jean-Baptiste Lallement (jibel) wrote :

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

Thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 88c3881..175265d 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,4 @@
6+<<<<<<< debian/changelog
7 ubiquity (21.04.3) hirsute; urgency=medium
8
9 [ Mauricio Faria de Oliveira ]
10@@ -22,6 +23,14 @@ ubiquity (21.04.1) hirsute; urgency=medium
11 default (jumping by two releases as we forgot this step for groovy).
12
13 -- Łukasz 'sil2100' Zemczak <lukasz.zemczak@ubuntu.com> Mon, 16 Nov 2020 18:56:41 +0100
14+=======
15+ubiquity (21.04.1) UNRELEASED; urgency=medium
16+
17+ * Provide some minimal user feedback on the UI when ubuntu-drivers is busy
18+ while running in the background. (LP: #1824905)
19+
20+ -- Łukasz 'sil2100' Zemczak <lukasz.zemczak@ubuntu.com> Fri, 30 Oct 2020 11:40:05 +0100
21+>>>>>>> debian/changelog
22
23 ubiquity (20.10.13) groovy; urgency=medium
24
25diff --git a/debian/ubiquity.templates b/debian/ubiquity.templates
26index fef62c4..552d6d9 100644
27--- a/debian/ubiquity.templates
28+++ b/debian/ubiquity.templates
29@@ -10,6 +10,10 @@ Type: boolean
30 Default: true
31 Description: enable frame buffer console on boot
32
33+Template: ubiquity/text/preparing_ud_label
34+Type: text
35+_Description: Preparing ubuntu-drivers...
36+
37 Template: ubiquity/text/connecting_label
38 Type: text
39 _Description: Connecting...
40diff --git a/gui/gtk/ubiquity.ui b/gui/gtk/ubiquity.ui
41index ec35580..552a1e9 100644
42--- a/gui/gtk/ubiquity.ui
43+++ b/gui/gtk/ubiquity.ui
44@@ -740,12 +740,12 @@
45 <property name="visible">True</property>
46 <property name="can_focus">False</property>
47 <child>
48- <object class="GtkBox" id="hbox_connecting">
49+ <object class="GtkBox" id="hbox_status">
50 <property name="visible">True</property>
51 <property name="can_focus">False</property>
52 <property name="spacing">6</property>
53 <child>
54- <object class="GtkSpinner" id="connecting_spinner">
55+ <object class="GtkSpinner" id="status_spinner">
56 <property name="can_focus">False</property>
57 </object>
58 <packing>
59@@ -755,9 +755,9 @@
60 </packing>
61 </child>
62 <child>
63- <object class="GtkLabel" id="connecting_label">
64+ <object class="GtkLabel" id="status_label">
65 <property name="can_focus">False</property>
66- <property name="label" translatable="yes">Connecting...</property>
67+ <property name="label"></property>
68 </object>
69 <packing>
70 <property name="expand">True</property>
71diff --git a/gui/qt/stepPrepare.ui b/gui/qt/stepPrepare.ui
72index cc6827f..38cee1d 100644
73--- a/gui/qt/stepPrepare.ui
74+++ b/gui/qt/stepPrepare.ui
75@@ -321,6 +321,19 @@
76 </layout>
77 </item>
78 <item>
79+ <widget class="QWidget" name="progress_container" native="true">
80+ <property name="enabled">
81+ <bool>true</bool>
82+ </property>
83+ <property name="sizePolicy">
84+ <sizepolicy hsizetype="Expanding" vsizetype="Expanding">
85+ <horstretch>0</horstretch>
86+ <verstretch>0</verstretch>
87+ </sizepolicy>
88+ </property>
89+ </widget>
90+ </item>
91+ <item>
92 <spacer name="verticalSpacer">
93 <property name="orientation">
94 <enum>Qt::Vertical</enum>
95diff --git a/tests/test_gtkui.py b/tests/test_gtkui.py
96index 60cbfd3..8c605be 100644
97--- a/tests/test_gtkui.py
98+++ b/tests/test_gtkui.py
99@@ -141,6 +141,9 @@ class TestFrontend(unittest.TestCase):
100 'disable_secureboot', 'prepare_foss_disclaimer',
101 'label_free_space', 'label_required_space',
102 'label_download_updates',
103+ # This one is set by either the wireless and prepare plugins
104+ # to indicate status
105+ 'status_label',
106 ]
107 deb_host_arch = subprocess.Popen(
108 ['dpkg-architecture', '-qDEB_HOST_ARCH'],
109diff --git a/ubiquity/frontend/kde_components/Spinner.py b/ubiquity/frontend/kde_components/Spinner.py
110index 03e02d0..e5e9069 100644
111--- a/ubiquity/frontend/kde_components/Spinner.py
112+++ b/ubiquity/frontend/kde_components/Spinner.py
113@@ -33,7 +33,14 @@ class Spinner(QtWidgets.QLabel):
114 size = 22
115 self.setFixedSize(size, size)
116
117- pixmap = QtGui.QIcon.fromTheme('process-working').pixmap(size)
118+ # XXX: pixmap() needs specification of the size of the pixmap to be
119+ # exported and, per documentation, the resulting returned pixmap will
120+ # have dimentions smaller or equal (but never bigger) to those values.
121+ # Since this pixmap is a set of images we don't want to hard-code
122+ # the number of frames (as this comes from an external theme), so to
123+ # make sure we fit all of them we select some big number as the
124+ # height.
125+ pixmap = QtGui.QIcon.fromTheme('process-working').pixmap(size, 512)
126 self.pixes = []
127 for y in range(0, pixmap.height(), size):
128 pix = pixmap.copy(0, y, size, size)
129diff --git a/ubiquity/frontend/kde_ui.py b/ubiquity/frontend/kde_ui.py
130index b085750..2adeb26 100644
131--- a/ubiquity/frontend/kde_ui.py
132+++ b/ubiquity/frontend/kde_ui.py
133@@ -1074,6 +1074,12 @@ class Wizard(BaseFrontend):
134 return
135
136 self.allow_change_step(False)
137+ ui = self.pages[self.pagesindex].ui
138+ if hasattr(ui, 'plugin_on_next_clicked'):
139+ if ui.plugin_on_next_clicked():
140+ # Stop processing and return to the page.
141+ self.allow_change_step(True)
142+ return
143
144 if self.dbfilter is not None:
145 self.dbfilter.ok_handler()
146diff --git a/ubiquity/plugins/ubi-prepare.py b/ubiquity/plugins/ubi-prepare.py
147index 8702646..8339803 100644
148--- a/ubiquity/plugins/ubi-prepare.py
149+++ b/ubiquity/plugins/ubi-prepare.py
150@@ -73,6 +73,12 @@ class PreparePageBase(plugin.PluginUI):
151 self.rst_title_text = i18n.get_string('rst_header', lang)
152 return
153
154+ def show_ubuntu_drivers_spinner(self):
155+ return
156+
157+ def hide_ubuntu_drivers_spinner(self):
158+ return
159+
160
161 class PageGtk(PreparePageBase):
162 restricted_package_name = 'ubuntu-restricted-addons'
163@@ -153,8 +159,22 @@ class PageGtk(PreparePageBase):
164 self.controller.go_to_page(self.current_page)
165 return True
166
167+ def show_ubuntu_drivers_spinner(self):
168+ frontend = self.controller._wizard
169+ frontend.status_spinner.show()
170+ frontend.status_spinner.start()
171+ frontend.status_label.show()
172+
173+ def hide_ubuntu_drivers_spinner(self):
174+ frontend = self.controller._wizard
175+ frontend.status_spinner.hide()
176+ frontend.status_spinner.stop()
177+ frontend.status_label.hide()
178+
179 def plugin_on_next_clicked(self):
180 if self.current_page != self.rst_page:
181+ if self.get_use_nonfree():
182+ self.show_ubuntu_drivers_spinner()
183 return
184
185 self.controller._wizard.do_reboot()
186@@ -208,6 +228,11 @@ class PageGtk(PreparePageBase):
187
188 def plugin_translate(self, lang):
189 PreparePageBase.plugin_translate(self, lang)
190+
191+ frontend = self.controller._wizard
192+ frontend.status_label.set_text(self.controller.get_string(
193+ 'ubiquity/text/preparing_ud_label', lang))
194+
195 release = misc.get_release()
196
197 from gi.repository import Gtk
198@@ -298,7 +323,10 @@ class PageKde(PreparePageBase):
199 self.controller = controller
200 try:
201 from PyQt5 import uic
202- from PyQt5 import QtGui
203+ from PyQt5 import QtGui, QtWidgets
204+ # No worries, this has nothing to do with NM, we just want the
205+ # generic progress indicator from there
206+ from ubiquity.frontend.kde_components.nmwidgets import ProgressIndicator
207 self.page = uic.loadUi('/usr/share/ubiquity/qt/stepPrepare.ui')
208 self.prepare_minimal_install = self.page.prepare_minimal_install
209 self.qt_label_minimal_install = self.page.qt_label_minimal_install
210@@ -314,6 +342,10 @@ class PageKde(PreparePageBase):
211 self.badPassword = self.page.badPassword
212 self.badPassword.setPixmap(QtGui.QPixmap(
213 "/usr/share/icons/oxygen/16x16/status/dialog-warning.png"))
214+ self.progress_indicator = ProgressIndicator()
215+ self.progress_indicator.hide()
216+ layout = QtWidgets.QHBoxLayout(self.page.progress_container)
217+ layout.addWidget(self.progress_indicator)
218 # TODO we should set these up and tear them down while on this
219 # page.
220 try:
221@@ -342,6 +374,18 @@ class PageKde(PreparePageBase):
222 def show_rst_page(self):
223 return False
224
225+ def show_ubuntu_drivers_spinner(self):
226+ self.progress_indicator.show()
227+ self.progress_indicator.setSpinnerVisible(True)
228+
229+ def hide_ubuntu_drivers_spinner(self):
230+ self.progress_indicator.setSpinnerVisible(False)
231+ self.progress_indicator.hide()
232+
233+ def plugin_on_next_clicked(self):
234+ if self.get_use_nonfree():
235+ self.show_ubuntu_drivers_spinner()
236+
237 def show_insufficient_space_page(self, required, free):
238 from PyQt5 import QtWidgets
239 QtWidgets.QMessageBox.critical(self.page,
240@@ -417,6 +461,9 @@ class PageKde(PreparePageBase):
241
242 def plugin_translate(self, lang):
243 PreparePageBase.plugin_translate(self, lang)
244+ # Translate the progress label
245+ self.progress_indicator.setText(self.controller.get_string(
246+ 'ubiquity/text/preparing_ud_label', lang))
247 # gtk does the ${RELEASE} replace for the title in gtk_ui but we do
248 # it per plugin because our title widget is per plugin
249 release = misc.get_release()
250@@ -543,3 +590,7 @@ class Page(plugin.Plugin):
251 'ubiquity/nonfree_package',
252 self.ui.restricted_package_name)
253 plugin.Plugin.ok_handler(self)
254+
255+ def cleanup(self):
256+ self.ui.hide_ubuntu_drivers_spinner()
257+ plugin.Plugin.cleanup(self)
258diff --git a/ubiquity/plugins/ubi-wireless.py b/ubiquity/plugins/ubi-wireless.py
259index b3045cd..b2d2ed8 100644
260--- a/ubiquity/plugins/ubi-wireless.py
261+++ b/ubiquity/plugins/ubi-wireless.py
262@@ -105,6 +105,8 @@ class PageGtk(WirelessPageBase):
263 frontend.next.set_label(self.connect_text)
264 if not self.back_normal:
265 frontend.back.set_label(self.stop_text)
266+ frontend.status_label.set_text(self.controller.get_string(
267+ 'ubiquity/text/connecting_label', lang))
268
269 def selection_changed(self, unused):
270 from ubiquity import nm
271@@ -132,9 +134,9 @@ class PageGtk(WirelessPageBase):
272 self.nmwidget.select_usable_row()
273 self.state_changed(None, self.state)
274 else:
275- frontend.connecting_spinner.hide()
276- frontend.connecting_spinner.stop()
277- frontend.connecting_label.hide()
278+ frontend.status_spinner.hide()
279+ frontend.status_spinner.stop()
280+ frontend.status_label.hide()
281 frontend.translate_widget(frontend.next)
282 self.next_normal = True
283 self.controller.allow_go_forward(True)
284@@ -145,9 +147,9 @@ class PageGtk(WirelessPageBase):
285 self.nmwidget.disconnect_from_ap()
286 return True
287 else:
288- frontend.connecting_spinner.hide()
289- frontend.connecting_spinner.stop()
290- frontend.connecting_label.hide()
291+ frontend.status_spinner.hide()
292+ frontend.status_spinner.stop()
293+ frontend.status_label.hide()
294 self.no_wireless.set_active(True)
295 return False
296
297@@ -157,9 +159,9 @@ class PageGtk(WirelessPageBase):
298 self.nmwidget.connect_to_ap()
299 return True
300 else:
301- frontend.connecting_spinner.hide()
302- frontend.connecting_spinner.stop()
303- frontend.connecting_label.hide()
304+ frontend.status_spinner.hide()
305+ frontend.status_spinner.stop()
306+ frontend.status_label.hide()
307 return False
308
309 def state_changed(self, unused, state):
310@@ -170,17 +172,17 @@ class PageGtk(WirelessPageBase):
311 if not self.use_wireless.get_active():
312 return
313 if state != nm.NM_STATE_CONNECTING:
314- frontend.connecting_spinner.hide()
315- frontend.connecting_spinner.stop()
316- frontend.connecting_label.hide()
317+ frontend.status_spinner.hide()
318+ frontend.status_spinner.stop()
319+ frontend.status_label.hide()
320
321 frontend.translate_widget(frontend.back)
322 self.back_normal = True
323 frontend.back.set_sensitive(True)
324 else:
325- frontend.connecting_spinner.show()
326- frontend.connecting_spinner.start()
327- frontend.connecting_label.show()
328+ frontend.status_spinner.show()
329+ frontend.status_spinner.start()
330+ frontend.status_label.show()
331
332 self.next_normal = True
333

Subscribers

People subscribed via source and target branches