Merge ~jibel/ubiquity:advanced_features_dialog into ubiquity:master

[Jean-Baptiste Lallement]

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

[Steve Langasek (vorlon)]

review: Needs Information

[Didier Roche-Tolomelli (didrocks)]

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)]

@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-Tolomelli (didrocks)]

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

diff --git a/debian/changelog b/debian/changelog
index 5d1a3dd..8a152ab 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,6 +1,8 @@
 ubiquity (20.04.1) UNRELEASED; urgency=medium
 
   * script/zsys-setup: fix tag name from org.zsys to com.ubuntu.zsys.
+  * Added an advanced features dialog to the partitioning page. LVM, LVM with
+    encryption and experimental ZFS support have been moved to this dialog.
 
  -- Jean-Baptiste Lallement <[e-mail]>  Mon, 28 Oct 2019 09:07:57 +0100
 
diff --git a/debian/ubiquity.templates b/debian/ubiquity.templates
index 57c5ca3..f56ac69 100644
--- a/debian/ubiquity.templates
+++ b/debian/ubiquity.templates
@@ -1449,6 +1449,38 @@ Default: false
 Description: for internal use; can be preseeded
  This will set up ZFS and configure it for zsys.
 
+Template: ubiquity/text/advanced_features_dialog
+Type: text
+_Description: Advanced Features
+
+Template: ubiquity/text/advanced_features_radio_none
+Type: text
+_Description: None
+
+Template: ubiquity/text/advanced_features_desc
+Type: text
+_Description: None selected
+
+Template: ubiquity/text/advanced_features_selected
+Type: text
+_Description: Advanced features...
+
+Template: ubiquity/text/advanced_features_none_selected
+Type: text
+_Description: None selected
+
+Template: ubiquity/text/advanced_features_lvm_selected
+Type: text
+_Description: LVM selected
+
+Template: ubiquity/text/advanced_features_crypto_selected
+Type: text
+_Description: LVM and encryption selected
+
+Template: ubiquity/text/advanced_features_zfs_selected
+Type: text
+_Description: ZFS selected
+
 Template: ubiquity/text/verified_crypto_label
 Type: text
 _Description:
diff --git a/gui/gtk/stepPartAsk.ui b/gui/gtk/stepPartAsk.ui
index bb89fc6..f734558 100644
--- a/gui/gtk/stepPartAsk.ui
+++ b/gui/gtk/stepPartAsk.ui
@@ -1,6 +1,169 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <interface>
   <!-- interface-requires gtk+ 3.0 -->
+  <object class="GtkDialog" id="advanced_features_dialog">
+    <property name="can_focus">False</property>
+    <property name="title" translatable="yes">Advanced Features</property>
+    <property name="resizable">False</property>
+    <property name="modal">True</property>
+    <property name="window_position">center-on-parent</property>
+    <property name="type_hint">dialog</property>
+    <child internal-child="vbox">
+      <object class="GtkBox" id="advanced_features_box1">
+        <property name="can_focus">False</property>
+        <property name="orientation">vertical</property>
+        <property name="spacing">24</property>
+        <child internal-child="action_area">
+          <object class="GtkButtonBox" id="advanced_features_buttonbox">
+            <property name="can_focus">False</property>
+            <property name="layout_style">end</property>
+            <child>
+              <object class="GtkButton" id="advanced_features_cancelbutton">
+                <property name="label">_Cancel</property>
+                <property name="use_action_appearance">False</property>
+                <property name="visible">True</property>
+                <property name="can_focus">True</property>
+                <property name="can_default">True</property>
+                <property name="receives_default">True</property>
+                <property name="use_underline">True</property>
+              </object>
+              <packing>
+                <property name="expand">False</property>
+                <property name="fill">False</property>
+                <property name="position">0</property>
+              </packing>
+            </child>
+            <child>
+              <object class="GtkButton" id="advanced_features_okbutton">
+                <property name="label">_OK</property>
+                <property name="use_action_appearance">False</property>
+                <property name="visible">True</property>
+                <property name="can_focus">True</property>
+                <property name="can_default">True</property>
+                <property name="has_default">True</property>
+                <property name="receives_default">True</property>
+                <property name="use_underline">True</property>
+              </object>
+              <packing>
+                <property name="expand">False</property>
+                <property name="fill">False</property>
+                <property name="position">1</property>
+              </packing>
+            </child>
+          </object>
+          <packing>
+            <property name="expand">False</property>
+            <property name="fill">False</property>
+            <property name="position">0</property>
+          </packing>
+        </child>
+        <child>
+          <object class="GtkBox" id="advanced_features_box2">
+            <property name="visible">True</property>
+            <property name="can_focus">False</property>
+            <property name="orientation">vertical</property>
+            <child>
+              <object class="GtkRadioButton" id="advanced_features_radio_none">
+                <property name="label" translatable="yes">None</property>
+                <property name="visible">True</property>
+                <property name="can_focus">True</property>
+                <property name="receives_default">True</property>
+                <property name="active">True</property>
+                <property name="draw_indicator">True</property>
+                <property name="margin_top">9</property>
+                <signal name="clicked" handler="advanced_features_option_changed" swapped="no"/>
+              </object>
+              <packing>
+                <property name="expand">False</property>
+                <property name="fill">True</property>
+                <property name="position">0</property>
+              </packing>
+            </child>
+            <child>
+              <object class="GtkRadioButton" id="use_lvm">
+                <property name="label" translatable="yes">Set up the system as an LVM volume group</property>
+                <property name="visible">True</property>
+                <property name="can_focus">True</property>
+                <property name="receives_default">False</property>
+                <property name="active">True</property>
+                <property name="draw_indicator">True</property>
+                <property name="margin_top">9</property>
+                <property name="group">advanced_features_radio_none</property>
+                <signal name="clicked" handler="advanced_features_option_changed" swapped="no"/>
+              </object>
+              <packing>
+                <property name="expand">False</property>
+                <property name="fill">True</property>
+                <property name="position">1</property>
+              </packing>
+            </child>
+            <child>
+              <object class="GtkCheckButton" id="use_crypto">
+                <property name="label" translatable="yes">Encrypt the volume group for security</property>
+                <property name="visible">True</property>
+                <property name="sensitive">False</property>
+                <property name="can_focus">True</property>
+                <property name="receives_default">False</property>
+                <property name="draw_indicator">True</property>
+                <property name="margin_left">24</property>
+              </object>
+              <packing>
+                <property name="expand">False</property>
+                <property name="fill">True</property>
+                <property name="position">2</property>
+              </packing>
+            </child>
+            <child>
+              <object class="GtkLabel" id="use_crypto_desc">
+                <property name="visible">True</property>
+                <property name="sensitive">False</property>
+                <property name="can_focus">False</property>
+                <property name="margin_left">48</property>
+                <property name="xalign">0</property>
+                <property name="yalign">0</property>
+                <property name="use_markup">True</property>
+                <property name="wrap">True</property>
+                <property name="label" translatable="yes">&lt;span size="small"&gt;You'll choose a security key in the next step&lt;/span&gt;</property>
+              </object>
+              <packing>
+                <property name="expand">False</property>
+                <property name="fill">True</property>
+                <property name="position">3</property>
+              </packing>
+            </child>
+            <child>
+              <object class="GtkRadioButton" id="use_zfs">
+                <property name="label" translatable="yes">Use ZFS (experimental)</property>
+                <property name="visible">True</property>
+                <property name="can_focus">True</property>
+                <property name="receives_default">False</property>
+                <property name="active">True</property>
+                <property name="draw_indicator">True</property>
+                <property name="margin_top">9</property>
+                <property name="group">advanced_features_radio_none</property>
+                <signal name="clicked" handler="advanced_features_option_changed" swapped="no"/>
+              </object>
+              <packing>
+                <property name="expand">False</property>
+                <property name="fill">True</property>
+                <property name="position">4</property>
+              </packing>
+            </child>
+          </object>
+          <packing>
+            <property name="expand">False</property>
+            <property name="fill">True</property>
+            <property name="position">1</property>
+          </packing>
+        </child>
+      </object>
+    </child>
+    <action-widgets>
+      <action-widget response="-6">advanced_features_cancelbutton</action-widget>
+      <action-widget response="-5">advanced_features_okbutton</action-widget>
+    </action-widgets>
+  </object>
+
   <object class="GtkAlignment" id="stepPartAsk">
     <property name="visible">True</property>
     <property name="can_focus">False</property>
@@ -185,118 +348,60 @@
                     <property name="height">1</property>
                   </packing>
                 </child>
+
                 <child>
-                  <object class="GtkLabel" id="use_crypto_desc">
+                  <object class="GtkBox" id="gtkbox1">
                     <property name="visible">True</property>
                     <property name="can_focus">False</property>
-                    <property name="margin_left">48</property>
-                    <property name="xalign">0</property>
-                    <property name="yalign">0</property>
-                    <property name="label" translatable="yes">&lt;span size="small"&gt;You’ll choose a security key in the next step.&lt;/span&gt;</property>
-                    <property name="use_markup">True</property>
-                    <property name="wrap">True</property>
-                  </object>
-                  <packing>
-                    <property name="left_attach">0</property>
-                    <property name="top_attach">9</property>
-                    <property name="width">1</property>
-                    <property name="height">1</property>
-                  </packing>
-                </child>
-                <child>
-                  <object class="GtkCheckButton" id="use_crypto">
-                    <property name="label" translatable="yes">Encrypt the new Ubuntu installation for security</property>
-                    <property name="visible">True</property>
-                    <property name="can_focus">True</property>
-                    <property name="margin_left">24</property>
-                    <property name="receives_default">False</property>
-                    <property name="margin_top">9</property>
-                    <property name="xalign">0</property>
-                    <property name="draw_indicator">True</property>
-                    <signal name="clicked" handler="on_crypto_lvm_toggled" swapped="no"/>
+                    <child>
+                      <object class="GtkButton" id="advanced_features_button">
+                        <property name="visible">True</property>
+                        <property name="can_focus">True</property>
+                        <property name="margin_left">24</property>
+                        <property name="receives_default">False</property>
+                        <property name="xalign">0</property>
+                        <signal name="clicked" handler="on_advanced_features_clicked" swapped="no"/>
+                        <child>
+                          <object class="GtkLabel" id="advanced_features_selected">
+                            <property name="visible">True</property>
+                            <property name="can_focus">False</property>
+                            <property name="label" translatable="yes">Advanced features...</property>
+                            <property name="use_markup">True</property>
+                          </object>
+                        </child>
+                      </object>
+                      <packing>
+                        <property name="expand">False</property>
+                        <property name="fill">True</property>
+                        <property name="position">1</property>
+                        <property name="left_attach">0</property>
+                        <property name="width">1</property>
+                        <property name="height">1</property>
+                      </packing>
+                    </child>
+                    <child>
+                      <object class="GtkLabel" id="advanced_features_desc">
+                        <property name="visible">True</property>
+                        <property name="can_focus">False</property>
+                        <property name="label" translatable="yes">None selected</property>
+                        <property name="margin_left">9</property>
+                      </object>
+                      <packing>
+                        <property name="expand">False</property>
+                        <property name="fill">True</property>
+                        <property name="position">1</property>
+                        <property name="width">1</property>
+                        <property name="height">1</property>
+                      </packing>
+                    </child>
                   </object>
                   <packing>
+                    <property name="expand">False</property>
+                    <property name="fill">True</property>
+                    <property name="position">2</property>
                     <property name="left_attach">0</property>
                     <property name="top_attach">8</property>
-                    <property name="width">1</property>
-                    <property name="height">1</property>
-                  </packing>
-                </child>
-                <child>
-                  <object class="GtkLabel" id="use_lvm_desc">
-                    <property name="visible">True</property>
-                    <property name="can_focus">False</property>
-                    <property name="margin_left">48</property>
-                    <property name="xalign">0</property>
-                    <property name="yalign">0</property>
-                    <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>
-                    <property name="use_markup">True</property>
-                    <property name="wrap">True</property>
-                  </object>
-                  <packing>
-                    <property name="left_attach">0</property>
-                    <property name="top_attach">11</property>
-                    <property name="width">1</property>
-                    <property name="height">1</property>
-                  </packing>
-                </child>
-                <child>
-                  <object class="GtkCheckButton" id="use_lvm">
-                    <property name="label" translatable="yes">Use LVM with the new Ubuntu installation</property>
-                    <property name="visible">True</property>
-                    <property name="can_focus">True</property>
-                    <property name="margin_left">24</property>
-                    <property name="receives_default">False</property>
-                    <property name="margin_top">9</property>
-                    <property name="xalign">0</property>
-                    <property name="draw_indicator">True</property>
-                    <signal name="clicked" handler="on_crypto_lvm_toggled" swapped="no"/>
-                  </object>
-                  <packing>
-                    <property name="left_attach">0</property>
-                    <property name="top_attach">10</property>
-                    <property name="width">1</property>
-                    <property name="height">1</property>
-                  </packing>
-                </child>
 
-                <child>
-                  <object class="GtkLabel" id="use_zfs_desc">
-                    <property name="visible">True</property>
-                    <property name="can_focus">False</property>
-                    <property name="margin_left">24</property>
-                    <property name="xalign">0</property>
-                    <property name="yalign">0</property>
-                    <property name="label" translatable="yes">&lt;span foreground="darkred"&gt;Warning:&lt;/span&gt; This will delete all your files on all operating systems.
-This is experimental and may cause data loss. Do not use on production systems.</property>
-                    <property name="use_markup">True</property>
-                    <property name="wrap">True</property>
-                  </object>
-                  <packing>
-                    <property name="left_attach">0</property>
-                    <property name="top_attach">14</property>
-                    <property name="width">1</property>
-                    <property name="height">1</property>
-                  </packing>
-                </child>
-                <child>
-                  <object class="GtkRadioButton" id="use_zfs">
-                    <property name="label" translatable="yes">EXPERIMENTAL: Erase disk and use ZFS</property>
-                    <property name="visible">True</property>
-                    <property name="can_focus">True</property>
-                    <property name="receives_default">False</property>
-                    <property name="margin_top">9</property>
-                    <property name="xalign">0</property>
-                    <property name="active">True</property>
-                    <property name="draw_indicator">True</property>
-                    <property name="group">reuse_partition</property>
-                    <signal name="clicked" handler="part_ask_option_changed" swapped="no"/>
-                  </object>
-                  <packing>
-                    <property name="left_attach">0</property>
-                    <property name="top_attach">13</property>
-                    <property name="width">1</property>
-                    <property name="height">1</property>
                   </packing>
                 </child>
 
@@ -313,7 +418,7 @@ This is experimental and may cause data loss. Do not use on production systems.<
                   </object>
                   <packing>
                     <property name="left_attach">0</property>
-                    <property name="top_attach">16</property>
+                    <property name="top_attach">10</property>
                     <property name="width">1</property>
                     <property name="height">1</property>
                   </packing>
@@ -333,7 +438,7 @@ This is experimental and may cause data loss. Do not use on production systems.<
                   </object>
                   <packing>
                     <property name="left_attach">0</property>
-                    <property name="top_attach">15</property>
+                    <property name="top_attach">9</property>
                     <property name="width">1</property>
                     <property name="height">1</property>
                   </packing>
diff --git a/tests/run-pep8 b/tests/run-pep8
index c0c8303..a71211d 100755
--- a/tests/run-pep8
+++ b/tests/run-pep8
@@ -5,5 +5,6 @@
 # FIXME: re-enable W504 and update for new breaking rules for binary op
 pep8 --exclude keyboard_names.py --exclude Keyboard.py \
     --ignore=E402,W504 \
+    --max-line-length=119 \
     $(grep -lr '#!.*bin/python' bin) \
     scripts tests tests/run tests/run-frontend ubiquity autopilot
diff --git a/tests/test_gtkui.py b/tests/test_gtkui.py
index 5a948bf..60cbfd3 100644
--- a/tests/test_gtkui.py
+++ b/tests/test_gtkui.py
@@ -106,6 +106,8 @@ class TestFrontend(unittest.TestCase):
                 'cancelbutton2', 'okbutton2', 'okbutton3',
                 'partition_dialog_okbutton', 'cancelbutton3',
                 'grub_fail_okbutton',
+                'advanced_features_cancelbutton',
+                'advanced_features_okbutton',
                 # These are calculated and set as the partitioning options are
                 # being calculated.
                 'reuse_partition_desc', 'reuse_partition',
@@ -113,6 +115,7 @@ class TestFrontend(unittest.TestCase):
                 'resize_use_free_desc', 'resize_use_free',
                 'use_device_desc', 'use_device', 'part_ask_heading',
                 'custom_partitioning_desc', 'custom_partitioning',
+                'advanced_features_selected',
                 # Pulled straight from debconf when the installation medium is
                 # already mounted.
                 'part_advanced_warning_message',
diff --git a/ubiquity/plugins/ubi-partman.py b/ubiquity/plugins/ubi-partman.py
index a1f731d..5601d7d 100644
--- a/ubiquity/plugins/ubi-partman.py
+++ b/ubiquity/plugins/ubi-partman.py
@@ -200,10 +200,6 @@ class PageGtk(PageBase):
             self.partition_toolbar.child_set_property(wdg, 'homogeneous',
                                                       False)
 
-        zpool_exists = os.path.exists('/sbin/zpool')
-        self.use_zfs.set_visible(zpool_exists)
-        self.use_zfs_desc.set_visible(zpool_exists)
-
         # GtkBuilder signal mapping is broken (LP: # 852054).
         self.part_auto_hidden_label.connect(
             'activate-link', self.part_auto_hidden_label_activate_link)
@@ -289,6 +285,44 @@ class PageGtk(PageBase):
                                       width, height)
             widget.show()
 
+    def on_advanced_features_clicked(self, widget):
+        from gi.repository import Gtk
+
+        # Save current state of advanced features
+        selected = None
+        crypto_selected = self.use_crypto.get_active()
+        for w in (self.advanced_features_radio_none,
+                  self.use_lvm, self.use_zfs):
+            if w.get_active():
+                selected = w
+                break
+
+        # Only show zfs when available
+        zpool_exists = os.path.exists('/sbin/zpool')
+        self.use_zfs.set_visible(zpool_exists)
+
+        dlg = self.advanced_features_dialog
+        dlg.show()
+        response = dlg.run()
+        dlg.hide()
+
+        if response == Gtk.ResponseType.OK:
+            label = ""
+            if self.advanced_features_radio_none.get_active():
+                label = self.controller.get_string('advanced_features_none_selected')
+            elif self.use_lvm.get_active():
+                label = self.controller.get_string('advanced_features_lvm_selected')
+                if self.use_crypto.get_active():
+                    label = self.controller.get_string('advanced_features_crypto_selected')
+            elif self.use_zfs.get_active():
+                label = self.controller.get_string('advanced_features_zfs_selected')
+            self.advanced_features_desc.set_text(label)
+        else:
+            # Restore previous selection
+            if selected:
+                selected.set_active(True)
+            self.use_crypto.set_active(crypto_selected)
+
     def plugin_on_next_clicked(self):
         reuse = self.reuse_partition.get_active()
         replace = self.replace_partition.get_active()
@@ -475,12 +509,20 @@ class PageGtk(PageBase):
                 self.controller.toggle_next_button()
             self.plugin_is_install = about_to_install
 
-        # Supporting crypto and lvm in new installs only for now
+        # Advanced features in new installs only for now
         use_device = self.use_device.get_active()
-        self.use_lvm.set_sensitive(use_device)
-        self.use_crypto.set_sensitive(use_device)
-        self.use_lvm_desc.set_sensitive(use_device)
-        self.use_crypto_desc.set_sensitive(use_device)
+        self.advanced_features_button.set_sensitive(use_device)
+        self.advanced_features_selected.set_sensitive(use_device)
+
+    def advanced_features_option_changed(self, widget):
+        if not widget.get_active():
+            return
+
+        use_lvm = self.use_lvm.get_active()
+        if not use_lvm:
+            self.use_crypto.set_active(False)
+        self.use_crypto.set_sensitive(use_lvm)
+        self.use_crypto_desc.set_sensitive(use_lvm)
 
     def initialize_resize_mode(self):
         disk_id = self.get_current_disk_partman_id()
@@ -662,21 +704,23 @@ class PageGtk(PageBase):
         ticked = False
         for option, name in option_to_widget:
             opt_widget = getattr(self, name)
-            opt_desc = getattr(self, name + '_desc')
+            opt_desc = getattr(self, name + '_desc', None)
 
             if option in options:
                 opt_widget.show()
-                opt_desc.show()
                 opt_widget.set_label(options[option].title)
-                opt_desc.set_markup(fmt % options[option].desc)
+
+                if opt_desc:
+                    opt_desc.show()
+                    opt_desc.set_markup(fmt % options[option].desc)
+
                 if not ticked and opt_widget.get_sensitive():
                     opt_widget.set_active(True)
                     ticked = True
             else:
                 opt_widget.hide()
-                opt_desc.hide()
-
-        self.use_zfs_desc.set_markup(fmt % self.use_zfs_desc.get_label())
+                if opt_desc:
+                    opt_desc.hide()
 
         # Process the default selection
         self.part_ask_option_changed(None)