-
-
Notifications
You must be signed in to change notification settings - Fork 827
Implementation of Scaffolds layer for Apple platforms, with related fixes #4605
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.
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
base: scaffolds
Are you sure you want to change the base?
Changes from 19 commits
89c90b7
e815479
d8b442c
795ab0a
a4220a2
622047b
2c5cc8b
d8f7f99
a875a76
c9b6dab
c339fd5
5bc2347
46bbfe3
0fcd4bd
ea22cca
6b9edba
1ccaa2d
8547f6a
57da14c
a87b57d
e2cdee8
43190a9
31e6658
2d91c98
559e059
c451112
f771f5c
638a842
d898218
af25698
2d87273
5bdaa52
c5aaebc
3500df8
520e956
4e115d7
c70170d
148751b
354be05
6cd0fec
2a6795a
85b8df2
a3fc6e8
38be61c
52be318
d9ef0c5
5e6d578
3ec08c7
e6e8b80
566cdb8
b66d507
e0319b5
9c03fc9
9189aa2
ed3c4d6
49feb37
536ecb1
c51995f
9030782
f543b35
2cbe1de
898e48f
3257570
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,181 @@ | ||
| from rubicon.objc import SEL, NSObject, objc_method, objc_property | ||
|
|
||
| from toga.command import Command, Separator | ||
| from toga_cocoa.container import ControlledContainer | ||
| from toga_cocoa.libs import NSMutableArray, NSToolbar, NSToolbarItem | ||
|
|
||
|
|
||
| def toolbar_identifier(cmd): | ||
| return f"Toolbar-{type(cmd).__name__}-{id(cmd)}" | ||
|
|
||
|
|
||
| class ToolbarDelegate(NSObject): | ||
| interface = objc_property(object, weak=True) | ||
| impl = objc_property(object, weak=True) | ||
|
|
||
| @objc_method | ||
| def toolbarAllowedItemIdentifiers_(self, toolbar): # pragma: no cover | ||
| """Determine the list of available toolbar items.""" | ||
| allowed = NSMutableArray.alloc().init() | ||
| for item in self.impl.toolbar_commands: | ||
| allowed.addObject_(toolbar_identifier(item)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should take this opportunity to clear up any vestigial method invocations - once upon a time, the trailing underscore was required, but we fixed that many years ago.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done; will push |
||
| return allowed | ||
|
|
||
| @objc_method | ||
| def toolbarDefaultItemIdentifiers_(self, toolbar): | ||
| """Determine the list of toolbar items that will display by default.""" | ||
| default = NSMutableArray.alloc().init() | ||
| prev_group = None | ||
| for item in self.impl.toolbar_commands: | ||
| if ( | ||
| prev_group is not None | ||
| and item.group != prev_group | ||
| and not isinstance(item, Separator) | ||
| ): | ||
| default.addObject_(toolbar_identifier(prev_group)) | ||
| default.addObject_(toolbar_identifier(item)) | ||
| prev_group = item.group | ||
|
|
||
| return default | ||
|
|
||
| @objc_method | ||
| def toolbar_itemForItemIdentifier_willBeInsertedIntoToolbar_( | ||
| self, | ||
| toolbar, | ||
| identifier, | ||
| insert: bool, | ||
| ): | ||
| """Create the requested toolbar button.""" | ||
| native = NSToolbarItem.alloc().initWithItemIdentifier_(identifier) | ||
| try: | ||
| item = self.impl._toolbar_items[str(identifier)] | ||
| native.setLabel(item.text) | ||
| native.setPaletteLabel(item.text) | ||
| if item.tooltip: | ||
| native.setToolTip(item.tooltip) | ||
| if item.icon: | ||
| native.setImage(item.icon._impl.native) | ||
|
|
||
| item._impl.native.add(native) | ||
|
|
||
| native.setTarget_(self) | ||
| native.setAction_(SEL("onToolbarButtonPress:")) | ||
| except KeyError: # Separator items | ||
| pass | ||
|
|
||
| return native | ||
|
|
||
| @objc_method | ||
| def validateToolbarItem_(self, item) -> bool: | ||
| """Confirm if the toolbar item should be enabled.""" | ||
| try: | ||
| return self.impl._toolbar_items[str(item.itemIdentifier)].enabled | ||
| except KeyError: # pragma: nocover | ||
| return False | ||
|
|
||
| @objc_method | ||
| def onToolbarButtonPress_(self, obj) -> None: | ||
| """Invoke the action tied to the toolbar button.""" | ||
| item = self.impl._toolbar_items[str(obj.itemIdentifier)] | ||
| item.action() | ||
|
|
||
|
|
||
| class Scaffold: | ||
| def __init__(self, interface): | ||
| self.interface = interface | ||
| self.container = ControlledContainer(on_refresh=self.content_refreshed) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why break this into a separate class, rather than having Scaffold have both a container and a controller? I'm not seeing any particular benefits, other than longer attribute access chains...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implementing SidebarScaffold and OptionScaffold requires the use of controllers for each individual 'tab' of content. So, if we have a class that conceptually bundles a view and a controller together, we can reuse it in future places, and when we do so we no longer have to keep track of content/controller separately when there's multiple content/controller pairs required. Doing ControlledContainer also maintains parity with iOS. Would you prefer this type of refactor to be done in a later PR, though? |
||
| self.root_controller = self.container.controller | ||
| self._toolbar_items = {} | ||
| self._toolbar_commands = [] | ||
| self.native_toolbar = None | ||
| self.toolbar_delegate = ToolbarDelegate.alloc().init() | ||
| self.toolbar_delegate.impl = self | ||
| self.toolbar_delegate.interface = self.interface | ||
|
|
||
| def __del__(self): | ||
| self.purge_toolbar() | ||
|
|
||
| @property | ||
| def current_container(self): | ||
| return self.container | ||
|
|
||
| def set_content(self, widget): | ||
| self.container.content = widget | ||
|
|
||
| @property | ||
| def title(self): | ||
| return self.container.controller.title | ||
|
|
||
| @title.setter | ||
| def title(self, value): | ||
| self.container.controller.title = value | ||
|
|
||
| def refresh(self): | ||
| if self.container.content: | ||
| self.container.content.interface.refresh() | ||
|
|
||
| @property | ||
| def toolbar_commands(self): | ||
| return self._toolbar_commands | ||
|
|
||
| # def notify_toolbar_change(self): | ||
| # window = self.interface.window | ||
| # if window is not None and getattr(window, "_impl", None) is not None: | ||
| # window._impl.update_toolbar(self) | ||
|
|
||
| def create_toolbar(self): | ||
| window = self.interface.window | ||
| self.purge_toolbar() | ||
|
|
||
| # Shouldn't happen in normal operations, but just in case | ||
| if window is None: # pragma: no cover | ||
| self.native_toolbar = None | ||
| self._toolbar_commands = [] | ||
| return | ||
|
|
||
| self._toolbar_commands = [] | ||
| if hasattr(window, "toolbar"): | ||
| self._toolbar_commands.extend(window.toolbar) | ||
|
|
||
| self._toolbar_items = {} | ||
| for cmd in self._toolbar_commands: | ||
| if isinstance(cmd, Command): | ||
| self._toolbar_items[toolbar_identifier(cmd)] = cmd | ||
|
|
||
| if self._toolbar_commands: | ||
| self.native_toolbar = NSToolbar.alloc().initWithIdentifier( | ||
| f"Toolbar-{id(self)}" | ||
| ) | ||
| self.native_toolbar.setDelegate(self.toolbar_delegate) | ||
| else: | ||
| self.native_toolbar = None | ||
|
|
||
| if window.content: | ||
| window.content.refresh() | ||
|
|
||
| def purge_toolbar(self): | ||
| window = self.interface.window | ||
|
|
||
| # Defensive measure | ||
| if window is None: # pragma: no cover | ||
| return | ||
|
|
||
| while self._toolbar_items: | ||
| dead_items = [] | ||
| _, cmd = self._toolbar_items.popitem() | ||
| # Only purge items associated with the current scaffold's | ||
| # toolbar delegate. This ensures proper cleanup. | ||
| for item_native in cmd._impl.native: | ||
| if ( | ||
| isinstance(item_native, NSToolbarItem) | ||
| and item_native.target == self.toolbar_delegate | ||
| ): | ||
| dead_items.append(item_native) | ||
|
|
||
| for item_native in dead_items: | ||
| cmd._impl.native.remove(item_native) | ||
|
|
||
| def content_refreshed(self, container): | ||
| # Apply the minimum size. This will autoresize the window if needed. | ||
| self.container.min_width = self.interface.content.layout.min_width | ||
| self.container.min_height = self.interface.content.layout.min_height | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving all the toolbar handling into scaffold implementation was a hard choice, since there's some additional bookeeping. But since we're already refactoring stuff here, I think we should do it now so we aren't scrambling to refactor the architecture when things like
SidebarScaffoldorOptionScaffoldstarts to declare their own toolbar items.On macOS, when an app has a sidebar, the toolbar is displayed inside the right pane, and the actions can depend on sidebar selections. The toolbar belongs to the scaffold's content pane visually and funcitonally, despite there only being a single native toolbar at the level of the window. This is also not a Liquid Glass quirk, and has been present for many versions of macOS.
But also, #4298 established that certain scaffold types can contribute items to the window toolbar, so Scaffolds will need to manage and create toolbar directly on macOS.
The alternative would be to for OptionScaffold or SidebarScaffold to hook into the Window-level toolbar instance instead, but then we'd have to handle the scaffold signaling the window to modify its toolbar items, which gets messy fast. So I've made this decision here. Is this appropriate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow why it's more messy. There's a Window-Scaffold communication issue either way.
In the macOS case specifically, it sounds like you're concerned that the Sidebar scaffold has/can have a toolbar that isn't the full width. However, AFAICT, that's a separate entity to the window's toolbar. For several releases, macOS has put "toolbar" items in the titlebar of the app.
The key detail for me - even in the SidebarScaffold or OptionScaffold world, the API for adding a toolbar in macOS is going to be
window.setToolbar(). Looking at the API for NSSplitViewController - there's no toolbar properties that I can see; the toolbar is still being set on the Window.It feels to me like you're convolving "how is the toolbar implemented" with "where are the toolbar items defined". In the case of macOS, the toolbar implementation is bound to the Window. It may ultimately need to interrogate the scaffold to determine some or all of the toolbar items - but that's more of a "get the initial toolbar contents on creation, update on notable UI event" task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I thought was that each scaffold could own one instance of the toolbar and the Window will just use its scaffold's toolbar instance. But turns out that we were recreating the toolbar instance each time we have an update, so yes, cross-signaling is still required.
Most definitely yes. Thanks for catching my conceptual misunderstanding.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll revert the placement of code here.
EDIT:: Sorry, typo. I meant I had reversed the placement of code here, but haven't pushed yet. Treat this as a done comment.