Implementation of Scaffolds layer for Apple platforms, with related fixes - #4605
Implementation of Scaffolds layer for Apple platforms, with related fixes#4605johnzhou721 wants to merge 63 commits into
Conversation
|
Pausing this for a bit as I'm working on #4628 at this moment. |
johnzhou721
left a comment
johnzhou721
left a comment
There was a problem hiding this comment.
Some design decisions / small driveby cleanups I made that I wanted to flag for discussion. Most of the driveby cleanups are co-morbid issues that happens while testing scaffolds, and I've documented all the causes from my investigations to the best of my abilities.
I've taken my best effort to explain these below, but there's a chance I might mis-explain or miss something. Feel free to flag all other geneeral inconsistencies.
I chose to put iOS and macOS implementations together in 1 PR because it's logical since they're both Apple, and to also avoid the possibility of making any desktop/mobile-specific assumptions in the first few implementations of scaffolds. If you'd prefer to split this into iOS and macOS pieces, let me know.
| return f"Toolbar-{type(cmd).__name__}-{id(cmd)}" | ||
|
|
||
|
|
||
| class ToolbarDelegate(NSObject): |
There was a problem hiding this comment.
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 SidebarScaffold or OptionScaffold starts 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.
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.
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.
It feels to me like you're convolving "how is the toolbar implemented" with "where are the toolbar items defined".
Most definitely yes. Thanks for catching my conceptual misunderstanding.
There was a problem hiding this comment.
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.
| 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)) |
There was a problem hiding this comment.
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.
| if self.get_window_state() == WindowState.PRESENTATION: | ||
| restore_presentation = True | ||
| # This is instaneous so yay!!! | ||
| self.set_window_state(WindowState.NORMAL) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Or... we could prohibit changing the scaffold while in presentation mode...
There was a problem hiding this comment.
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.
| self.native.bind( | ||
| NSTitleBinding, | ||
| toObject=self.native, | ||
| withKeyPath="contentViewController.title", | ||
| options=None, | ||
| ) |
There was a problem hiding this comment.
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.
| def __del__(self): # pragma: nocover | ||
| self._remove_constraints() | ||
| # If this gets called on the other threads than hilarity ensues. | ||
| # So we delegate cleanup to another non-self-bound function and | ||
| # use Weakrefs for everything. | ||
| try: | ||
| self.widget.interface.app.loop.call_soon_threadsafe( | ||
| partial( | ||
| _remove_constraints, | ||
| ref(self.container), | ||
| self.constraints_created, | ||
| [ | ||
| ref(self.width_constraint), | ||
| ref(self.height_constraint), | ||
| ref(self.left_constraint), | ||
| ref(self.top_constraint), | ||
| ], | ||
| ) | ||
| ) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
I have no idea why _remove_constraints used to be reliable, but now it seems like with scaffolds introduced this del happens on the test thread extremely often, so I've changed the cleanup flow here to async call into the main thread.
There was a problem hiding this comment.
This makes me very nervous. Having core logic be substantially more complex so that the testbed doesn't crash is a bit of an anti pattern.
There was a problem hiding this comment.
I understand and am too concerned about the fact that this is significant complexity, esp. given the fact that Constraints is used everywhere and breaks a lot of things if mishandled.
Hrm... but Python itself does not guarantee that del cleanup logic only runs on a certain thread, so this seems like a safer approach to me.
I had tried to work out why this del was called on the test thread more often before adding this workaround, but at this point I'm out of ideas. Suggestions to simplify this would be helpful here.
| # Alter both height and width to exceed window size at once | ||
| box3 = toga.Box(style=Pack(background_color=LIGHTBLUE, width=300, height=90)) | ||
| second_window.content.add(box3) | ||
| with box1.style.batch_apply(): | ||
| box1.style.width = 300 | ||
| box2.style.height = 290 |
There was a problem hiding this comment.
Minor unrelated simplification.
There was a problem hiding this comment.
Not sure I understand how this is a simplification... if only because it's one more line... but also it appears to be doing something completely different, with different box sizes.
There was a problem hiding this comment.
So before this piece of code was changed, we used another subbox of second_window.content in order to increase both height and width at the same time. The intent was that the window's content size will have both its width and height increased.
I used batch_apply here to do this explicitly, so it's conceptually simplified as we do not have to add anoteher widget into hte window to force the content size to expand.
If you'd rather not do this, I can revert this small change and there'll be few impact.
| def set_text_align(self, value): | ||
| if self.interface.window and 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
Yep, new alignment isn't applied at all, and the alignment value remains the old one.
There was a problem hiding this comment.
I've added and pushed hte code to reapply focus.
Refactor _remove_constraints function for clarity and reliability.
|
Sorry for the extra noise in pushing commits; I'm trying to track down an intermittent CI issue, and I'm experimenting with various ways to fix it. My iOS simulator environment also got borked locally today and I've only managed to fix it now, so there were some extra commits fixing issues in my various attempts. |
Merge upsteram
Add assertions to check container dimensions are positive.
|
I’m asking for another review here… having finally debugged the CI instabilities, I’d like for y’all to run the macOS and iOS test beds each about at least 5 times to rule out to possibility or them reoccurring. You may see that there’s a lot of irrelevant changes in this PR; this is because the branch scaffolds it out of date. Therefore. Please merge main into scaffolds on the BeeWare upstream repo before doing anything else here. |
phildini
left a comment
phildini
left a comment
There was a problem hiding this comment.
Thank you for your time in trying to move the Scaffold discussion forward.
The combination of:
- CI script changes
- towncrier .md changes
- overall lint / typo fixes
makes this PR extremely hard to review properly.
A clean rebase with just your changes makes this PR far more likely to get reviewed, although currently there's so much extra that it's hard to tell if your changes meet the spirit of #4271.
Rebase drift is a hard thing to manage; it is in fact acceptable for you to move your changes to a clean branch, close this PR, and open a new one, if that's what you would prefer.
Hey @phildini, Thanks for taking the time to look at the PR. This stack of changes on here is intentional, and I apologize for not explaining so clearly. This PR is not made to the default branch of Toga. This PR is made to a branch named scaffolds in beeware/toga, which is for a larger-scale refactor in Toga to introduce the Scaffold layer as seen in #4271. The Now, I merged the default branch of Toga, In conclusion, I suggest merging the branch Thank you. Let me know if you need any clarifications. |
|
(FWIW: Having Russ confirm what to do with the merging/rebasing is merely a preferrance. If any core team member deems that in this situation merging into the current scaffolds branch and then catching up with the drift from main later is more helpful, then I will follow their requests. I undestand that I am not in an authority to decide that a specific core team member needs to response, do not intend to imply so, and apologizes for any wording in the previous messages that appears to do so.) |
898e48f to
9c03fc9
Compare
|
[Edited to heavily simplify] I've followed through with your original requests and rebased on top of the older branch I thought about this again and now agrees with your approach. So sorry for the previous noise. Thank you! EDIT: Still ready for review after additional commits; those were commits I made after the merge cherry-picked here. |
I merged the scaffold branch with main late last week; I've just updated again following the weekly dependabot updates. Generally speaking, we prefer merge commits over rebasing because rebasing loses the context for any historical review comments. |
4297f0f to
898e48f
Compare
Yep, I did use merge commits but because scaffolds was not up to date the extra commits showed. I've repushed the state of the PR when I made the request; now the diff should still be clean, so @freakboy3742 when you get a chance another look at this would be super appreciated. |
|
Hmm... seems like scaffolds still shows 85 commits behind main on https://github.com/beeware/toga/tree/scaffolds. Have I missed something? |
You haven't missed anything - the problem exists between the keyboard and chair at my end. I merged the branch, and pushed it to I've just pushed to beeware/scaffolds. |
|
Thanks for the clarification, and no worries about the problem! For reference, I had to merge 3 times before getting upstream main merged into this PR 🤦, and even then something went astray as it conflicted with the new scaffolds branch you merged... not your fault, though, because I've already merged 3 different hashes somehow I probably messed up somewhere, but things will be squashed anyways, so I assumed if the diff is fine the intermediate commits does not matter. But if I'm wrong at this end, feel free to point out any issues. But if there are no such issues, to be explicit, this is still ready for you to take a look at at your convenince, as the conflict resolution did not change any funcitonal content of this PR. |
3 participants
I'm back on Scaffolds! Sorry for not getting this done before the Q2 deadline.
What's changed
This PR implements scaffolds for iOS and macOS. All the design and extra fixes are documented in #4605 (review) — lots of things only make sense when pointed to concrete code inline, so I figured I might post a separate comment with all the documentation of the design decisions made.
Most notably I moved toolbar handling to Scaffolds because future types of scaffolds will have more complex forms of toolbar declaration and moving toolbars to scaffolds will remove a lot more coordination logic in the future.
The changes in this PR make toga-core incompatible with the rest of the backends, but I will be fixing them later.
Validation
(This section is not written by AI. I figure it might be humorous to use these stereotypical title names, as in this case there really are additional manual testing needed and additional headings make navigation easier.)
Make sure these things work:
windowandsimpleappexample apps on macOS 26, iPhone and iPad 18 and 26.Requires Full Screenkey from Info.Plist of generated Xcode projects and rebuild to test that resizing window works, and that the contents are only inset if the window actually overlaps with the top status bar.windowexample app is set properly; window size should be retained even after changing content; changing content in PRESENTATION mode works.PR Checklist:
Assisted-by: GitHub Copilot, ChatGPT,