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

Fix rootEnvironment not merged with .defaultEnvironment #461

Merged
merged 4 commits into from
Jan 3, 2022

Conversation

regexident
Copy link
Contributor

While spelunking through the implementation of EnvironmentValues and its uses in Tokamak I noticed that a couple of functions were accepting a rootEnvironment: EnvironmentValues? = nil argument but then discarding whatever value you pass to it in favor of .defaultEnvironment.

This seems wrong to me? 🤔

@carson-katri
Copy link
Member

Good catch. I think you may still need to add in the values .defaultEnvironment sets up if they are not already setup by the rootEnvironment though, specifically _ToggleStyleKey, .colorScheme, ._defaultAppStorage, and _DefaultSceneStorageProvider.

@regexident
Copy link
Contributor Author

Let me know if you'd rather have the merging logic as a private implementation detail. I figured since effectively every backend/renderer implementation would need it it might be worth providing a canonical method for it. Providing both .merge(_:) and .merging(_:) might a bit overkill though.

@carson-katri
Copy link
Member

Since this isn't part of SwiftUI, may be best to make it internal with @_spi so the renderers can access it but users don't.

@carson-katri carson-katri added the bug Something isn't working label Jan 1, 2022
@MaxDesiatov MaxDesiatov changed the title Make actual use of rootEnvironment passed into functions, falling back to .defaultEnvironment Fix rootEnvironment not merged with .defaultEnvironment Jan 1, 2022
@regexident
Copy link
Contributor Author

Absolutely! Fixed.

Copy link
Member

@carson-katri carson-katri left a comment

Choose a reason for hiding this comment

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

I think you still need to add @_spi(TokamakCore) to some imports to fix the builds. Once that's fixed this LGTM!

@regexident
Copy link
Contributor Author

Fixed!

Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@carson-katri carson-katri merged commit a9addc8 into TokamakUI:main Jan 3, 2022
@regexident regexident deleted the fix-environment branch January 3, 2022 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

3 participants