Restore ability to do quick single exports #1173
Conversation
lisham2000
requested review from
Ion-e and
Tiomat85
and removed request for
Ion-e
Adjust unit test to adhere to new menu dropdowns
lisham2000
force-pushed
the
RestoreSingleExports
branch
from
d5e70ff
to
ea1b8e2
Compare
| "action_id": "file.export_svg" | ||
| "type": "sub_menu", | ||
| "menu_id": "file_export", | ||
| "title": "Export...", |
There was a problem hiding this comment.
Sub-menus should not have "..." on them. But I don't think these should go into a sub-menu, at least for now.
As part of this change, though, we could update the context menu to supply "single export" and "export svg". That's done here:
nionswift/nion/swift/DocumentController.py
Line 2641 in 6f71ebc
There was a problem hiding this comment.
Hi Chris, Can I ask why we wouldn't want to group these in a sub menu? The spec in #1134 James talks about a sub menu with items so i may have interpreted this wrong
There was a problem hiding this comment.
I'm not 100% opposed to it, but the File menu is relatively short, so adding an item isn't a big change. The downside of the sub menu is it is (a) less visible to users exploring the menus (b) one extra click/mouse-motion to select.
I actually think it might be nice to have them all visible with a separator between the 'Import' items and the 'Export' items. This would be the addition of two lines in the menu.
If your group has a strong opinion, it's fine. But if not, let's go with no sub-menu with the option of putting them in a sub-menu in another PR.
nion/swift/DocumentController.py
Outdated
| action_id = "file.export" | ||
| action_name = _("Export...") | ||
| class SingleExportAction(Window.Action): | ||
| action_id = "export.single_export" |
There was a problem hiding this comment.
The old action_id should not change. It's unlikely that it is used, but the action id's are meant to be stable over long periods of time. Likewise, I'd like to use consistent identifiers, even though your identifier names are better. That means using file.batch_export or maybe file.export_batch to make it easily distinguishable in an alphabetized list.
Along those same lines, the classes should be in alphabetical order in the file. This is to make it easier to locate them, which is a problem in the huge file. To that end, I'd keep the original name along with the original identifier (for the reasons above): ExportAction or perhaps ExportSingleAction.
There was a problem hiding this comment.
Can I ask the silly question of why do classes need to be alphabetical in a file? Most modern IDEs (including PyCharm) can list classes/functions in a file in alphabetical order anyway. If a file has enough classes in it that that list isn't enough surely that is indicative that the file could do with being broken down into smaller chunks rather than trying to use the first few letters of a class name to create pseudo-structure - potentially leading to names that are less descriptive of the actual function and instead are named just to keep them together.
There was a problem hiding this comment.
They're not really totally alphabetical - only within each major section. It's simply something to make it easier when searching for commands. This file should definitely be broken down into more manageable pieces and now might be a sensible time to do that (just before a release). Or maybe just after. Either way, when a file gets split, it's more difficult to get a history of that file for regressions, so I tend not to do it for that reason.
There was a problem hiding this comment.
NOTE: the important part of my comment is to not rename the existing command.
nion/swift/DocumentController.py
Outdated
| @@ -3080,13 +3099,13 @@ def invoke(self, context: Window.ActionContext) -> Window.ActionResult: | |||
|
|
|||
| Window.register_action(DeleteItemAction()) | |||
| Window.register_action(DeleteDataItemAction()) | |||
| Window.register_action(ExportAction()) | |||
| Window.register_action(SingleExportAction()) | |||
There was a problem hiding this comment.
These should appear in alphabetical order within their group.
nion/swift/DocumentController.py
Outdated
| return len(context.display_items) == 1 and context.display_item is not None | ||
|
|
||
|
|
||
| class BatchExportAction(Window.Action): |
There was a problem hiding this comment.
These should appear in alphabetical order, and I'd rename it to ExportBatchAction to make sure it's located in the same area of the code as ExportAction when alphabetized.
|
Merged with minor edits 3e084a5 |
No description provided.