Skip to content

Conversation

@CippoX
Copy link

@CippoX CippoX commented May 2, 2023

Description

Changed SourceControlAccount as a class that conforms to ObservableObject, so var description: String is now @Published.
This is just an attempt. As a beginner, I don't know if this is the right way to fix the problem.

Related Issues

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code

Screenshots

Registrazione.schermo.2023-05-02.alle.15.06.05.mov

@CippoX CippoX force-pushed the fixed-account-description-text-field-in-account-settings branch from e5f9982 to 4dcead0 Compare May 2, 2023 13:24
@Qubik65536
Copy link
Contributor

I think this is already fixed by #1249 ? That branch works in my testing
/cc @austincondiff

@austincondiff
Copy link
Collaborator

That is correct but I might prefer this approach. I'll do some testing to verify.

@austincondiff
Copy link
Collaborator

austincondiff commented May 3, 2023

The question is, which approach to this problem to we prefer?

Do we want to change the struct to a class as done in this PR or do we want to create a parallel state variable in the view as I have done here? Both are not pretty solutions in my opinion. Maybe there is another option we aren't considering, who knows. Notice I use this state variable thought the file and when this state variable gets any updates it sets the settings variable like this.

I know @CippoX put work into this PR and I really appreciate that, but I am interested in the pattern we want to continue with moving forward and what is best for the project overall. Anyone have any thoughts on this.

@CippoX
Copy link
Author

CippoX commented May 3, 2023

My understanding is that changing the variable to @Published is the right way to solve this, but as I mentioned, I am unable to tell which of the two approaches is preferable. Of course what matters is what is right for the project

@Qubik65536
Copy link
Contributor

The question is, which approach to this problem to we prefer?

Do we want to change the struct to a class as done in this PR or do we want to create a parallel state variable in the view as I have done here? Both are not pretty solutions in my opinion. Maybe there is another option we aren't considering, who knows. Notice I use this state variable thought the file and when this state variable gets any updates it sets the settings variable like this.

I know @CippoX put work into this PR and I really appreciate that, but I am interested in the pattern we want to continue with moving forward and what is best for the project overall. Anyone have any thoughts on this.

I also did some researches on this problem, as @CippoX mentioned, @Published seems to be the right solution for this problem. The thing that makes this solution not very elegant is the needs for reimplementation of some functions. But since this seems to be the right thing to do (from what we know now), in my opinion, it's better for us to adopt this one and we can try to find other options in the future.

@austincondiff
Copy link
Collaborator

After speaking with several maintainers, we've concluded that the we'd like to keep Settings all structs for consistency. This is my primary concern with this PR here. I agree with the idea that it should use @Published, however if this PR is used, it will only increase in complexity the more we add. We will go with my solution for the time being until we can find a solution that does not require a parallel state variable.

@CippoX CippoX deleted the fixed-account-description-text-field-in-account-settings branch May 25, 2023 10:47
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.

4 participants