[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