-
-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add NSViewRepresentable
for AppKit & support Swift 6 concurrency.
#105
Conversation
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.
Thanks for the PR 🙏 just requested a few changes
Signed-off-by: furby™ <devs@wabi.foundation>
Signed-off-by: furby™ <devs@wabi.foundation>
Signed-off-by: furby™ <devs@wabi.foundation>
Signed-off-by: furby™ <devs@wabi.foundation>
* Part of our efforts to condense and streamline all the various C/C++ dependencies under one central repository, this is important because Swift makes it all too easy to have 30 different zlib libraries for example, which leads to a headache of duplicated symbols and what not. * That central repository is https://github.com/the-swift-collective Signed-off-by: furby™ <devs@wabi.foundation>
Signed-off-by: furby™ <devs@wabi.foundation>
Signed-off-by: furby™ <devs@wabi.foundation>
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.
Looks good now 🙏
Actually, where did the |
Lol, oh really? Yeah I need to actually test it -- I simply compiled it is all... what would the implementation look like for those methods? I simply removed them because they were calling into that final class RepresentingWidget<Representable: NSViewRepresentable>: BaseWidget
{} so I assumed those methods were not necessary. Here are the removed functions that were utilizing that underlying What did you have in mind instead? |
cc. @stackotter ^ |
Signed-off-by: furby™ <devs@wabi.foundation>
@stackotter checkout my latest revision for a implementation assumption I made on adding your |
@stackotter upon actual testing I can confirm the latest changes in this PR allow it to work (without crashing & with expected behavior at least with my USD hydra powered viewport app). Notable changes
|
also this is macOS 10.15+ compliant with the |
* This ensures scui works with swift 6 strict concurrency - however it is lopsided in that makeCoordinator() is not marked with a main actor. Signed-off-by: furby™ <devs@wabi.foundation>
After coming across a thread on the swift forums, it looks like there are some issues regarding swift 6 strict concurrency and non-Apple GUI frameworks like GTK, as such I've put in place a "lopsided" approach, which minimally allows things to work with swift 6 strict concurrency but removes the So things should be forwards compatible, and we can open up proper support for swift 6 cross-platform once this issue is addressed: |
Signed-off-by: furby™ <devs@wabi.foundation>
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.
Gonna merge this into a temp branch and test/fix things there
9f42f6d
into
stackotter:nsviewrepresentable
…105) Co-authored-by: stackotter <stackotter@stackotter.dev>
…105) Co-authored-by: stackotter <stackotter@stackotter.dev>
Note: this revision bumps the minimum macOS version from v10.15 to v11 though we should probably do as the CI compiler error for the examples indicate and maintain backwards compatibility via
#available
macros.Leaving the error in for now, to make obvious there was a minimum version bump and discuss how we should best move forward before merging this in.