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

mirrorAppearanceState and initially hidden views #19

Closed
kabouzeid opened this issue Apr 18, 2022 · 10 comments
Closed

mirrorAppearanceState and initially hidden views #19

kabouzeid opened this issue Apr 18, 2022 · 10 comments

Comments

@kabouzeid
Copy link

I have a View within a TabView, and I'm using .mirrorAppearanceState on the View. Initially the View's tab is not selected and thus the View is not visible.

The problem is that the View will now keep auto updating in the background until the respective tab is selected for the first time. onDisappear is not called because the View never appeared in the first place.

@groue
Copy link
Owner

groue commented Apr 18, 2022

Hello @kabouzeid,

You're correct, since mirrorAppearanceState detects appearance with the onAppear and onDisappear SwiftUI callbacks.

You do not report a bug in GRDBQuery.

Now, if mirrorAppearanceState is not the tool you need, you can still use the isAutoupdating binding directly, and all your SwiftUI and Combine knowledge. Please search for a solution, and report your findings! Maybe you'll find a way to improve the library, and that would be a great contribution.

@kabouzeid
Copy link
Author

Thanks! I'm experimenting with different ways to get around this right now. I'll report back once I find a good solution.

What threw me off initially is this comment:

/// You can use this binding in order to stop tracking the database when
/// a view is not on screen:
///
/// ```swift
/// struct PlayerList: View {
/// @Query(PlayerRequest())
/// var players: [Player]
///
/// var body: some View {
/// List(players) { player in ... }
/// .mirrorAppearanceState(to: $players.isAutoupdating)
/// }
/// }

@groue
Copy link
Owner

groue commented Apr 19, 2022

Thank you @kabouzeid, I'll rewrite this documentation so that it better describes the reality.

groue added a commit that referenced this issue Apr 19, 2022
groue added a commit that referenced this issue Apr 19, 2022
@groue
Copy link
Owner

groue commented Apr 19, 2022

I hope f8443a2 makes the doc less ambiguous.


I agree that delaying the first database access until the View appears on screen is a reasonable need. Your TabView example is great.

It just happens that @Query was first introduced in order to access the database before the view is rendered for the first time (see the Why @Query? paragraph) 😅

I'll report back once I find a good solution.

Thank you! Maybe you'll want to perform modification to the library itself: I'll happily welcome a pull request, and guide you if you meet difficulties.

@kabouzeid
Copy link
Author

My PR implements the behaviour I'm looking for with minimal changes: Fetch once immediately, then only start auto updating once I set isAutoupdating = true. The obvious drawback is that this is a breaking change because now setting isAutoupdating = true in onAppear is required for getting updates.

@groue
Copy link
Owner

groue commented Apr 19, 2022

The obvious drawback is that this is a breaking change because now setting isAutoupdating = true in onAppear is required for getting updates.

Yup. This is not acceptable, I'm sorry. Dealing with the initial state of the view is the most important feature of the lib. Please read again the Why @Query? paragraph.

@kabouzeid
Copy link
Author

kabouzeid commented Apr 19, 2022

The initial state is still fetched immediately (before onAppear), nothing has changed there. The only difference is that the publisher will not auto update after that until isAutoupdating is explicitly set to true.

Basically isAutoupdating = false by default, but the initial fetch will always happen.

@groue
Copy link
Owner

groue commented Apr 19, 2022

All right, I'll have a better look. Thanks for pointing my mistaken interpretation :-)

@kabouzeid
Copy link
Author

Perhaps you'll like #22 better.

It allows both

@Query(Request(), in: \.database) private var values: [Value] // same as before, `isAutoupdating = true` by default

and

@Query(Request(), in: \.database, isAutoupdating: false) private var values: [Value]

...

.mirrorAppearanceState(to: $values.isAutoupdating)

@groue
Copy link
Owner

groue commented May 1, 2022

🚀 v0.3.0 has shipped with QueryObservation, which addresses shortcomings of mirrorAppearanceState and isAutoupdating. Most demanding applications can use the low-level \.queryObservationEnabled environment key. Maybe future GRDBQuery versions will ship with more observation strategies, based on user feedback and their custom use of \.queryObservationEnabled.

@groue groue closed this as completed May 1, 2022
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

No branches or pull requests

2 participants