search

AmericanWhitewater / aw-ios

American Whitewater's iOS app
5 stars 2 forks source link

Small fixes to the 1.4.0(2) TF build #267

Closed yeahphil closed 3 years ago

yeahphil commented 3 years ago

See https://console.firebase.google.com/u/1/project/american-whitewater-1234/crashlytics/app/ios:org.americanwhitewater.aw/issues/a8f7361272c0a9c9c4b3250363c6ae35?time=last-seven-days&versions=1.4.0%20(2)&sessionEventKey=e805326fff5e46829f79384bc7339b58_1592225718857010802

Gregliest commented 3 years ago

Is there no way to indicate that the request is still in flight? Isn't there a guard to make sure that we don't have two simultaneous requests? So what happens in the following scenario:

  1. User pulls to refresh then leaves the screen
  2. User comes back to the screen, spinner is now hidden, but request is still in flight
  3. They try to pull to refresh again.

Will it just not update until the first request comes back?

yeahphil commented 3 years ago

Ah yes, we should be able to accurately say if a request is in flight. I was trying to work around the refresh control being visible but not animating if you switch tabs while a request is in flight. I'm not sure what's up with that, but I saw the same behavior. Calling endRefreshing() then beginRefreshing() from viewWillAppear doesn't do the right thing and leaves the control in a weird state. So as a quick coverup, I figured, leave the control refreshing but don't set it to be visible. Not really the right answer of course.

We've now got ReachUpdater managing/limiting requests (via its static isFetchingReaches property), but only for updateReaches(regionCodes:). Maybe the right answer is:

  1. Have it manage isFetchingReaches for both updateReaches(regionCodes:) and updateReaches(reachIds:) (i.e. all the requests that can update multiple reaches). isFetchingReaches should reliably answer the question, is a Reach request in flight?
  2. Call endRefreshing() on viewDidDisappear
  3. Call beginRefreshing on viewWillAppear if a request is in flight

I worry maintaining the state of isFetchingReaches will end up getting messed up, or have some concurrency/race issue and wedge the app. But I suppose we're there now, just inconsistently and with more complex behavior.

Gregliest commented 3 years ago

Having a central place like ReachUpdater to keep track of this state sounds good. Maybe we should consider some sort of safety for that boolean though. For instance, maybe put a timeout of 30s or something, so it can recover if it somehow gets wedged?