[anaconda:master] Simplify and robustify handling of fstype combo box.

mulhern amulhern at redhat.com
Wed Dec 10 16:46:47 UTC 2014

Changes are:
* just fill the combo from scratch rather than traversing it to remove
unwanted items.
  - therefore, don't fill the combo in _initialize
* Handle possible BTRFS device type in _setup_fstype_combo().
* Sort _all_ the names alphabetically, instead of tacking on a few
  unsupported filesystem names at end.
* Make _fs_types attribute be a set, not a list.
* Make a note that _update_fstype_combo() is idempotent and needs to stay
that way.

Non-changes are:
* Retain the complicated check for the original format's name. I don't
know why the check is all about the format's type instead of the name,
but it is, so I figure I'ld better leave it like that.

Advantages are:
* Get rid of a code smell. The first half of the code that removed
unsupported filesystem names assumed unsupported
filesystems names could occur anywhere in the combo box,
while the second part of the removal code assumed that unsupported
filesystem names could only occur after the device's filesystem name.
In the current setup, the second assumption is the correct one, so the
code actually worked. If the first assumption had been correct, then
the second half of the code would have been buggy. Therefore, the whole
code block looked suspicious.
* Make the custom spoke a bit less stateful.
* Make sure that btrfs name is included correctly. It currently is,
but only because _setup_device_type_combo() is invoked after
_setup_fstype_combo(), and _setup_device_type_combo() causes
_typeCombo._setActive() to be invoked, which triggers an event which
causes on_device_type_changed() to be invoked, which in turn invokes
the _update_fstype_combo() method. It seems wrong
to expect memory of that ordering requirement to be retained forever,
so better to make it unnecessary by invoking _update_fstype_combo()
explicitly in _setup_fstype_combo(). This means that _update_fstype_combo()
is called twice, but since it is idempotent, that is no problem.
* Fewer lines of code.

Signed-off-by: mulhern <amulhern at redhat.com>
 pyanaconda/ui/gui/spokes/custom.py | 71 ++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 37 deletions(-)

diff --git a/pyanaconda/ui/gui/spokes/custom.py b/pyanaconda/ui/gui/spokes/custom.py
index fca7f3f..e65316c 100644
--- a/pyanaconda/ui/gui/spokes/custom.py
+++ b/pyanaconda/ui/gui/spokes/custom.py
@@ -85,7 +85,7 @@ from pyanaconda.ui.gui.spokes.lib.custom_storage_helpers import get_container_ty
 from pyanaconda.ui.gui.spokes.lib.custom_storage_helpers import AddDialog, ConfirmDeleteDialog, DisksDialog, ContainerDialog, HelpDialog
 from pyanaconda.ui.gui.utils import setViewportBackground, fancy_set_sensitive, ignoreEscape
-from pyanaconda.ui.gui.utils import really_hide, really_show, GtkActionList, timed_action
+from pyanaconda.ui.gui.utils import really_hide, really_show, timed_action
 from pyanaconda.ui.categories.system import SystemCategory
 from gi.repository import Gdk, Gtk
@@ -161,7 +161,7 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
         self._devices = []
         self._error = None
         self._hidden_disks = []
-        self._fs_types = []             # list of supported fstypes
+        self._fs_types = set()             # set of supported fstypes
         self._free_space = Size(0)
         self._device_disks = []
@@ -273,15 +273,14 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
         self._accordion = Accordion()
-        # Populate the list of valid filesystem types from the format classes.
-        # Unfortunately, we have to narrow them down a little bit more because
-        # this list will include things like PVs and RAID members.
-        self._fsCombo.remove_all()
         threadMgr.add(AnacondaThread(name=THREAD_CUSTOM_STORAGE_INIT, target=self._initialize))
     def _initialize(self):
-        self._fs_types = []
+        """ Populate the set of valid filesystem types from the format classes.
+            Restrict the set to ones that we might allow users to select.
+        """
+        _fs_types = []
         for cls in device_formats.itervalues():
             obj = cls()
@@ -292,15 +291,9 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
                             (isinstance(obj, FS) or
                              obj.type in ["biosboot", "prepboot", "swap"]))
             if supported_fs:
-                self._fs_types.append(obj.name)
-        # Display the file system types in alphabetical order.
-        actions = GtkActionList()
-        self._fs_types.sort()
-        for ty in self._fs_types:
-            actions.add_action(self._fsCombo.append_text, ty)
+                _fs_types.append(obj.name)
-        actions.fire()
+        self._fs_types = set(_fs_types)
     def _clearpartDevices(self):
@@ -1303,32 +1296,34 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
            or defaultContainerRaidLevel(devicefactory.get_device_type(use_dev))
     def _setup_fstype_combo(self, device):
-        # remove any fs types that aren't supported
-        remove_indices = []
-        for idx, row in enumerate(self._fsCombo.get_model()):
-            fs_type = row[0]
-            if fs_type not in self._fs_types:
-                remove_indices.append(idx)
-                continue
-            if fs_type == device.format.name:
-                self._fsCombo.set_active(idx)
+        """ Setup the filesystem combo box.
-        # remove items from the combobox in reversed order so that item 3
-        # doesn't become item 2 by removing item 1 etc.
-        map(self._fsCombo.remove, reversed(remove_indices))
-        # if the current device has unsupported formatting, add an entry for it
-        if device.format.name not in self._fs_types:
-            self._fsCombo.append_text(device.format.name)
-            self._fsCombo.set_active(len(self._fsCombo.get_model()) - 1)
+            :param device: blivet.devices.Device instance
+        """
+        type_name = device.format.name
-        # Give them a way to reset to original formatting. Whenever we add a
-        # "reformat this" widget this will need revisiting.
+        # Possibly unsupported but still required filesystem names
         if device.exists and \
            device.format.type != device.originalFormat.type and \
            device.originalFormat.type not in self._fs_types:
-            self._fsCombo.append_text(device.originalFormat.name)
+            extra_names = (type_name, device.originalFormat.name)
+        else:
+            extra_names = (type_name,)
+        names = list(self._fs_types.union(extra_names))
+        names.sort()
+        # Add all desired fileystem type names to the box, sorted alphabetically
+        self._fsCombo.remove_all()
+        for ty in names:
+            self._fsCombo.append_text(ty)
+        # set the active filesystem type
+        idx = next(i for i, data in enumerate(self._fsCombo.get_model()) if data[0] == type_name)
+        self._fsCombo.set_active(idx)
+        # do additional updating handled by other method
+        self._update_fstype_combo(devicefactory.get_device_type(device))
     def _btrfs_in_typecombo(self, device):
         """ Whether BTRFS should appear in device type combo box.
@@ -2495,6 +2490,8 @@ class CustomPartitioningSpoke(NormalSpoke, StorageChecker):
             * the filesystem combo contains at least the default filesystem
             * the default filesystem is not the same as btrfs
             * if device_type is DEVICE_TYPE_BTRFS, btrfs is supported
+            This method is idempotent, and must remain so.
         # Find unique instance of btrfs in fsCombo, if any.
         btrfs_idx = next((idx for idx, data in enumerate(self._fsCombo.get_model()) if data[0] == "btrfs"), None)

More information about the anaconda-patches mailing list