Add ExpandingVC Timeout + Explicitly Call updateScrollViewHeight on main thread #182
Conversation
NicoHinderling
force-pushed
the
height-expansion-new-logic
branch
2 times, most recently
from
40e4a1c to
c04a7a8
Compare
NicoHinderling
force-pushed
the
height-expansion-new-logic
branch
from
c04a7a8 to
9ac6fd6
Compare
NicoHinderling
force-pushed
the
height-expansion-new-logic
branch
from
7e6bb8b to
aecba58
Compare
|
|
||
| func startTimer() { | ||
| if self.timer == nil { | ||
| self.startTime = Date() |
There was a problem hiding this comment.
Can we eliminate all the self. that aren't necessary? I noticed they aren't used in other functions like "stopAndResetTimer", we shouldn't have them here either
| NSLog("ExpandingViewController: Expanding Scroll View timed out. Current height is \(scrollView?.visibleContentHeight ?? -1)") | ||
|
|
||
| // Setting anchors back to full | ||
| let fittingSize = self.sizeThatFits(in: UIScreen.main.bounds.size) |
There was a problem hiding this comment.
I worry a bit about this sizeThatFits call only because it has to run on the main thread, and could take time/cause other layout changes. What is the reason for setting the anchors here rather than just calling runCallback?
There was a problem hiding this comment.
because in our discussion last week, I recall you saying if we time out, we should ensure that the size of the snapshot reverts back to "default" aka just the full screen
is that no longer the case?
There was a problem hiding this comment.
I don't think this is necessary/not sure why it would be helpful
There was a problem hiding this comment.
Even if we did need to change the size might make more sense to use the full screen like you said, but this would use a variable height right? Either way I don't think we should touch the heightAnchor once we decide it's going to be an error
| // Timeout limit | ||
| if timer == nil { | ||
| startTimer() | ||
| } else if elapsedTime >= HeightExpansionTimeLimitInSeconds { |
There was a problem hiding this comment.
Why do we have this here and when the timer fires? Shouldn't we only need it when the timer fires? If we do need it in both places, can we pull out the common handling code into a function + add a comment about why it's called from both places?
There was a problem hiding this comment.
yeah I was on the fence about this. I will remove this so it's only in the location of the timer callback
| // MARK: - Timer | ||
|
|
||
| func startTimer() { | ||
| if timer == nil { |
There was a problem hiding this comment.
Slightly more swifty to make this a guard
| func startTimer() { | ||
| if timer == nil { | ||
| startTime = Date() | ||
| timer = Timer.scheduledTimer(withTimeInterval: 0.1, repeats: true) { [weak self] _ in |
There was a problem hiding this comment.
Maybe this time interval should be higher to prevent it from firing a bunch of times without doing anything?
There was a problem hiding this comment.
for sure, i changed this to 1 second rather than 0.1
| guard let self else { | ||
| return | ||
| } | ||
| if let start = startTime { |
There was a problem hiding this comment.
You can move this into the above guard
| public var expansionSettled: ((EmergeRenderingMode?, Float?, Bool?) -> Void)? { | ||
| private var startTime: Date? | ||
| private var timer: Timer? | ||
| private var elapsedTime: TimeInterval = 0 |
There was a problem hiding this comment.
Can we delete this and just make it a local var in the timeout handler? It doesn't seem to be used outside of that
There was a problem hiding this comment.
yes mb, not anymore. good call
| NSLog("ExpandingViewController: Expanding Scroll View timed out. Current height is \(firstScrollView?.visibleContentHeight ?? -1)") | ||
| runCallback(timeoutError) | ||
| } | ||
| print("Timer scheduled") |
There was a problem hiding this comment.
Can we delete this log? Seems like we don't need it in prod?
No description provided.