Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add persistence for unit values in SVG #1144

Closed
wants to merge 3 commits into from
Closed

Add persistence for unit values in SVG #1144

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4bb8468
Add persistence for unit values in SVG
lisham2000 Aug 20, 2024
608d2a1
Change to use string persistent model in persistence of units within …
lisham2000 Aug 27, 2024
d0d66f0
Change to use string persistent model in persistence of units within …
lisham2000 Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions nion/swift/ExportDialog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,15 +261,18 @@ class UnitType(enum.Enum):


class ExportSizeModel(Observable.Observable):
def __init__(self, display_item: DisplayItem.DisplayItem) -> None:
def __init__(self, display_item: DisplayItem.DisplayItem, units_persistent_model: UserInterface.StringPersistentModel) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, you can just call it units_model: Model.PropertyModel[str] instead of the more specific units_persistent_model: UserInterface.StringPersistentModel. When receiving parameters, it's better to take the more general case instead of requiring the more specific. It makes testing easier. You could also consider adding a test here to avoid the bug that you'll discover when merging the latest changes on master to this branch.

super().__init__()
self.__units_persistent_model = units_persistent_model
display_size = self.__calculate_display_size_in_pixels(display_item)
self.__width = display_size.width
self.__height = display_size.height
self.__aspect_ratio = self.__width / self.__height
self.__units = UnitType.PIXELS
self.__float_to_string_converter = Converter.FloatToStringConverter()
self.__primary_field = 'width' # Primary field to determine which text is calculated
self.__primary_field = 'width' # Primary field to determine which text is calculater
# Load the persisted units value, default to PIXELS if not found
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# Load the persisted units value, default to PIXELS if not found

This comment is outdated and can be removed. The default value is set by the caller.

persisted_units = self.__units_persistent_model.value
self.__units = UnitType[str(persisted_units)]
Comment on lines +274 to +275
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of notes here (minor points, depending on how you look at it):

  • if you were to keep this name, can you use persistent_units rather than the past tense persisted
  • instead of making the assumption that these are persisted, it would be better to just assume the value is coming from the model, which is a string, and so should be named units_str

A more difficult problem that is slightly trickier: we may want to expand the list of units in the future. If we do that and then try to load a file in this version, it may have persistent units that aren't recognized in this version and will throw an exception on line 275 above.

The way I usually like to deal with this is by creating a separate map and reverse map for converting between the enum and the persistent value. Something along these lines (I didn't run this, but it's close):

unit_to_str_map = { UnitType.PIXELS: "pixels", UnitType.INCHES: "inches", UnitType.CENTIMETERS: "centimeters" }
str_to_unit_map = { "pixels": UnitType.PIXELS, "inches": UnitType.INCHES, "centimeters": UnitType.CENTIMETERS 

self.__units = str_to_unit_map.get(units_str, UnitType.PIXELS)
self.__units_model.value = unit_to_str_map[self.__units]

Also, this avoids tying the file format to Python code (the enum.name).


def __calculate_display_size_in_pixels(self, display_item: DisplayItem.DisplayItem) -> Geometry.IntSize:
if display_item.display_data_shape and len(display_item.display_data_shape) == 2:
Expand Down Expand Up @@ -327,6 +330,7 @@ def units(self, new_units: int) -> None:
new_enum = UnitType(new_units)
if self.__units != new_enum:
self.__units = new_enum
self.__units_persistent_model.value = self.__units.name
self.notify_property_changed("width")
self.notify_property_changed("height")
self.notify_property_changed("width_text")
Expand Down Expand Up @@ -391,7 +395,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_persistent_model = UserInterface.StringPersistentModel(
ui=document_controller.ui,
storage_key="export_units",
value=UnitType.PIXELS.name # Default value if not found
)
self.__model = ExportSizeModel(display_item, self.__units_persistent_model)
self.__handler = ExportSVGHandler(self.__model)
self.__init_ui()

Expand Down