[DRAFT] Impression observer logic added to tiles #29919
Conversation
| private var observedScrollViews: Set<UIScrollView> = [] | ||
|
|
||
| private var visibleAreaFraction: CGFloat { | ||
| guard let window = window, !isHidden, alpha > 0.01, !bounds.isEmpty else { return 0 } |
There was a problem hiding this comment.
I think I got this logic right... but I would love a sanity check here as I think I've stared at this code for too long
🧹 Tidy commitJust 3 file(s) touched. Thanks for keeping it clean and review-friendly! ❌ Per-file test coverage gateThe following changed file(s) are below 35.0% coverage:
Client.app: Coverage: 37.18
Generated by 🚫 Danger Swift against c64800e |
…lt in a second dwell fire during lifetime
fixed formatting formatting formatting
| private var inViewAreaFraction: CGFloat { | ||
| guard let window = window, !isHidden, alpha > 0.01, !bounds.isEmpty else { return 0 } | ||
| var rect = convert(bounds, to: window).intersection(window.bounds) | ||
| if rect.isNull { return 0 } |
There was a problem hiding this comment.
This should probably also be a guard !rect.isNull else { return 0 } (usually guard is used for this type of early-returning)
| var rect = convert(bounds, to: window).intersection(window.bounds) | ||
| if rect.isNull { return 0 } | ||
| var a = superview | ||
| while let s = a, s !== window { |
There was a problem hiding this comment.
I feel like this code would be clearer if broken out into a separate function with more well-defined variable names
There was a problem hiding this comment.
Yeah that's a good call. Let me clean this up! I was very fast and loose with this one.
| // MARK: Visibility timer | ||
| private func startVisibilityTimerIfNeeded() { | ||
| guard visibilityTimer == nil, !wasVisibleForThisLifetime else { return } | ||
| let t = Timer(timeInterval: visibleTimeThresholdSeconds, repeats: false) { [weak self] _ in |
There was a problem hiding this comment.
I believe you can use Timer's built-in initializer instead of instantiating it with its constructor directly so you don't need to manually add it to the run loop like:
visibilityTimer = Timer.scheduledTimer(withTimeInterval: visibleTimeThresholdSeconds, repeats: false) { [weak self] _ in
// do stuff...
}
| } | ||
| self.stopVisibilityTimer() | ||
| } | ||
| t.tolerance = visibleTimeThresholdSeconds * 0.1 |
There was a problem hiding this comment.
I don't think we should add this tolerance. In practice, this will just ensure that it almost always fires later than you intend.
adudenamedruby
added
the
Do Not Merge ⛔️
4 participants
DO NOT MERGE: This is a draft PR and is still a work in progress.
The goal of this PR is to include a new impression observer class that handles its own view state using a custom UICollectionViewCell base class.
This draft has been opened for comments on the approach. Any release of this code must be done behind a feature flag.
📝 Checklist