Merge ~jibel/ubiquity:advanced_features_dialog into ubiquity:master

Proposed by Jean-Baptiste Lallement on 2019-10-30
Status: Merged
Merged at revision: 5a2667c9678e03f9a154cee3306b5204adb2a8d6
Proposed branch: ~jibel/ubiquity:advanced_features_dialog
Merge into: ubiquity:master
Diff against target: 561 lines (+308/-121)
6 files modified
debian/changelog (+2/-0)
debian/ubiquity.templates (+32/-0)
gui/gtk/stepPartAsk.ui (+211/-106)
tests/run-pep8 (+1/-0)
tests/test_gtkui.py (+3/-0)
ubiquity/plugins/ubi-partman.py (+59/-15)
Reviewer Review Type Date Requested Status
Didier Roche (community) 2019-10-30 Approve on 2019-11-05
Steve Langasek 2019-10-30 Needs Information on 2019-10-31
Review via email: mp+374920@code.launchpad.net

Commit message

New design for advanced features

A new dialog has been added to the guided partitioning page for advanced features. LVM, encrypted LVM and ZFS have been moved to this new dialog.

Specification:
https://docs.google.com/document/d/1bZ4yQIVgGaUGSYu3qiUHnQt3ieBZoqunP_DcleHCr3I/preview#heading=h.9wow0hy0pim7

To post a comment you must log in.
Steve Langasek (vorlon) :
review: Needs Information
Didier Roche (didrocks) wrote :

I have nothing to add on the code changes themselves.

I have a done some tests with current 19.10 image + this branch on top of it. Apart from the selected translations still in english (but I see them in the template below and it makes sense to update the translations in a separate commits as discussed on IRC before the review), I didn't spot any major issues compared to the linked design.

Next/Back works as expected, even if going to the custom screen selection and back then (previous option still selected). However (and I checked that this isn't a regression from that MP), once pressing back from the custom screen, "Entire disk" is selected instead of "Something else". The previous advanced feature selection is still remembered though.

Just a note: there are some missing margins on the Cancel/OK buttons in the dialog compared to the spec.

Jean-Baptiste Lallement (jibel) wrote :

@didrocks thanks for the review.

I'll have a look at the next/back behaviour separately.
For the margins, they are the same than on the advanced partitioning dialog boxes but different than the partman confirmation dialog boxes for instance. I'll leave it like this and align all the margins in a separate MP.

for reference https://imgur.com/Ai6wKV4

Didier Roche (didrocks) wrote :

with those additional info + a chat on IRC, I'm fine with the testing and it seems to align with ubiquity code style and mpt design.

+1 on my side. Just think about the followup commit to refresh templates. 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 5d1a3dd..8a152ab 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,6 +1,8 @@
6 ubiquity (20.04.1) UNRELEASED; urgency=medium
7
8 * script/zsys-setup: fix tag name from org.zsys to com.ubuntu.zsys.
9+ * Added an advanced features dialog to the partitioning page. LVM, LVM with
10+ encryption and experimental ZFS support have been moved to this dialog.
11
12 -- Jean-Baptiste Lallement <jean-baptiste.lallement@ubuntu.com> Mon, 28 Oct 2019 09:07:57 +0100
13
14diff --git a/debian/ubiquity.templates b/debian/ubiquity.templates
15index 57c5ca3..f56ac69 100644
16--- a/debian/ubiquity.templates
17+++ b/debian/ubiquity.templates
18@@ -1449,6 +1449,38 @@ Default: false
19 Description: for internal use; can be preseeded
20 This will set up ZFS and configure it for zsys.
21
22+Template: ubiquity/text/advanced_features_dialog
23+Type: text
24+_Description: Advanced Features
25+
26+Template: ubiquity/text/advanced_features_radio_none
27+Type: text
28+_Description: None
29+
30+Template: ubiquity/text/advanced_features_desc
31+Type: text
32+_Description: None selected
33+
34+Template: ubiquity/text/advanced_features_selected
35+Type: text
36+_Description: Advanced features...
37+
38+Template: ubiquity/text/advanced_features_none_selected
39+Type: text
40+_Description: None selected
41+
42+Template: ubiquity/text/advanced_features_lvm_selected
43+Type: text
44+_Description: LVM selected
45+
46+Template: ubiquity/text/advanced_features_crypto_selected
47+Type: text
48+_Description: LVM and encryption selected
49+
50+Template: ubiquity/text/advanced_features_zfs_selected
51+Type: text
52+_Description: ZFS selected
53+
54 Template: ubiquity/text/verified_crypto_label
55 Type: text
56 _Description:
57diff --git a/gui/gtk/stepPartAsk.ui b/gui/gtk/stepPartAsk.ui
58index bb89fc6..f734558 100644
59--- a/gui/gtk/stepPartAsk.ui
60+++ b/gui/gtk/stepPartAsk.ui
61@@ -1,6 +1,169 @@
62 <?xml version="1.0" encoding="UTF-8"?>
63 <interface>
64 <!-- interface-requires gtk+ 3.0 -->
65+ <object class="GtkDialog" id="advanced_features_dialog">
66+ <property name="can_focus">False</property>
67+ <property name="title" translatable="yes">Advanced Features</property>
68+ <property name="resizable">False</property>
69+ <property name="modal">True</property>
70+ <property name="window_position">center-on-parent</property>
71+ <property name="type_hint">dialog</property>
72+ <child internal-child="vbox">
73+ <object class="GtkBox" id="advanced_features_box1">
74+ <property name="can_focus">False</property>
75+ <property name="orientation">vertical</property>
76+ <property name="spacing">24</property>
77+ <child internal-child="action_area">
78+ <object class="GtkButtonBox" id="advanced_features_buttonbox">
79+ <property name="can_focus">False</property>
80+ <property name="layout_style">end</property>
81+ <child>
82+ <object class="GtkButton" id="advanced_features_cancelbutton">
83+ <property name="label">_Cancel</property>
84+ <property name="use_action_appearance">False</property>
85+ <property name="visible">True</property>
86+ <property name="can_focus">True</property>
87+ <property name="can_default">True</property>
88+ <property name="receives_default">True</property>
89+ <property name="use_underline">True</property>
90+ </object>
91+ <packing>
92+ <property name="expand">False</property>
93+ <property name="fill">False</property>
94+ <property name="position">0</property>
95+ </packing>
96+ </child>
97+ <child>
98+ <object class="GtkButton" id="advanced_features_okbutton">
99+ <property name="label">_OK</property>
100+ <property name="use_action_appearance">False</property>
101+ <property name="visible">True</property>
102+ <property name="can_focus">True</property>
103+ <property name="can_default">True</property>
104+ <property name="has_default">True</property>
105+ <property name="receives_default">True</property>
106+ <property name="use_underline">True</property>
107+ </object>
108+ <packing>
109+ <property name="expand">False</property>
110+ <property name="fill">False</property>
111+ <property name="position">1</property>
112+ </packing>
113+ </child>
114+ </object>
115+ <packing>
116+ <property name="expand">False</property>
117+ <property name="fill">False</property>
118+ <property name="position">0</property>
119+ </packing>
120+ </child>
121+ <child>
122+ <object class="GtkBox" id="advanced_features_box2">
123+ <property name="visible">True</property>
124+ <property name="can_focus">False</property>
125+ <property name="orientation">vertical</property>
126+ <child>
127+ <object class="GtkRadioButton" id="advanced_features_radio_none">
128+ <property name="label" translatable="yes">None</property>
129+ <property name="visible">True</property>
130+ <property name="can_focus">True</property>
131+ <property name="receives_default">True</property>
132+ <property name="active">True</property>
133+ <property name="draw_indicator">True</property>
134+ <property name="margin_top">9</property>
135+ <signal name="clicked" handler="advanced_features_option_changed" swapped="no"/>
136+ </object>
137+ <packing>
138+ <property name="expand">False</property>
139+ <property name="fill">True</property>
140+ <property name="position">0</property>
141+ </packing>
142+ </child>
143+ <child>
144+ <object class="GtkRadioButton" id="use_lvm">
145+ <property name="label" translatable="yes">Set up the system as an LVM volume group</property>
146+ <property name="visible">True</property>
147+ <property name="can_focus">True</property>
148+ <property name="receives_default">False</property>
149+ <property name="active">True</property>
150+ <property name="draw_indicator">True</property>
151+ <property name="margin_top">9</property>
152+ <property name="group">advanced_features_radio_none</property>
153+ <signal name="clicked" handler="advanced_features_option_changed" swapped="no"/>
154+ </object>
155+ <packing>
156+ <property name="expand">False</property>
157+ <property name="fill">True</property>
158+ <property name="position">1</property>
159+ </packing>
160+ </child>
161+ <child>
162+ <object class="GtkCheckButton" id="use_crypto">
163+ <property name="label" translatable="yes">Encrypt the volume group for security</property>
164+ <property name="visible">True</property>
165+ <property name="sensitive">False</property>
166+ <property name="can_focus">True</property>
167+ <property name="receives_default">False</property>
168+ <property name="draw_indicator">True</property>
169+ <property name="margin_left">24</property>
170+ </object>
171+ <packing>
172+ <property name="expand">False</property>
173+ <property name="fill">True</property>
174+ <property name="position">2</property>
175+ </packing>
176+ </child>
177+ <child>
178+ <object class="GtkLabel" id="use_crypto_desc">
179+ <property name="visible">True</property>
180+ <property name="sensitive">False</property>
181+ <property name="can_focus">False</property>
182+ <property name="margin_left">48</property>
183+ <property name="xalign">0</property>
184+ <property name="yalign">0</property>
185+ <property name="use_markup">True</property>
186+ <property name="wrap">True</property>
187+ <property name="label" translatable="yes">&lt;span size="small"&gt;You'll choose a security key in the next step&lt;/span&gt;</property>
188+ </object>
189+ <packing>
190+ <property name="expand">False</property>
191+ <property name="fill">True</property>
192+ <property name="position">3</property>
193+ </packing>
194+ </child>
195+ <child>
196+ <object class="GtkRadioButton" id="use_zfs">
197+ <property name="label" translatable="yes">Use ZFS (experimental)</property>
198+ <property name="visible">True</property>
199+ <property name="can_focus">True</property>
200+ <property name="receives_default">False</property>
201+ <property name="active">True</property>
202+ <property name="draw_indicator">True</property>
203+ <property name="margin_top">9</property>
204+ <property name="group">advanced_features_radio_none</property>
205+ <signal name="clicked" handler="advanced_features_option_changed" swapped="no"/>
206+ </object>
207+ <packing>
208+ <property name="expand">False</property>
209+ <property name="fill">True</property>
210+ <property name="position">4</property>
211+ </packing>
212+ </child>
213+ </object>
214+ <packing>
215+ <property name="expand">False</property>
216+ <property name="fill">True</property>
217+ <property name="position">1</property>
218+ </packing>
219+ </child>
220+ </object>
221+ </child>
222+ <action-widgets>
223+ <action-widget response="-6">advanced_features_cancelbutton</action-widget>
224+ <action-widget response="-5">advanced_features_okbutton</action-widget>
225+ </action-widgets>
226+ </object>
227+
228 <object class="GtkAlignment" id="stepPartAsk">
229 <property name="visible">True</property>
230 <property name="can_focus">False</property>
231@@ -185,118 +348,60 @@
232 <property name="height">1</property>
233 </packing>
234 </child>
235+
236 <child>
237- <object class="GtkLabel" id="use_crypto_desc">
238+ <object class="GtkBox" id="gtkbox1">
239 <property name="visible">True</property>
240 <property name="can_focus">False</property>
241- <property name="margin_left">48</property>
242- <property name="xalign">0</property>
243- <property name="yalign">0</property>
244- <property name="label" translatable="yes">&lt;span size="small"&gt;You’ll choose a security key in the next step.&lt;/span&gt;</property>
245- <property name="use_markup">True</property>
246- <property name="wrap">True</property>
247- </object>
248- <packing>
249- <property name="left_attach">0</property>
250- <property name="top_attach">9</property>
251- <property name="width">1</property>
252- <property name="height">1</property>
253- </packing>
254- </child>
255- <child>
256- <object class="GtkCheckButton" id="use_crypto">
257- <property name="label" translatable="yes">Encrypt the new Ubuntu installation for security</property>
258- <property name="visible">True</property>
259- <property name="can_focus">True</property>
260- <property name="margin_left">24</property>
261- <property name="receives_default">False</property>
262- <property name="margin_top">9</property>
263- <property name="xalign">0</property>
264- <property name="draw_indicator">True</property>
265- <signal name="clicked" handler="on_crypto_lvm_toggled" swapped="no"/>
266+ <child>
267+ <object class="GtkButton" id="advanced_features_button">
268+ <property name="visible">True</property>
269+ <property name="can_focus">True</property>
270+ <property name="margin_left">24</property>
271+ <property name="receives_default">False</property>
272+ <property name="xalign">0</property>
273+ <signal name="clicked" handler="on_advanced_features_clicked" swapped="no"/>
274+ <child>
275+ <object class="GtkLabel" id="advanced_features_selected">
276+ <property name="visible">True</property>
277+ <property name="can_focus">False</property>
278+ <property name="label" translatable="yes">Advanced features...</property>
279+ <property name="use_markup">True</property>
280+ </object>
281+ </child>
282+ </object>
283+ <packing>
284+ <property name="expand">False</property>
285+ <property name="fill">True</property>
286+ <property name="position">1</property>
287+ <property name="left_attach">0</property>
288+ <property name="width">1</property>
289+ <property name="height">1</property>
290+ </packing>
291+ </child>
292+ <child>
293+ <object class="GtkLabel" id="advanced_features_desc">
294+ <property name="visible">True</property>
295+ <property name="can_focus">False</property>
296+ <property name="label" translatable="yes">None selected</property>
297+ <property name="margin_left">9</property>
298+ </object>
299+ <packing>
300+ <property name="expand">False</property>
301+ <property name="fill">True</property>
302+ <property name="position">1</property>
303+ <property name="width">1</property>
304+ <property name="height">1</property>
305+ </packing>
306+ </child>
307 </object>
308 <packing>
309+ <property name="expand">False</property>
310+ <property name="fill">True</property>
311+ <property name="position">2</property>
312 <property name="left_attach">0</property>
313 <property name="top_attach">8</property>
314- <property name="width">1</property>
315- <property name="height">1</property>
316- </packing>
317- </child>
318- <child>
319- <object class="GtkLabel" id="use_lvm_desc">
320- <property name="visible">True</property>
321- <property name="can_focus">False</property>
322- <property name="margin_left">48</property>
323- <property name="xalign">0</property>
324- <property name="yalign">0</property>
325- <property name="label" translatable="yes">&lt;span size="small"&gt;This will set up Logical Volume Management. It allows taking snapshots and easier partition resizing.&lt;/span&gt;</property>
326- <property name="use_markup">True</property>
327- <property name="wrap">True</property>
328- </object>
329- <packing>
330- <property name="left_attach">0</property>
331- <property name="top_attach">11</property>
332- <property name="width">1</property>
333- <property name="height">1</property>
334- </packing>
335- </child>
336- <child>
337- <object class="GtkCheckButton" id="use_lvm">
338- <property name="label" translatable="yes">Use LVM with the new Ubuntu installation</property>
339- <property name="visible">True</property>
340- <property name="can_focus">True</property>
341- <property name="margin_left">24</property>
342- <property name="receives_default">False</property>
343- <property name="margin_top">9</property>
344- <property name="xalign">0</property>
345- <property name="draw_indicator">True</property>
346- <signal name="clicked" handler="on_crypto_lvm_toggled" swapped="no"/>
347- </object>
348- <packing>
349- <property name="left_attach">0</property>
350- <property name="top_attach">10</property>
351- <property name="width">1</property>
352- <property name="height">1</property>
353- </packing>
354- </child>
355
356- <child>
357- <object class="GtkLabel" id="use_zfs_desc">
358- <property name="visible">True</property>
359- <property name="can_focus">False</property>
360- <property name="margin_left">24</property>
361- <property name="xalign">0</property>
362- <property name="yalign">0</property>
363- <property name="label" translatable="yes">&lt;span foreground="darkred"&gt;Warning:&lt;/span&gt; This will delete all your files on all operating systems.
364-This is experimental and may cause data loss. Do not use on production systems.</property>
365- <property name="use_markup">True</property>
366- <property name="wrap">True</property>
367- </object>
368- <packing>
369- <property name="left_attach">0</property>
370- <property name="top_attach">14</property>
371- <property name="width">1</property>
372- <property name="height">1</property>
373- </packing>
374- </child>
375- <child>
376- <object class="GtkRadioButton" id="use_zfs">
377- <property name="label" translatable="yes">EXPERIMENTAL: Erase disk and use ZFS</property>
378- <property name="visible">True</property>
379- <property name="can_focus">True</property>
380- <property name="receives_default">False</property>
381- <property name="margin_top">9</property>
382- <property name="xalign">0</property>
383- <property name="active">True</property>
384- <property name="draw_indicator">True</property>
385- <property name="group">reuse_partition</property>
386- <signal name="clicked" handler="part_ask_option_changed" swapped="no"/>
387- </object>
388- <packing>
389- <property name="left_attach">0</property>
390- <property name="top_attach">13</property>
391- <property name="width">1</property>
392- <property name="height">1</property>
393 </packing>
394 </child>
395
396@@ -313,7 +418,7 @@ This is experimental and may cause data loss. Do not use on production systems.<
397 </object>
398 <packing>
399 <property name="left_attach">0</property>
400- <property name="top_attach">16</property>
401+ <property name="top_attach">10</property>
402 <property name="width">1</property>
403 <property name="height">1</property>
404 </packing>
405@@ -333,7 +438,7 @@ This is experimental and may cause data loss. Do not use on production systems.<
406 </object>
407 <packing>
408 <property name="left_attach">0</property>
409- <property name="top_attach">15</property>
410+ <property name="top_attach">9</property>
411 <property name="width">1</property>
412 <property name="height">1</property>
413 </packing>
414diff --git a/tests/run-pep8 b/tests/run-pep8
415index c0c8303..a71211d 100755
416--- a/tests/run-pep8
417+++ b/tests/run-pep8
418@@ -5,5 +5,6 @@
419 # FIXME: re-enable W504 and update for new breaking rules for binary op
420 pep8 --exclude keyboard_names.py --exclude Keyboard.py \
421 --ignore=E402,W504 \
422+ --max-line-length=119 \
423 $(grep -lr '#!.*bin/python' bin) \
424 scripts tests tests/run tests/run-frontend ubiquity autopilot
425diff --git a/tests/test_gtkui.py b/tests/test_gtkui.py
426index 5a948bf..60cbfd3 100644
427--- a/tests/test_gtkui.py
428+++ b/tests/test_gtkui.py
429@@ -106,6 +106,8 @@ class TestFrontend(unittest.TestCase):
430 'cancelbutton2', 'okbutton2', 'okbutton3',
431 'partition_dialog_okbutton', 'cancelbutton3',
432 'grub_fail_okbutton',
433+ 'advanced_features_cancelbutton',
434+ 'advanced_features_okbutton',
435 # These are calculated and set as the partitioning options are
436 # being calculated.
437 'reuse_partition_desc', 'reuse_partition',
438@@ -113,6 +115,7 @@ class TestFrontend(unittest.TestCase):
439 'resize_use_free_desc', 'resize_use_free',
440 'use_device_desc', 'use_device', 'part_ask_heading',
441 'custom_partitioning_desc', 'custom_partitioning',
442+ 'advanced_features_selected',
443 # Pulled straight from debconf when the installation medium is
444 # already mounted.
445 'part_advanced_warning_message',
446diff --git a/ubiquity/plugins/ubi-partman.py b/ubiquity/plugins/ubi-partman.py
447index a1f731d..5601d7d 100644
448--- a/ubiquity/plugins/ubi-partman.py
449+++ b/ubiquity/plugins/ubi-partman.py
450@@ -200,10 +200,6 @@ class PageGtk(PageBase):
451 self.partition_toolbar.child_set_property(wdg, 'homogeneous',
452 False)
453
454- zpool_exists = os.path.exists('/sbin/zpool')
455- self.use_zfs.set_visible(zpool_exists)
456- self.use_zfs_desc.set_visible(zpool_exists)
457-
458 # GtkBuilder signal mapping is broken (LP: # 852054).
459 self.part_auto_hidden_label.connect(
460 'activate-link', self.part_auto_hidden_label_activate_link)
461@@ -289,6 +285,44 @@ class PageGtk(PageBase):
462 width, height)
463 widget.show()
464
465+ def on_advanced_features_clicked(self, widget):
466+ from gi.repository import Gtk
467+
468+ # Save current state of advanced features
469+ selected = None
470+ crypto_selected = self.use_crypto.get_active()
471+ for w in (self.advanced_features_radio_none,
472+ self.use_lvm, self.use_zfs):
473+ if w.get_active():
474+ selected = w
475+ break
476+
477+ # Only show zfs when available
478+ zpool_exists = os.path.exists('/sbin/zpool')
479+ self.use_zfs.set_visible(zpool_exists)
480+
481+ dlg = self.advanced_features_dialog
482+ dlg.show()
483+ response = dlg.run()
484+ dlg.hide()
485+
486+ if response == Gtk.ResponseType.OK:
487+ label = ""
488+ if self.advanced_features_radio_none.get_active():
489+ label = self.controller.get_string('advanced_features_none_selected')
490+ elif self.use_lvm.get_active():
491+ label = self.controller.get_string('advanced_features_lvm_selected')
492+ if self.use_crypto.get_active():
493+ label = self.controller.get_string('advanced_features_crypto_selected')
494+ elif self.use_zfs.get_active():
495+ label = self.controller.get_string('advanced_features_zfs_selected')
496+ self.advanced_features_desc.set_text(label)
497+ else:
498+ # Restore previous selection
499+ if selected:
500+ selected.set_active(True)
501+ self.use_crypto.set_active(crypto_selected)
502+
503 def plugin_on_next_clicked(self):
504 reuse = self.reuse_partition.get_active()
505 replace = self.replace_partition.get_active()
506@@ -475,12 +509,20 @@ class PageGtk(PageBase):
507 self.controller.toggle_next_button()
508 self.plugin_is_install = about_to_install
509
510- # Supporting crypto and lvm in new installs only for now
511+ # Advanced features in new installs only for now
512 use_device = self.use_device.get_active()
513- self.use_lvm.set_sensitive(use_device)
514- self.use_crypto.set_sensitive(use_device)
515- self.use_lvm_desc.set_sensitive(use_device)
516- self.use_crypto_desc.set_sensitive(use_device)
517+ self.advanced_features_button.set_sensitive(use_device)
518+ self.advanced_features_selected.set_sensitive(use_device)
519+
520+ def advanced_features_option_changed(self, widget):
521+ if not widget.get_active():
522+ return
523+
524+ use_lvm = self.use_lvm.get_active()
525+ if not use_lvm:
526+ self.use_crypto.set_active(False)
527+ self.use_crypto.set_sensitive(use_lvm)
528+ self.use_crypto_desc.set_sensitive(use_lvm)
529
530 def initialize_resize_mode(self):
531 disk_id = self.get_current_disk_partman_id()
532@@ -662,21 +704,23 @@ class PageGtk(PageBase):
533 ticked = False
534 for option, name in option_to_widget:
535 opt_widget = getattr(self, name)
536- opt_desc = getattr(self, name + '_desc')
537+ opt_desc = getattr(self, name + '_desc', None)
538
539 if option in options:
540 opt_widget.show()
541- opt_desc.show()
542 opt_widget.set_label(options[option].title)
543- opt_desc.set_markup(fmt % options[option].desc)
544+
545+ if opt_desc:
546+ opt_desc.show()
547+ opt_desc.set_markup(fmt % options[option].desc)
548+
549 if not ticked and opt_widget.get_sensitive():
550 opt_widget.set_active(True)
551 ticked = True
552 else:
553 opt_widget.hide()
554- opt_desc.hide()
555-
556- self.use_zfs_desc.set_markup(fmt % self.use_zfs_desc.get_label())
557+ if opt_desc:
558+ opt_desc.hide()
559
560 # Process the default selection
561 self.part_ask_option_changed(None)

Subscribers

People subscribed via source and target branches