[anaconda:master] Simplify and robustify handling of fstype combo box.
Vratislav Podzimek
vpodzime at redhat.com
Mon Dec 15 10:07:55 UTC 2014
----- Original Message -----
> From: "mulhern" <amulhern at redhat.com>
> To: anaconda-patches at lists.fedorahosted.org
> Sent: Wednesday, December 10, 2014 5:46:47 PM
> Subject: [anaconda:master] Simplify and robustify handling of fstype combo box.
>
> 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()
> self._partitionsViewport.add(self._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)
I don't understand the purpose of the _fs_types variable here. Can't we clear and populate the self._fs_types set?
>
> @property
> 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()
This can be done in a single step by using sorted() which returns a list.
> +
> + # 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)
Other than the two comments above this looks good to me.
--
Vratislav Podzimek
More information about the anaconda-patches
mailing list