Skip to content
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

Merged
merged 10 commits into from
Mar 2, 2025

Conversation

furby-tm
Copy link
Contributor

@furby-tm furby-tm commented Feb 16, 2025

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.

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 the PR 🙏 just requested a few changes

@furby-tm furby-tm requested a review from stackotter March 1, 2025 07:14
furby-tm added 7 commits March 1, 2025 01:34
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>
stackotter
stackotter previously approved these changes Mar 1, 2025
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.

Looks good now 🙏

@stackotter
Copy link
Owner

Actually, where did the asWidget and update methods for NSViewRepresentable's View implementation go? I don't think the code works in its current form (haven't tested yet though).

@furby-tm
Copy link
Contributor Author

furby-tm commented Mar 1, 2025

Actually, where did the asWidget and update methods for NSViewRepresentable's View implementation go? I don't think the code works in its current form (haven't tested yet though).

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 BaseWidget class that you had me remove:

final class RepresentingWidget<Representable: NSViewRepresentable>: BaseWidget
{}

so I assumed those methods were not necessary.

Here are the removed functions that were utilizing that underlying BaseWidget class:
0d34485#diff-e04a1f95fd782aab32cd2305bc927fc9466a10cda97b441b7d8c78c5d7340f7eL124-L160

What did you have in mind instead?

@furby-tm
Copy link
Contributor Author

furby-tm commented Mar 1, 2025

cc. @stackotter ^

Signed-off-by: furby™ <devs@wabi.foundation>
@furby-tm
Copy link
Contributor Author

furby-tm commented Mar 1, 2025

@stackotter checkout my latest revision for a implementation assumption I made on adding your update and asWidget methods back, still have not gotten around to testing it yet... but compiling it is still successful.

@furby-tm
Copy link
Contributor Author

furby-tm commented Mar 1, 2025

@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

  • I had found BaseWidget to be quite an important implementation detail as I could not create an asWidget method implementation that did not crash without it, it's totally possible I was just missing something obvious -- but will need your eyes to look at it closer to see if it's actually necessary -- my testing told me it was though.
  • The added @MainActor specification on the func makeCoordinator() -> Coordinator created a bit of a domino effect that caused me to litter many of scui's methods with @MainActor declarations - following the compiler errors all the way to a successful compilation ended in adding those declarations all the way up to App's main() function, but I suppose in hindsight at least that bit makes sense.

@furby-tm
Copy link
Contributor Author

furby-tm commented Mar 1, 2025

also this is macOS 10.15+ compliant with the #available macros being utilized here, using safeAreaLayoutGuide on more modern versions of macOS.

* 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>
@furby-tm
Copy link
Contributor Author

furby-tm commented Mar 2, 2025

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 @MainActor from makeCoordinator() determineViewSize.

So things should be forwards compatible, and we can open up proper support for swift 6 cross-platform once this issue is addressed:
https://forums.swift.org/t/pitch-custom-main-and-global-executors/77247/39

Signed-off-by: furby™ <devs@wabi.foundation>
@stackotter stackotter changed the base branch from main to nsviewrepresentable March 2, 2025 00:41
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.

Gonna merge this into a temp branch and test/fix things there

@stackotter stackotter merged commit 9f42f6d into stackotter:nsviewrepresentable Mar 2, 2025
3 of 4 checks passed
stackotter added a commit that referenced this pull request Mar 2, 2025
…105)

Co-authored-by: stackotter <stackotter@stackotter.dev>
stackotter added a commit that referenced this pull request Mar 2, 2025
…105)

Co-authored-by: stackotter <stackotter@stackotter.dev>
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