From 77b42e7c5ab8261a843253053d690cf21c7a7f62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marta=20Marczykowska-G=C3=B3recka?= Date: Fri, 10 Jul 2020 14:08:22 +0200 Subject: [PATCH] Final improvements and cleanup to list widgets handling in manager Also removed old opaque prepare_choice functions and added extensive docstrings. --- qubesmanager/backup.py | 13 +- qubesmanager/bootfromdevice.py | 33 +- qubesmanager/create_new_vm.py | 23 +- qubesmanager/settings.py | 6 +- qubesmanager/tests/test_create_new_vm.py | 4 +- qubesmanager/tests/test_global_settings.py | 2 +- qubesmanager/utils.py | 335 +++++++++------------ 7 files changed, 188 insertions(+), 228 deletions(-) diff --git a/qubesmanager/backup.py b/qubesmanager/backup.py index 3c09fb1..651a996 100644 --- a/qubesmanager/backup.py +++ b/qubesmanager/backup.py @@ -98,18 +98,17 @@ class BackupVMsWindow(ui_backupdlg.Ui_Backup, QtWidgets.QWizard): self.total_size = 0 - self.target_vm_list, self.target_vm_idx = utils.prepare_vm_choice( - self.appvm_combobox, - self.qubes_app, - None, - self.qubes_app.domains['dom0'], + utils.initialize_widget_with_vms( + widget=self.appvm_combobox, + qubes_app=self.qubes_app, filter_function=(lambda vm: vm.klass != 'TemplateVM' and vm.is_running() and not vm.features.get('internal', False)), - allow_default=False, - allow_none=False + allow_internal=True, ) + self.appvm_combobox.setCurrentIndex( + self.appvm_combobox.findText("dom0")) self.unrecognized_config_label.setVisible(False) self.load_settings() diff --git a/qubesmanager/bootfromdevice.py b/qubesmanager/bootfromdevice.py index 5a9add1..03fa172 100644 --- a/qubesmanager/bootfromdevice.py +++ b/qubesmanager/bootfromdevice.py @@ -58,8 +58,7 @@ class VMBootFromDeviceWindow(ui_bootfromdevice.Ui_BootDialog, if self.blockDeviceRadioButton.isChecked(): cdrom_location = self.blockDeviceComboBox.currentText() elif self.fileRadioButton.isChecked(): - cdrom_location = str( - self.vm_list[self.fileVM.currentIndex()]) + \ + cdrom_location = str(self.fileVM.currentData()) + \ ":" + self.pathText.text() else: QtWidgets.QMessageBox.warning( @@ -93,22 +92,20 @@ class VMBootFromDeviceWindow(ui_bootfromdevice.Ui_BootDialog, self.fileRadioButton.clicked.connect(self.radio_button_clicked) self.selectFileButton.clicked.connect(self.select_file_dialog) - self.vm_list, self.vm_idx = utils.prepare_vm_choice( - self.fileVM, - self.vm, None, - None, - None, - allow_default=False, allow_none=False) + utils.initialize_widget_with_vms( + widget=self.fileVM, + qubes_app=self.qubesapp, + allow_internal=True + ) - self.block_list, self.block_idx = utils.prepare_choice( - self.blockDeviceComboBox, - self.vm, - None, - [device for domain in self.vm.app.domains - for device in domain.devices["block"]], - None, - None, - allow_default=False, allow_none=False + device_choice = [(str(device), device) for domain in self.vm.app.domains + for device in domain.devices["block"]] + + utils.initialize_widget( + widget=self.blockDeviceComboBox, + choices=device_choice, + selected_value=device_choice[0][1], + add_current_label=False ) def radio_button_clicked(self): @@ -119,7 +116,7 @@ class VMBootFromDeviceWindow(ui_bootfromdevice.Ui_BootDialog, self.pathText.setEnabled(self.fileRadioButton.isChecked()) def select_file_dialog(self): - backend_vm = self.vm_list[self.fileVM.currentIndex()] + backend_vm = self.fileVM.currentData() error_occurred = False try: diff --git a/qubesmanager/create_new_vm.py b/qubesmanager/create_new_vm.py index eb4ba74..f695774 100644 --- a/qubesmanager/create_new_vm.py +++ b/qubesmanager/create_new_vm.py @@ -41,7 +41,6 @@ class CreateVMThread(QtCore.QThread): def __init__(self, app, vmclass, name, label, template, properties, pool): QtCore.QThread.__init__(self) - print(vmclass, name, label, template, properties, pool) self.app = app self.vmclass = vmclass self.name = name @@ -54,18 +53,14 @@ class CreateVMThread(QtCore.QThread): def run(self): try: if self.vmclass == 'StandaloneVM' and self.template is not None: - if self.template is qubesadmin.DEFAULT: - src_vm = self.app.default_template - else: - src_vm = self.template - args = { 'ignore_volumes': ['private'] } if self.pool: args['pool'] = self.pool - vm = self.app.clone_vm(src_vm, self.name, self.vmclass, **args) + vm = self.app.clone_vm(self.template, self.name, + self.vmclass, **args) vm.label = self.label for k, v in self.properties.items(): @@ -107,22 +102,22 @@ class NewVmDlg(QtWidgets.QDialog, Ui_NewVMDlg): utils.initialize_widget_with_default( widget=self.template_vm, - item_list=self.app.domains, - filter_function=(lambda vm: not utils.is_internal(vm) and vm.klass == 'TemplateVM'), + choices=[(vm.name, vm) for vm in self.app.domains + if not utils.is_internal(vm) and vm.klass == 'TemplateVM'], mark_existing_as_default=True, default_value=self.app.default_template) utils.initialize_widget_with_default( widget=self.netvm, - item_list=self.app.domains, - filter_function=(lambda vm: not utils.is_internal(vm) and vm.provides_network), + choices=[(vm.name, vm) for vm in self.app.domains + if not utils.is_internal(vm) and vm.provides_network], add_none=True, add_qubes_default=True, default_value=self.app.default_netvm) utils.initialize_widget_with_default( widget=self.storage_pool, - item_list=self.app.pools.values(), + choices=[(str(pool), pool) for pool in self.app.pools.values()], add_qubes_default=True, mark_existing_as_default=True, default_value=self.app.default_pool) @@ -232,14 +227,14 @@ class NewVmDlg(QtWidgets.QDialog, Ui_NewVMDlg): self.install_system.setEnabled(False) self.install_system.setChecked(False) - if self.vm_type.currentData() == 'Standalone-copy': + if self.vm_type.currentData() == 'StandaloneVM-copy': self.template_vm.setEnabled(True) if self.template_vm.currentIndex() == -1: self.template_vm.setCurrentIndex(0) self.install_system.setEnabled(False) self.install_system.setChecked(False) - if self.vm_type.currentData() == 'Standalone-empty': + if self.vm_type.currentData() == 'StandaloneVM-empty': self.template_vm.setEnabled(False) self.template_vm.setCurrentIndex(-1) self.install_system.setEnabled(True) diff --git a/qubesmanager/settings.py b/qubesmanager/settings.py index da3efa2..0775fae 100644 --- a/qubesmanager/settings.py +++ b/qubesmanager/settings.py @@ -153,6 +153,10 @@ class VMSettingsWindow(ui_settingsdlg.Ui_SettingsDialog, QtWidgets.QDialog): self.tabWidget.currentChanged.connect(self.current_tab_changed) + # Initialize several auxillary variables for pylint's sake + self.root_img_size = None + self.priv_img_size = None + ###### basic tab self.__init_basic_tab__() self.rename_vm_button.clicked.connect(self.rename_vm) @@ -665,7 +669,7 @@ class VMSettingsWindow(ui_settingsdlg.Ui_SettingsDialog, QtWidgets.QDialog): self.progress.setModal(True) self.thread_closes = True self.progress.show() -# TODO: maybe this can not be repeated all the time? +# TODO: improvement: maybe this can be refactored into less repetition? thread.start() ######### advanced tab diff --git a/qubesmanager/tests/test_create_new_vm.py b/qubesmanager/tests/test_create_new_vm.py index e701f53..4bf0c0e 100644 --- a/qubesmanager/tests/test_create_new_vm.py +++ b/qubesmanager/tests/test_create_new_vm.py @@ -67,7 +67,7 @@ class NewVmTest(unittest.TestCase): self.mock_thread.assert_called_once_with( self.qapp, "AppVM", "test-vm", - unittest.mock.ANY, qubesadmin.DEFAULT, + unittest.mock.ANY, self.qapp.default_template, {'provides_network': False}, unittest.mock.ANY) self.mock_thread().start.assert_called_once_with() @@ -82,7 +82,7 @@ class NewVmTest(unittest.TestCase): self.mock_thread.assert_called_once_with( self.qapp, "AppVM", "test-vm", - self.qapp.labels['blue'], qubesadmin.DEFAULT, + self.qapp.labels['blue'], self.qapp.default_template, unittest.mock.ANY, unittest.mock.ANY) self.mock_thread().start.assert_called_once_with() diff --git a/qubesmanager/tests/test_global_settings.py b/qubesmanager/tests/test_global_settings.py index a4856c5..b570ff0 100644 --- a/qubesmanager/tests/test_global_settings.py +++ b/qubesmanager/tests/test_global_settings.py @@ -137,7 +137,7 @@ class GlobalSettingsTest(unittest.TestCase): or widget.currentText().startswith("(none)"): widget.setCurrentIndex(widget.currentIndex() + 1) - return widget.currentText() + return widget.currentData() def __set_none(self, widget): widget.setCurrentIndex(0) diff --git a/qubesmanager/utils.py b/qubesmanager/utils.py index 060ce46..4fe2606 100644 --- a/qubesmanager/utils.py +++ b/qubesmanager/utils.py @@ -5,6 +5,8 @@ # Copyright (C) 2012 Marek Marczykowski-Górecki # # Copyright (C) 2017 Wojtek Porczyk +# Copyright (C) 2020 Marta Marczykowska-Górecka +# # # This program is free software: you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -33,23 +35,33 @@ from qubesadmin import events from PyQt5 import QtWidgets, QtCore, QtGui # pylint: disable=import-error -#TODO: remove -def _filter_internal(vm): - return (not vm.klass == 'AdminVM' - and not vm.features.get('internal', False)) - +# important usage note: which initialize_widget should I use? +# - if you want a list of VMs, use initialize_widget_with_vms, optionally +# adding a property if you want to handle qubesadmin.DEFAULT and the +# current (potentially default) value +# - if you want a list of labels or kernals, use +# initialize_widget_with_kernels/labels +# - list of some things, but associated with a definite property (optionally +# with qubesadmin.DEFAULT) - initialize_widget_for_property +# - list of some things, not associated with a property, but still having a +# default - initialize_widget_with_default +# - just a list, no properties or defaults, just a nice list with a "current" +# value - initialize_widget def is_internal(vm): + """checks if the VM is either an AdminVM or has the 'internal' features""" return (vm.klass == 'AdminVM' or vm.features.get('internal', False)) def translate(string): + """helper function for translations""" return QtCore.QCoreApplication.translate( "ManagerUtils", string) class SizeSpinBox(QtWidgets.QSpinBox): + """A SpinBox subclass with extended handling for sizes in MB and GB""" # pylint: disable=invalid-name, no-self-use def __init__(self, *args, **kwargs): super(SizeSpinBox, self).__init__(*args, **kwargs) @@ -80,26 +92,32 @@ class SizeSpinBox(QtWidgets.QSpinBox): def get_boolean_feature(vm, feature_name): + """heper function to get a feature converted to a Bool if it does exist. + Necessary because of the true/false in features being coded as 1/empty + string.""" result = vm.features.get(feature_name, None) if result is not None: result = bool(result) return result -# TODO: doublecheck translation - def did_widget_selection_change(widget): + """a simple heuristic to check if the widget text contains appropriately + translated 'current'""" return not translate(" (current)") in widget.currentText() -def initialize_widget(widget, choices, selected_value=None, icon_getter=None, add_current_label=True): +def initialize_widget(widget, choices, selected_value=None, + icon_getter=None, add_current_label=True): """ populates widget (ListBox or ComboBox) with items. Previous widget contents are erased. - :param widget: widget to populate - :param choices: list of tuples (text, value) to use to populate widget - :param selected_value: value to populate widget with + :param widget: QListBox or QComboBox; must support addItem and findText + :param choices: list of tuples (text, value) to use to populate widget. + text should be a string, value can be of any type, including None + :param selected_value: initial widget value :param icon_getter: function of value that returns desired icon + :param add_current_label: if initial value should be labelled as (current) :return: """ @@ -128,7 +146,23 @@ def initialize_widget(widget, choices, selected_value=None, icon_getter=None, ad def initialize_widget_for_property( widget, choices, holder, property_name, allow_default=False, icon_getter=None, add_current_label=True): - # potentially add default + """ + populates widget (ListBox or ComboBox) with items, based on a listed + property. Supports discovering the system default for the given property + and handling qubesadmin.DEFAULT special value. Value of holder.property + will be set as current item. Previous widget contents are erased. + :param widget: QListBox or QComboBox; must support addItem and findText + :param choices: list of tuples (text, value) to use to populate widget. + text should be a string, value can be of any type, including None + :param holder: object to use as property_name's holder + :param property_name: name of the property + :param allow_default: boolean, should a position with qubesadmin.DEFAULT + be added; default False + :param icon_getter: a function applied to values (from choices) that + returns a QIcon to be used as a item icon; default None + :param add_current_label: if initial value should be labelled as (current) + :return: + """ if allow_default: default_property = holder.property_get_default(property_name) if default_property is None: @@ -150,15 +184,28 @@ def initialize_widget_for_property( add_current_label=add_current_label) -# TODO: add use icons here -def initialize_widget_with_vms(widget, - qubes_app, - filter_function=(lambda x: True), - allow_none=False, - holder=None, - property_name=None, - allow_default=False, - allow_internal=False): +# TODO: improvement: add optional icon support +def initialize_widget_with_vms( + widget, qubes_app, filter_function=(lambda x: True), + allow_none=False, holder=None, property_name=None, + allow_default=False, allow_internal=False): + """ + populates widget (ListBox or ComboBox) with vm items, optionally based on + a given property. Supports discovering the system default for the property + and handling qubesadmin.DEFAULT special value. Value of holder.property + will be set as current item. Previous widget contents are erased. + :param widget: QListBox or QComboBox; must support addItem and findText + :param qubes_app: Qubes() object + :param filter_function: function used to filter vms; optional + :param allow_none: should a None option be added; default False + :param holder: object to use as property_name's holder + :param property_name: name of the property + :param allow_default: should a position with qubesadmin.DEFAULT be added; + default False + :param allow_internal: should AdminVMs and vms with feature 'internal' be + used + :return: + """ choices = [] for vm in qubes_app.domains: @@ -171,34 +218,61 @@ def initialize_widget_with_vms(widget, if allow_none: choices.append((translate("(none)"), None)) - initialize_widget_for_property( - widget=widget, choices=choices, holder=holder, - property_name=property_name, allow_default=allow_default) + if holder is None: + initialize_widget(widget, + choices, + selected_value=choices[0][1], + add_current_label=False) + else: + initialize_widget_for_property( + widget=widget, choices=choices, holder=holder, + property_name=property_name, allow_default=allow_default) -def initialize_widget_with_default(widget, - item_list, - filter_function=(lambda x: True), - add_none=False, - add_qubes_default=False, # refers to qubesdamin.default - mark_existing_as_default=False, # needed because the default value can be none - default_value=None): - choices = [] +def initialize_widget_with_default( + widget, choices, add_none=False, add_qubes_default=False, + mark_existing_as_default=False, default_value=None): + """ + populates widget (ListBox or ComboBox) with items. Used when there is no + corresponding property, but support for special qubesadmin.DEFAULT value + is still needed. + :param widget: QListBox or QComboBox; must support addItem and findText + :param choices: list of tuples (text, value) to use to populate widget. + text should be a string, value can be of any type, including None + :param add_none: should a 'None' position be added + :param add_qubes_default: should a qubesadmin.DEFAULT position be added + (requires default_value to be set to something meaningful) + :param mark_existing_as_default: should an existing value be marked + as default. If used with conjuction with add_qubes_default, the + default_value listed will be replaced by qubesadmin.DEFAULT + :param default_value: what value should be used as the default + :return: + """ + added_existing = False - for item in item_list: - if not filter_function(item): - continue - if mark_existing_as_default and item == default_value: - choices.append((translate("default ({})").format(item), item)) - else: - choices.append((str(item), item)) + if mark_existing_as_default: + existing_default = [item for item in choices + if item[1] == default_value] + if existing_default: + choices = [item for item in choices if item not in existing_default] - if add_qubes_default: + if add_qubes_default: + # if for some reason (e.g. storage pools) we want to mark an + # actual value as default and replace it with qubesadmin.DEFAULT + default_value = qubesadmin.DEFAULT + + choices.insert( + 0, (translate("default ({})").format(existing_default[0][0]), + default_value)) + added_existing = True + + elif add_qubes_default: choices.insert(0, (translate("default ({})").format(default_value), qubesadmin.DEFAULT)) if add_none: - if mark_existing_as_default and default_value is None: + if mark_existing_as_default and default_value is None and \ + not added_existing: choices.append((translate("default (none)"), None)) else: choices.append((translate("(none)"), None)) @@ -211,16 +285,26 @@ def initialize_widget_with_default(widget, selected_value = choices[0][1] initialize_widget( - widget=widget, choices=choices, selected_value=selected_value, add_current_label=False) + widget=widget, choices=choices, selected_value=selected_value, + add_current_label=False) -def initialize_widget_with_kernels(widget, - qubes_app, - allow_none=False, - holder=None, - property_name=None, - allow_default=False - ): +def initialize_widget_with_kernels( + widget, qubes_app, allow_none=False, holder=None, + property_name=None, allow_default=False): + """ + populates widget (ListBox or ComboBox) with kernel items, based on a given + property. Supports discovering the system default for the property + and handling qubesadmin.DEFAULT special value. Value of holder.property + will be set as current item. Previous widget contents are erased. + :param widget: QListBox or QComboBox; must support addItem and findText + :param qubes_app: Qubes() object + :param allow_none: should a None item be added + :param holder: object to use as property_name's holder + :param property_name: name of the property + :param allow_default: should a qubesadmin.DEFAULT item be added + :return: + """ kernels = [kernel.vid for kernel in qubes_app.pools['linux-kernel'].volumes] kernels = sorted(kernels, key=KernelVersion) @@ -234,10 +318,18 @@ def initialize_widget_with_kernels(widget, property_name=property_name, allow_default=allow_default) -def initialize_widget_with_labels(widget, - qubes_app, - holder=None, - property_name='label'): +def initialize_widget_with_labels(widget, qubes_app, + holder=None, property_name='label'): + """ + populates widget (ListBox or ComboBox) with label items, optionally based + on a given property. Value of holder.property will be set as current item. + Previous widget contents are erased. + :param widget: QListBox or QComboBox; must support addItem and findText + :param qubes_app: Qubes() object + :param holder: object to use as property_name's holder; can be None + :param property_name: name of the property + :return: + """ labels = sorted(qubes_app.labels.values(), key=lambda l: l.index) choices = [(label.name, label) for label in labels] @@ -245,12 +337,11 @@ def initialize_widget_with_labels(widget, QtGui.QIcon.fromTheme(label.icon)) if holder: - initialize_widget_for_property( - widget=widget, - choices=choices, - holder=holder, - property_name=property_name, - icon_getter=icon_getter) + initialize_widget_for_property(widget=widget, + choices=choices, + holder=holder, + property_name=property_name, + icon_getter=icon_getter) else: initialize_widget(widget=widget, choices=choices, @@ -259,96 +350,6 @@ def initialize_widget_with_labels(widget, add_current_label=False) - -def prepare_choice(widget, holder, propname, choice, default, - filter_function=None, *, - icon_getter=None, allow_internal=None, allow_default=False, - allow_none=False, transform=None): - # for newly created vms, set propname to None - - # clear the widget, so that prepare_choice functions can be used - # to refresh widget values - while widget.count() > 0: - widget.removeItem(0) - - debug( - 'prepare_choice(widget={widget!r}, ' - 'holder={holder!r}, ' - 'propname={propname!r}, ' - 'choice={choice!r}, ' - 'default={default!r}, ' - 'filter_function={filter_function!r}, ' - 'icon_getter={icon_getter!r}, ' - 'allow_internal={allow_internal!r}, ' - 'allow_default={allow_default!r}, ' - 'allow_none={allow_none!r})'.format(**locals())) - - if propname is not None and allow_default: - default = holder.property_get_default(propname) - - if allow_internal is None: - allow_internal = propname is None or not propname.endswith('vm') - - if propname is not None: - if holder.property_is_default(propname): - oldvalue = qubesadmin.DEFAULT - else: - oldvalue = getattr(holder, propname) - if oldvalue == '': - oldvalue = None - if transform is not None and oldvalue is not None: - oldvalue = transform(oldvalue) - else: - oldvalue = object() # won't match for identity - idx = 0 - - choice_list = list(choice)[:] - if not allow_internal: - choice_list = filter(_filter_internal, choice_list) - if filter_function is not None: - choice_list = filter(filter_function, choice_list) - choice_list = list(choice_list) - - if allow_default: - choice_list.insert(0, qubesadmin.DEFAULT) - if allow_none: - choice_list.append(None) - - for i, item in enumerate(choice_list): - debug('i={} item={}'.format(i, item)) - # 0: default (unset) - if item is qubesadmin.DEFAULT: - default_string = str(default) if default is not None else 'none' - if transform is not None: - default_string = transform(default_string) - text = QtCore.QCoreApplication.translate( - "ManagerUtils", 'default ({})').format(default_string) - # N+1: explicit None - elif item is None: - text = QtCore.QCoreApplication.translate("ManagerUtils", '(none)') - # 1..N: choices - else: - text = str(item) - if transform is not None: - text = transform(text) - - if item == oldvalue: - text += QtCore.QCoreApplication.translate( - "ManagerUtils", ' (current)') - idx = i - - widget.insertItem(i, text) - - if icon_getter is not None: - icon = icon_getter(item) - if icon is not None: - widget.setItemIcon(i, icon) - - widget.setCurrentIndex(idx) - - return choice_list, idx - - class KernelVersion: # pylint: disable=too-few-public-methods # Cannot use distutils.version.LooseVersion, because it fails at handling # versions that have no numbers in them @@ -370,42 +371,6 @@ class KernelVersion: # pylint: disable=too-few-public-methods return self_content < other_content -def prepare_kernel_choice(widget, holder, propname, default, *args, **kwargs): - try: - app = holder.app - except AttributeError: - app = holder - kernels = [kernel.vid for kernel in app.pools['linux-kernel'].volumes] - kernels = sorted(kernels, key=KernelVersion) - - return prepare_choice( - widget, holder, propname, kernels, default, *args, **kwargs) - - -def prepare_label_choice(widget, holder, propname, default, *args, **kwargs): - try: - app = holder.app - except AttributeError: - app = holder - - return prepare_choice(widget, holder, propname, - sorted(app.labels.values(), key=lambda l: l.index), - default, *args, - icon_getter=(lambda label: - QtGui.QIcon.fromTheme(label.icon)), - **kwargs) - - -def prepare_vm_choice(widget, holder, propname, default, *args, **kwargs): - try: - app = holder.app - except AttributeError: - app = holder - - return prepare_choice(widget, holder, propname, app.domains, default, - *args, **kwargs) - - def is_debug(): return os.getenv('QUBES_MANAGER_DEBUG', '') not in ('', '0')