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 tab content view recreation #24

Open
wants to merge 1 commit into
base: release/2.0.0
Choose a base branch
from

Conversation

winddpan
Copy link

This PR intent to prevent tab content view recreation.
Mainly used StateObject to maintain the state and set opacity=0 to keep the view. (And aslo support tab content view lazy loading)
Sorry for the many format changes due to SwiftFormat. Also, I made some modifications to the test the view keeping in the demo. If you don't like it, I can submit a new PR with minimal changes.

Copy link
Owner

@onl1ner onl1ner left a comment

Choose a reason for hiding this comment

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

Hello, @winddpan, thank you for your PR!

I have reviewed your changes and highlighted some issues that need to be addressed before merging. Also, if I understand your approach correctly, you plan to introduce a StateObject entity to store the items of TabBar and the selection property. If this approach successfully resolves the issue with unexpected view recreation, we should consider removing the PreferenceKey usage, as it would become redundant.

Package.swift Show resolved Hide resolved
@State private var items: [TabItem]

@StateObject private var selectedItem: TabBarSelection<TabItem>
private let content: () -> Content
Copy link
Owner

Choose a reason for hiding this comment

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

Storing the closure as a parameter rather than invoking it immediately could lead to performance issues, as it disrupts the memoization process of SwiftUI.

Copy link
Author

Choose a reason for hiding this comment

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

In fact, delayed closures perform better in practice. SwiftUI needs to construct View instances to compare function signatures, resulting in a higher frequency of View initialization compared to the View body getter.

Copy link
Owner

@onl1ner onl1ner Apr 12, 2024

Choose a reason for hiding this comment

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

Storing closures in a parameter makes the View (in this case TabBar) to re-evaluate on every @State change of the on the closure provider side, even on unrelated one, because closures are not Equatable and SwiftUI cannot check if the return value of this closure changed or not.

Comment on lines +78 to 90
struct TextWrapper: View {
@State var string: String = UUID().uuidString

var body: some View {
Text(string)
.onTapGesture {
string = UUID().uuidString
}
.onAppear(perform: {
print("onAppear:", string)
})
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please clarify the purpose of this wrapper? I didn't quite understand it.

Copy link
Author

Choose a reason for hiding this comment

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

Just for the purpose of testing recreation and lazy loading, no other meaning.
If you don't like it, I can minimize the PR.

Copy link
Owner

Choose a reason for hiding this comment

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

I think it will be better revert these changes, as Example project most likely will be updated later.

Comment on lines +72 to +74
.onChange(of: selection) { newValue in
print("selection changed:", newValue)
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should remove this print statement, as it doesn't add any significant value here.

Copy link
Author

Choose a reason for hiding this comment

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

Just for the purpose of testing, can remove it.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, remove it please.

@onl1ner onl1ner changed the base branch from master to release/2.0.0 April 12, 2024 19:44
@onl1ner
Copy link
Owner

onl1ner commented Apr 12, 2024

Updated the base branch to release/2.0.0 as it introduces breaking changes.

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