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 72 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 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.
There was a problem hiding this comment.
As noted in another comment - I think the bigger question here is "should it be possible to change the scaffold"?
| 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.
There was a problem hiding this comment.
I think this should be rolled back. Changing a test isn't something you do accidentally, or in passing. Unless there's a motivating reason behind a test change, it's a lot safer to keep a test the way it is.
| 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.
There was a problem hiding this comment.
Yes - but you've modified the code to always claim focus. It's possible to set text alignment when the widget doesn't have focus. We should only be re-applying focus if the widget had focus in the first place, and we explicitly released it to apply text alignment.
There was a problem hiding this comment.
Noted; will do!
Refactor _remove_constraints function for clarity and reliability.
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. |
|
@freakboy3742 @phildini It has been a week since my last request for review; tagging to make sure this is not missed. The merging has all been resolved. If y'all are busy with something else though, please note that there is no pressure to review this right now. Thanks! |
It hasn't been missed; I'm currently buried in preparations for PyCon AU next week. Hoping to give this a review by the end of the week. |
Ack; Thanks! Excited about how Scaffolds will turn out in Toga!!! |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Ok - this is starting to look pretty good. A couple of fairly minor things flagged inline... and one big thing, that may need some larger discussion.
Namely: much of the complexity and edge cases here are caused by the fact that scaffolds can change. Is that a reasonable thing to expect (or, more generally - is it a thing that we could lock down, at least in the first iteration, to make our lives significantly simpler)?
There has historically been a need to replace the content of a window - and I imagine that will continue. However, the overall idea behind scaffolds is that they are 'foundation' level structure - and I'm not sure I see a strong use case for changing that foundation in the general case. Visual Studio doesn't change from being a "sidebar" app to being an "option tabs" app - the basic structure of a window is defined when the Window is created.
So - would it make sense that you need to specify the Scaffold at the time the Window is created, and then can't change it? That avoids the initial "create a dummy scaffold" problem, it simplifies the PRESENTATION mode issues...
We could certainly revisit this in the future if it turns out to be useful - but for right now, are we making our lives more difficult that they need to be in service of a capability that nobody will need in practice?
| 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.
Yes - but you've modified the code to always claim focus. It's possible to set text alignment when the widget doesn't have focus. We should only be re-applying focus if the widget had focus in the first place, and we explicitly released it to apply text alignment.
| 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) |
There was a problem hiding this comment.
The same focus change is needed here. I wonder if it might be worth adding a general purpose focus-release/reclaim decorator to the base widget module/class, since the pattern should be the same.
There was a problem hiding this comment.
Sure; this will probably be worth it.
| NSNumber.numberWithBool(True), forKey="NSFullScreenModeAllScreens" | ||
| ) | ||
| self.container.native.exitFullScreenModeWithOptions(opts) | ||
| self._scaffold.current_container.controller.view.exitFullScreenModeWithOptions( |
There was a problem hiding this comment.
So... if the path to an object requires 4 property lookups, is used multiple times, and is long enough that you're almost hitting line length limits, it might be a good idea to do the lookup once, and then reference that object...
| return self._pending_state_transition | ||
| if self.container.native.isInFullScreenMode(): | ||
| if ( | ||
| hasattr(self, "_scaffold") |
There was a problem hiding this comment.
So - my inclination here is to work out what makes that condition happen, and try to make it not happen. What's the sequence of events that causes get_window_state() to be invoked?
The other possibility - should we be preventing the scaffold from being changed? Is there a use case for an app switching from being an OptionScaffold app to being a SidebarScaffold app during app lifecycle (or - more importantly - can we reasonably make that a constraint right now, and revisit it later?)
| 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.
As noted in another comment - I think the bigger question here is "should it be possible to change the scaffold"?
| # of the status bar to use as our inset. | ||
| # The testbed will not instantiate a simple app so no-cover the first | ||
| # branch | ||
| if self.navigation_bar_hidden: # pragma: no cover |
There was a problem hiding this comment.
This is a big block of logic to have no-cover. I accept that we don't have a "simple" app - but hiding the navigation bar is a programmatic concept (or, could be in the context of a test) - can we exercise it that way?
There was a problem hiding this comment.
(as in - I'd be OK for our testbed to do some "make the app simple" calls using internal APIs if that's what we need to get coverage here. Those tests could even be gated as "mobile only" or "only if the backend allows hiding window chrome" or similar)
There was a problem hiding this comment.
Calling internal API sounds great for a test; but I did not previously take this opportunity becuase this coverage gap is existing and not newly introduced by this PR. I personally prefer this to be separated into a different issue/PR pair; if you'd rather this be done, let me know here, but otherwise, I'm still going to proceed with your original suggestion since this was brought up here.
| if hasattr(self, "scaffold"): | ||
| scaffold.title = self.get_title() |
There was a problem hiding this comment.
This sort of check is better as an "if None" check, rather than a hasattr check.
| "pytest-asyncio==1.4.0", | ||
| "pytest-retry==1.7.0", | ||
| # This enables people to debug intermittent hard crash errors in CI by enabling --instafail in the test options. | ||
| "pytest-instafail==0.5.0", |
There was a problem hiding this comment.
Not sure I follow what this is doing.. how does it help hard crashes? Can you point at a test run where it helped?
There was a problem hiding this comment.
My theory was that sometimes hard crashes will have some sort of exception thrown before it fails out. But this wasn't common, and I managed to guess the correct causes of tests without this plugin, so this is untested and can be removed; will do, and sorry for forgetting to do so after I removed --instafail.
| # 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.
I think this should be rolled back. Changing a test isn't something you do accidentally, or in passing. Unless there's a motivating reason behind a test change, it's a lot safer to keep a test the way it is.
|
Thanks for the review, @freakboy3742. Responded to some comments inline, mostly clarifications and agreements by this point and no additional concerns (and only a minor clarification that is not strictly required to move forward); hopefully I have more time to work on this PR by the end of the week and realize those suggestions.
The specific use case for changing scaffolds for me personally is to have one of these "loading screens" or "splash screens" before the UI displays; these screens are generally a separate scaffold, as they do not have toolbar/sidebar. If memory serves me right, I think this is a support query BeeWare gets a lot. So I think handling changing scaffolds is at least useful. Another significant factor is testing. Specifically, all of our current tests are ran on a main_window, and if we prohibit changing scaffolds then on mobile we will have to destroy the current window to create a new one so that it can have another new scaffold, which is a lot of refactors to tests and bookkeeping. In the future, when we want to add scaffold changing, I think working out how to integrate those changes (and other quirks in differnet platforms) may become more difficult when we pre-assume that scaffolds do not change, so I wanted to allow this possibility earlier instead of later. I personally believe the current status quo is good. Most of the complexities from changing scaffolds are macOS implementation specific, and while sometimes they do get a bit tedious, I suggest that they are mostly conceptually low-cost. So personally, I suggest that we do not prohibiting the changing of scaffolds. But this is of course the core team's call—it's entirely possible that I am underestimating the conceptual costs of all the changes required. |
|
@freakboy3742 I've addressed all of your directly actionable requested changes, and put some reasoning for allowing scaffold changes in my previous comment. I am therefore looking for another review at your convenience; happy to be proven wrong on architectural fronts if anything I did here is not good design. |
|
Thanks for those updates; to set expectations - I'm about to head to PyCon AU, so my bandwidth to review code will be reduced for about a week. I'll take a look if I get a chance, but it might take a little longer than normal. |
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,