-
-
Notifications
You must be signed in to change notification settings - Fork 826
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 all 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,33 @@ | ||
| from toga_cocoa.container import ControlledContainer | ||
|
|
||
|
|
||
| class Scaffold: | ||
| def __init__(self, interface): | ||
| self.interface = interface | ||
| self.container = ControlledContainer(on_refresh=self.content_refreshed) | ||
| # Expose the root controller for the window to embed | ||
| self.root_controller = self.container.controller | ||
|
|
||
| @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() | ||
|
|
||
| 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -190,16 +190,19 @@ def set_background_color(self, color): | |
| self.native_input.bezeled = True | ||
| self.native_input.backgroundColor = native_color(color) | ||
|
|
||
| @property | ||
| def has_focus(self): | ||
| # When the NSTextField gets focus, a field editor is created, and that editor | ||
| # has the original widget as the delegate. The first responder is the | ||
| # Field Editor. | ||
| return isinstance(self.native.window.firstResponder, NSTextView) and ( | ||
| self.native.window.firstResponder.delegate == self.native_input | ||
| return ( | ||
| self.native.window is not None | ||
| and isinstance(self.native.window.firstResponder, NSTextView) | ||
| and (self.native.window.firstResponder.delegate == self.native_input) | ||
| ) | ||
|
|
||
| def focus(self): | ||
| if not self.has_focus(): | ||
| if self.interface.window and not self.has_focus: | ||
| self.interface.window._impl.native.makeFirstResponder(self.native_input) | ||
|
|
||
| def get_readonly(self): | ||
|
|
@@ -224,7 +227,13 @@ def set_max_value(self, value): | |
| self.native_stepper.maxValue = float(value) | ||
|
|
||
| def set_text_align(self, value): | ||
| if self.has_focus: | ||
| # Drop focus if we're currently focussed, or else alignment setting | ||
| # will not work properly with Cocoa | ||
| self.interface.window._impl.native.makeFirstResponder(None) | ||
| self.native_input.alignment = NSTextAlignment(value) | ||
|
Comment on lines
229
to
234
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. Setting a contentViewController on window now causes Cocoa to refocus the first focussable widget on the window when a new scaffold is set. I think this is the correct behavior; the same occured previously when one showed a window, and the initial widget is focusseed. Now, this causes some complications with testing, and the result was that when we test input widgets, the input widgets are now focused even on alignment tests. This revealed a bug in the Cocoa backend, since Cocoa does not allow focussed widgets to change alignment. I've thus made set_text_align defocus first if neccessary across the input widgets we have in Toga. I made the choice to defocus because changing text alignment when someone is editing in an input is likely not good UI anyways, and so there's no good expectation that when we change display settings of the widget it should stay focussed.
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. This strikes me more as a feature/bug of the testbed, rather than the widget. When you say that changing alignment "doesn't work" - do you mean that the new alignment isn't applied at all? Or it isn't applied until focus is lost? Does it refuse to apply the property value? Raise an error? My main concern here is that it isn't at all intuitive to me that from an external user's perspective, changing alignment on a text widget would cause focus to be lost. If we're going to drop focus, it seems to me like we should be reclaiming it once the alignment has been set.
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.
Yep, new alignment isn't applied at all, and the alignment value remains the old one.
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. I've added and pushed hte code to reapply focus. |
||
| # Refocus after we're done. | ||
| self.focus() | ||
|
|
||
| def set_font(self, font): | ||
| self.native_input.font = font._impl.native | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,14 +12,14 @@ | |
| from toga.constants import WindowState | ||
| from toga.types import Position, Size | ||
| from toga.window import _initial_position | ||
| from toga_cocoa.container import Container | ||
| from toga_cocoa.libs import ( | ||
| NSBackingStoreBuffered, | ||
| NSImage, | ||
| NSMutableArray, | ||
| NSMutableDictionary, | ||
| NSNumber, | ||
| NSScreen, | ||
| NSTitleBinding, | ||
| NSToolbar, | ||
| NSToolbarItem, | ||
| NSWindow, | ||
|
|
@@ -135,7 +135,7 @@ def toolbarAllowedItemIdentifiers_(self, toolbar): # pragma: no cover | |
| # can't ever be invoked - but we need to provide an implementation. | ||
| allowed = NSMutableArray.alloc().init() | ||
| for item in self.interface.toolbar: | ||
| allowed.addObject_(toolbar_identifier(item)) | ||
| allowed.addObject(toolbar_identifier(item)) | ||
| return allowed | ||
|
|
||
| @objc_method | ||
|
|
@@ -151,8 +151,8 @@ def toolbarDefaultItemIdentifiers_(self, toolbar): | |
| and item.group != prev_group | ||
| and not isinstance(item, Separator) | ||
| ): | ||
| default.addObject_(toolbar_identifier(prev_group)) | ||
| default.addObject_(toolbar_identifier(item)) | ||
| default.addObject(toolbar_identifier(prev_group)) | ||
| default.addObject(toolbar_identifier(item)) | ||
| prev_group = item.group | ||
|
|
||
| return default | ||
|
|
@@ -167,7 +167,7 @@ def toolbar_itemForItemIdentifier_willBeInsertedIntoToolbar_( | |
| """Create the requested toolbar button.""" | ||
| try: | ||
| item = self.impl._toolbar_items[str(identifier)] | ||
| native = NSToolbarItem.alloc().initWithItemIdentifier_(identifier) | ||
| native = NSToolbarItem.alloc().initWithItemIdentifier(identifier) | ||
| native.setLabel(item.text) | ||
| native.setPaletteLabel(item.text) | ||
| if item.tooltip: | ||
|
|
@@ -208,7 +208,7 @@ def onToolbarButtonPress_(self, obj) -> None: | |
|
|
||
|
|
||
| class Window: | ||
| def __init__(self, interface, title, position, size): | ||
| def __init__(self, interface, position, size): | ||
| self.interface = interface | ||
| self.interface._impl = self | ||
|
|
||
|
|
@@ -230,6 +230,12 @@ def __init__(self, interface, title, position, size): | |
| backing=NSBackingStoreBuffered, | ||
| defer=False, | ||
| ) | ||
| self.native.bind( | ||
| NSTitleBinding, | ||
| toObject=self.native, | ||
| withKeyPath="contentViewController.title", | ||
| options=None, | ||
| ) | ||
|
Comment on lines
+233
to
+238
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. Some controllers like NSTabViewController will make the controller's title property that of the currently selected tab, so making controllers manage window content will be convenient when we implement other scaffolds later. This also makes controller/title behavior more symmetric with iOS backend. |
||
| self.native.interface = self.interface | ||
| self.native.impl = self | ||
|
|
||
|
|
@@ -242,29 +248,20 @@ def __init__(self, interface, title, position, size): | |
| # Pending Window state transition variable: | ||
| self._pending_state_transition = None | ||
|
|
||
| self.set_title(title) | ||
| self.set_size(size) | ||
| self.set_position(position if position is not None else _initial_position()) | ||
|
|
||
| self.native.delegate = self.native | ||
|
|
||
| self.container = Container(on_refresh=self.content_refreshed) | ||
| self.native.contentView = self.container.native | ||
|
|
||
| # Ensure that the container renders it's background in the same color as the | ||
| # window. | ||
| self.native.wantsLayer = True | ||
| self.container.native.backgroundColor = self.native.backgroundColor | ||
|
|
||
| ###################################################################### | ||
| # Window properties | ||
| ###################################################################### | ||
|
|
||
| def get_title(self): | ||
| return str(self.native.title) | ||
| return str(self._scaffold.title) | ||
|
|
||
| def set_title(self, title): | ||
| self.native.title = title | ||
| self._scaffold.title = title | ||
|
|
||
| ###################################################################### | ||
| # Window lifecycle | ||
|
|
@@ -287,34 +284,31 @@ def show(self): | |
| # Window content and resources | ||
| ###################################################################### | ||
|
|
||
| def content_refreshed(self, container): | ||
| min_width = self.interface.content.layout.min_width | ||
| min_height = self.interface.content.layout.min_height | ||
|
|
||
| # If the minimum layout is bigger than the current window, | ||
| # increase the size of the window. | ||
| def set_scaffold(self, scaffold): | ||
| restore_presentation = False | ||
| if self.get_window_state() == WindowState.PRESENTATION: | ||
| restore_presentation = True | ||
| self.set_window_state(WindowState.NORMAL) | ||
|
Comment on lines
+289
to
+291
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. This (along with restoring back to PRESENTAITON) at the end was required because PRESENTATION operates on the underlying container, not on the window object itself. A scaffold assignment is a change in container, so we must operate in non-PRESENTATION states when we set scaffold, and then restore PRESENTATION later using the newer container. Fortunately the way we implement PRESENTATION implies that it's synchronous so saves a lot of headaches here.
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. Or... we could prohibit changing the scaffold while in presentation mode...
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. The logic here isn't extremely complex, and this is more of a macOS-specific quirk of how we implement PRESENTATION. So I'm inclined to just allow this possibility. I'll leave the final decision to you on this. |
||
| frame = self.native.frame | ||
| if frame.size.width < min_width and frame.size.height < min_height: | ||
| self.set_size((min_width, min_height)) | ||
| elif frame.size.width < min_width: | ||
| self.set_size((min_width, frame.size.height)) | ||
| elif frame.size.height < min_height: | ||
| self.set_size((frame.size.width, min_height)) | ||
|
Comment on lines
-297
to
-302
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. FWIW: This piece of code was gotten rid of when it was moved to scaffolds/base because the constraints set on the container will enforce this automatically if the existing window size is small, so this was just unneccessary. |
||
|
|
||
| self.container.min_width = min_width | ||
| self.container.min_height = min_height | ||
|
|
||
| def set_content(self, widget): | ||
| # Get the current title and sync it up with the new scaffold. | ||
| # This check is required as the initial scaffold set will not have | ||
| # a previous scaffold to grab title from. | ||
| if hasattr(self, "_scaffold"): | ||
| scaffold.title = self.get_title() | ||
| self._scaffold = scaffold | ||
| # Set the content of the window's container | ||
| self.container.content = widget | ||
| self.native.contentViewController = scaffold.root_controller | ||
| self.native.setFrame(frame, display=True, animate=False) | ||
|
johnzhou721 marked this conversation as resolved.
|
||
| if restore_presentation: | ||
| self.set_window_state(WindowState.PRESENTATION) | ||
|
|
||
| ###################################################################### | ||
| # Window size | ||
| ###################################################################### | ||
|
|
||
| def get_size(self) -> Size: | ||
| if self.interface.state == WindowState.PRESENTATION: | ||
| native_frame = self.container.native.frame | ||
| native_frame = self._scaffold.current_container.controller.view.frame | ||
| else: | ||
| native_frame = self.native.frame | ||
| return Size(int(native_frame.size.width), int(native_frame.size.height)) | ||
|
|
@@ -384,7 +378,12 @@ def get_visible(self): | |
| def get_window_state(self, in_progress_state=False): | ||
| if in_progress_state and self._pending_state_transition: | ||
| return self._pending_state_transition | ||
| if self.container.native.isInFullScreenMode(): | ||
| # Set scaffold will call get_window_state and back then during init there | ||
| # may not be any scaffold yet so we need to check the first condition | ||
| if ( | ||
| hasattr(self, "_scaffold") | ||
|
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. When is this condition not true?
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. set_scaffold checks for presentation mode, so this is needed here. Documented inline. |
||
| and self._scaffold.current_container.controller.view.isInFullScreenMode() | ||
| ): | ||
| return WindowState.PRESENTATION | ||
| elif self.native.styleMask & NSWindowStyleMask.FullScreen: | ||
| return WindowState.FULLSCREEN | ||
|
|
@@ -469,22 +468,25 @@ def _apply_state(self, target_state): | |
| # window._impl.native.contentView is window._impl.container.native. | ||
| # Hence, we need to go fullscreen on window._impl.container.native | ||
| # instead. | ||
| self.container.native.enterFullScreenMode( | ||
| self._scaffold.current_container.controller.view.enterFullScreenMode( | ||
| self.interface.screen._impl.native, withOptions=opts | ||
| ) | ||
|
|
||
| # Going presentation mode causes the window content to be re-homed in a | ||
| # NSFullScreenWindow; Teach the new parent window about its Toga | ||
| # Going presentation mode causes the window content to be re-homed in | ||
| # a NSFullScreenWindow; Teach the new parent window about its Toga | ||
| # representations. | ||
| self.container.native.window._impl = self | ||
| self.container.native.window.interface = self.interface | ||
| self._scaffold.current_container.controller.view.window._impl = self | ||
| self._scaffold.current_container.controller.view.window.interface = ( | ||
| self.interface | ||
| ) | ||
| # Manually trigger the resize event as the original NSWindow's size | ||
| # remains unchanged, hence the windowDidResize_ would not be notified | ||
| # when the window goes into presentation mode. | ||
| self.interface.on_resize() | ||
| self.interface.content.refresh() | ||
|
|
||
| # No need to check for other pending states, since this is fully applied | ||
| # No need to check for other pending states, since this is fully | ||
| # applied | ||
| # at this point. | ||
| self._pending_state_transition = None | ||
|
|
||
|
|
@@ -503,7 +505,9 @@ def _apply_state(self, target_state): | |
| opts.setObject( | ||
| NSNumber.numberWithBool(True), forKey="NSFullScreenModeAllScreens" | ||
| ) | ||
| self.container.native.exitFullScreenModeWithOptions(opts) | ||
| self._scaffold.current_container.controller.view.exitFullScreenModeWithOptions( | ||
| opts | ||
| ) | ||
| # Manually trigger the resize event as the original NSWindow's size | ||
| # remains unchanged, hence the windowDidResize_ would not be notified | ||
| # when the window goes out of the presentation mode. | ||
|
|
@@ -520,11 +524,12 @@ def _apply_state(self, target_state): | |
| ###################################################################### | ||
|
|
||
| def get_image_data(self): | ||
| bitmap = self.container.native.bitmapImageRepForCachingDisplayInRect( | ||
| self.container.native.bounds | ||
| container = self._scaffold.current_container | ||
| bitmap = container.native.bitmapImageRepForCachingDisplayInRect( | ||
| container.native.bounds | ||
| ) | ||
| self.container.native.cacheDisplayInRect( | ||
| self.container.native.bounds, toBitmapImageRep=bitmap | ||
| container.native.cacheDisplayInRect( | ||
| container.native.bounds, toBitmapImageRep=bitmap | ||
| ) | ||
|
|
||
| # Get a reference to the CGImage from the bitmap | ||
|
|
@@ -539,8 +544,8 @@ def get_image_data(self): | |
|
|
||
|
|
||
| class MainWindow(Window): | ||
| def __init__(self, interface, title, position, size): | ||
| super().__init__(interface, title, position, size) | ||
| def __init__(self, interface, position, size): | ||
| super().__init__(interface, position, size) | ||
|
Comment on lines
+547
to
+548
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. This is a no-op.
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. Noted, but note to self to not make this change because I'll be moving toolbar logic back into Window.
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. This is no longer a noop after moving the logic back here. |
||
|
|
||
| # By default, no toolbar | ||
| self._toolbar_items = {} | ||
|
|
||
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.
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...
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.
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?