Refactored Export Dialog in Declarative Mode #1055
Conversation
Fixed the bug with no checkboxes led to duplicate filenames, stopping all duplicate names happening. Reimplemented in Declarative UI model. Extracted the settings to a dataclass that can be re-used for scripting. Extracted the export functionality to a static function that can be re-used for scripting.
Added handler base class, specified types for PropertyModel
There was a problem hiding this comment.
Thanks for the PR - this is a good start and heading in the right direction. It will be nice to have the export dialog be declarative once we get this merged.
I'm adding some individual comments - but would it be possible to split this PR into two: (1) switching the dialog to declarative; and (2) the fix for avoiding duplicate file names?
I've added comments on both parts here, but splitting it into two PR's will make the submit-review-edit cycle easier. I'll have additional comments on the declarative part once it is split into its own PR.
nion/swift/ExportDialog.py
Outdated
| self.ui_view = column | ||
|
|
||
| @staticmethod | ||
| def build_filename(components: typing.List[str], extension: str, suffix: str = "($)", path: str = "") -> str: |
There was a problem hiding this comment.
The suffix argument is unused. This looks partially implemented. So... when splitting into the two PR's, you can probably just drop it. Then let's discuss the unique filename argument. I'd rather see a slightly more holistic approach where we keep track of what filenames have been used and only then make the subsequent matching filenames unique. I think it's difficult to do it with a single build_filename method unless a dict of used base filenames and their count is passed in.
There was a problem hiding this comment.
The suffix bit was part over-engineering on my part in case we wanted to change the format for how to identify the duplicate (instead of just (1) for example).
Do you want to have the discussion in the original Issue, a separate conversation on Teams, or what?
There was a problem hiding this comment.
We can discuss here; but at this point, it's unused code, so it can be removed from this PR, right?
There was a problem hiding this comment.
Ok so the suffix part has been removed as you are correct, it is unused. Onto the actual unique filename bit.
So the requirement as I understood it was to make sure that when exporting files to not automatically overwrite so the user did not lose data, especially in the case where the parameters selected meant that multiple exported images would have the same filename. The simplest solution to this is to follow the Windows paradigm where if attempting to create a file with the same name to simply add an incrementing suffix in brackets after the filename.
You have mentioned some dictionary of data items to filenames, but I have some questions about that. Does it actually provide any extra benefit over the standard Windows method? How should it handle already existing files in that directory that are not in that dictionary?
nion/swift/ExportDialog.py
Outdated
| # file must already exist | ||
| if suffix and '$' in suffix: | ||
| found_available = False | ||
| next_index = 1 | ||
| max_index = 999 | ||
| while not found_available and next_index <= max_index: | ||
| test_suffix = suffix.replace("$", str(next_index)) | ||
| test_filename = filename + test_suffix + extension | ||
| if path: | ||
| test_filename = path + test_filename | ||
| if not os.path.exists(test_filename): | ||
| return test_filename | ||
| next_index = next_index + 1 |
There was a problem hiding this comment.
This seems weird. But see comment above.
Removing unneeded type declarations, and the variable suffix type.
|
@Tiomat85 Thanks for the PR - looks overall good - although I ended up making more changes. Since the changes were more than seemed sensible in the comments, I made a new PR with my changes, and added detailed explanations here: If you're happy with the additional changes on the new PR, please close this PR and we'll work from the other one instead. |
#586 Fixed the bug with no checkboxes led to duplicate filenames, stopping all duplicate names happening. Reimplemented in Declarative UI model.
Extracted the settings to a dataclass that can be re-used for scripting. Extracted the export functionality to a static function that can be re-used for scripting.