Update branch to align and add in str to unit maps and update variable names #1171
Conversation
nion/swift/ExportDialog.py
Outdated
| "centimeters": UnitType.CENTIMETERS | ||
| } | ||
|
|
||
| def __init__(self, display_item: DisplayItem.DisplayItem, units_model: UserInterface.StringPersistentModel) -> None: |
There was a problem hiding this comment.
unit_model should be a more general type here, e.g. Model.PropertyModel[str] - that would allow us to change the model used in the future without having to alter this function
| @@ -423,7 +438,12 @@ def __init__(self, document_controller: DocumentController.DocumentController, | |||
| super().__init__() | |||
| self.__document_controller = document_controller | |||
| self.__display_item = display_item | |||
| self.__model = ExportSizeModel(display_item) | |||
| self.__units_model = UserInterface.StringPersistentModel( | |||
There was a problem hiding this comment.
Can you create the persistent model using ui.create_persistent_string_model instead of instantiating it directly to be consistent with other similar usages throughout the code. Also can you qualify the storage_key further, something like export_svg_dialog_units instead of export_units?
| self.__float_to_string_converter = Converter.FloatToStringConverter() | ||
| self.__primary_field = 'width' | ||
| units_str: str = str(self.__units_model.value) |
There was a problem hiding this comment.
This should be units_str = self.__units_model.value or str() since value may be None. Also, you don't need the type specifier in the original code (or the new code) since the type checker can determine the type from context.
| self.__last_input_units = self.__units | ||
| self.__last_input_value: float = display_size.width/ConversionUnits[self.__last_input_units] |
There was a problem hiding this comment.
There should be spaces around the / -> / to make it readable.
| @@ -366,6 +380,7 @@ def units(self, new_units: int) -> None: | |||
| new_enum = UnitType(new_units) | |||
| if self.__units != new_enum: | |||
| self.__units = new_enum | |||
| self.__units_model.value = self.unit_to_str_map[self.__units] | |||
There was a problem hiding this comment.
The self.__units could just be eliminated. The value can be computed on the fly using UnitType(self.__units_model.value or 0) and set directly. Watch out for None values when making that change, though. You could add a new property named _units for use within this class.
I think as part of that change, there might need to be another property units_index and that new property should be the one that is used for binding in ExportSVGHandler. It's confusing as it is now to have units property referring to an int and self.__units referring to an enum.
There are many ways to handle this, but it needs to be cleaned up and clear to someone reading the code.
| @@ -261,15 +261,29 @@ class UnitType(enum.Enum): | |||
|
|
|||
|
|
|||
| class ExportSizeModel(Observable.Observable): | |||
| def __init__(self, display_item: DisplayItem.DisplayItem) -> None: | |||
| unit_to_str_map = { | |||
There was a problem hiding this comment.
I know this same thing is done in other parts of the code, but I think we should use better naming for these variables - unit_to_unit_identifier_map and unit_identifier_to_unit_map are verbose, but explain better what they are. str is too general and is confusing with the titles used in ExportSVGHandler.
No description provided.