Skip to content

Conversation

@MiaKoring
Copy link
Contributor

Is it already Christmas? No but I brought you a gift anyway.

Jokes aside.

This PR adds the .sheet Modifier, like we know it from SwiftUI with isPresented Binding and optional onDismiss callback.

Also some sheet modifiers:
presentationBackground
presentationDragIndicatorVisibility
presentationCornerRadius
presentationDetents
interactiveDismissDisabled

and Example usage in WindowingExample

support for the modifiers varies quite a bit. most are ignored if unsupported, interactiveDismissDisabled and presentationBackground are required as both shouldn’t be limited by Framework features.

Currently only UIKitBackend, AppKitBackend and GtkBackend support sheets, as I sadly couldn’t get them to work properly on WinUI. I hope someone else can build on the Foundation.

As was to expect, there are some weird quirks:
on UIKitBackend and AppKitBackend sheet content is placed top-leading. On GtkBackend on Linux Text elements are very small without further modification instead of allocation space. Same thing happened on macOS with GtkBackend in the 2nd and 3rd window (before and after my changes).

I expect those to be fixable through SCUI Frontend Code. If you know how to fix those in the Backends, I’d be happy to do so before a merge.

Copy link
Owner

@stackotter stackotter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love how much of the SwiftUI sheets API you've managed to implement in this PR! Sorry about all the requested changes; they're generally about the internals rather than the API surface. You've had to use a lot of SwiftCrossUI's more obscure internals like the preferences system and the Gtk bindings signal system, so it's completely understandable 😅 I'll test this out on my own devices locally when I get the time (probably tomorrow).

@stackotter
Copy link
Owner

I'm getting package resolution errors when I try to build this PR locally (due to an issue fixed by #224). I believe that merging in the latest changes from main should fix that issue.

Copy link
Owner

@stackotter stackotter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing those initial requested changes! I've got a few new ones (one of which I should've realised during the first review lol; the background colour one).

case large

/// A detent at a custom fractional height of the available space.
/// falling back to medium on iOS 15
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved for the one below but not this one. Please add a full stop at the end of the Falls back to ... sentence as well (in both).

@stackotter
Copy link
Owner

On macOS it appears that the dismiss button in a nested sheet breaks if you dismiss the parent sheet first.

@MiaKoring
Copy link
Contributor Author

On macOS it appears that the dismiss button in a nested sheet breaks if you dismiss the parent sheet first.

Dismissing a sheet up the graph should now lead to every sheet below it getting dismissed too.
On all supported platforms.

@MiaKoring
Copy link
Contributor Author

The new sheetParent environment value was added due to UIKitBackend.Sheet not being compatible with UIKitBackend.Window, so environment.with(.window, sheet) couldn’t be used.

UIKitBackend is also the only (currently sheet-supporting) platform where Backend.Window can’t be used to add a sheet. It needs a UIViewController, accessible trough UIWindow.rootViewController when creating the first sheet. to avoid compiler flags/UIKitBackend specific code in SCUI I decided to make it Any? and let each backend resolve the property/type it needs to create a sheet.

This should also be future proof for any other Backend with Window-Sheet incompatibility.

I noticed I created sheets always from the main Window before (definetely AppKit, Gtk I’m not sure).
This lead to the sheet z order occasionally getting messed up when dismissing a sheet down the graph.

Now using the deepest window to present a sheet should solve this and I was able to switch to a non-critical sheet on AppKitBackend (not sure if it would make a difference with the current implementation).

I don’t know how we could test wether the z order issue actually is fixed now, due to it appearing to be non deterministic from the layer we are working on.

Another nice side effect is a theoretical slight performance improvement in cases where there is a big stack of sheets, because it gets passed the deepest UIViewController instead of needing to loop like before.

I was afraid setting the managedAttachedSheet to nil on scope exiting could cause dismiss animation bugs, this doesn’t seem to be the case, but should be kept in mind just in case. Doing it is important though, because otherwise the root window on GtkBackend and AppKitBackend would keep a reference to the entire last presented Sheet Graph:

RootWindow
┗ managedAttachedSheet
┗ managedAttachedSheet
┗ managedAttachedSheet
┗ managedAttachedSheet
┗ …

Thus it couldn’t be freed up.

Like you suggested an AppKitBackend.Sheet now keeps a reference to its background node, if set.
So now it only gets its color changed when the function gets called instead of creating a new node each time.

tvOS only accepts UIModalPresentationStyle.formSheet starting on 26+. I decided to fall back to UIModalPresentationStyle.overFullscreen for now to allow developers to decide wether they cover the whole View with opaque content or just part of it (overFullscreen isn't opaque by itself).

https://developer.apple.com/documentation/uikit/uimodalpresentationstyle/formsheet
https://developer.apple.com/documentation/uikit/uimodalpresentationstyle/overfullscreen

CI fails with (formSheet unavailable on tvOS), locally it works,

if #available(tvOS 26.0, *) {
    sheet.modalPresentationStyle = .formSheet
} else {
    sheet.modalPresentationStyle = .overFullScreen
}

so I only used .overFullScreen for now.

I believe thats everything important to know about todays changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants