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] Custom Boolean bindings bug on Mac Catalyst #358

Merged
merged 7 commits into from
Mar 28, 2024

Conversation

CalebRas
Copy link
Contributor

Description

This PR implements a fix for a bug where toggle states would not update on Mac Catalyst when a custom binding was used. The bindings were replaced with State variables that trigger updates to a given value through an onChange modifier.

Sample affected:

  • Display dimensions
  • Find route around barriers
  • Group layers together

Linked Issue(s)

  • swift/issues/4994

How To Test

  • Test the toggles in the samples listed on Mac Catalyst.
  • Ensure the samples' functionality is otherwise unchanged.

@CalebRas CalebRas self-assigned this Mar 27, 2024
@CalebRas CalebRas requested review from a team, pgruenler and njarecha and removed request for a team March 27, 2024 00:12
Copy link
Contributor

@njarecha njarecha left a comment

Choose a reason for hiding this comment

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

Few suggestions

Comment on lines 35 to 37
_routingFindsBestSequence = State(initialValue: routeParameters.findsBestSequence)
_routePreservesFirstStop = State(initialValue: routeParameters.preservesFirstStop)
_routePreservesLastStop = State(initialValue: routeParameters.preservesLastStop)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason to use State(initialValue:)?

Suggested change
_routingFindsBestSequence = State(initialValue: routeParameters.findsBestSequence)
_routePreservesFirstStop = State(initialValue: routeParameters.preservesFirstStop)
_routePreservesLastStop = State(initialValue: routeParameters.preservesLastStop)
self.routingFindsBestSequence = routeParameters.findsBestSequence
self.routePreservesFirstStop = routeParameters.preservesFirstStop
self.routePreservesLastStop = routeParameters.preservesLastStop

Copy link
Contributor

Choose a reason for hiding this comment

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

The doc is pretty clear that state properties should not be set in an initializer, neither by explicitly calling init(initialValue:) nor by implicitly calling init(wrappedValue:):

You don’t call this initializer directly. Instead, SwiftUI calls it for you when you declare a property with the @State attribute and provide an initial value

The reason being is that state is a private, implementation detail. The parent view's body being re-computed shouldn't alter the view's state, which is what happens when a state property is set from inside an initializer. For values provided by an initializer, non-state properties should be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philium sorry I missed your comment. Would it be better to use bindings here in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I took a closer look at what this sample is doing and in this case, I don't see an alternative option that isn't more complicated. As I mentioned, the concern with setting the state in an initializer is that the state will be reset every time the parent's body is re-computed, but in this case, that isn't an issue. (In an ideal world, RouteParameters would be a struct and the settings view would take a binding to it. Then everything would just work without needing separate state properties.)

A couple other things I noticed:

  • The settings view initializer parameter label should be routeParameters instead of for. From the Swift API Design Guidelines: "The first argument to initializer…calls should not form a phrase starting with the base name, e.g. x.makeWidget(cogCount: 47)"
  • The settings view should use Form instead of List, as Form is "a container for grouping controls used for data entry, such as in settings or inspectors."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philium Sounds good. I'll make another PR to address those issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The settings view should use Form instead of List

here is a card for SprintX 😼

@CalebRas CalebRas requested a review from njarecha March 27, 2024 22:36
@CalebRas CalebRas requested a review from njarecha March 28, 2024 02:00
@CalebRas
Copy link
Contributor Author

Thank you guys for reviewing!

@CalebRas CalebRas merged commit 4e87341 into v.next Mar 28, 2024
1 check passed
@CalebRas CalebRas deleted the Caleb/Fix-MacCatalystCustomBooleanBindingsBug branch March 28, 2024 16:24
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.

None yet

5 participants