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

RUMM-1218 App going background stops active RUM view #479

Merged
merged 3 commits into from
May 5, 2021

Conversation

buranmert
Copy link
Contributor

What and why?

When the app goes to background, technically there is no active view. Previous behavior didn't match that.

How?

When the app goes to background (by resigning active) UIKitRUMViewsHandler stops the last started RUM view and keeps a weak reference to it.
When the app comes back to foreground (IOW when it does become active) UIKitRUMViewsHandler re-starts that view.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

@buranmert buranmert self-assigned this May 3, 2021
@buranmert buranmert requested a review from a team as a code owner May 3, 2021 21:02
Comment on lines 39 to 41
if lastActiveAppVC != nil {
return
}
Copy link
Member

Choose a reason for hiding this comment

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

What if the lastActiveAppVC is not nil but doesn't match the lastStartedViewController ? When the app comes back in background, won't you be starting the wrong RUM View ?

Copy link
Member

Choose a reason for hiding this comment

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

Also, I'm wondering if we need the lastActiveAppVC property at all. As far as I can see, we either set it to lastStartedViewController or to nil, so can't we simply do:

  • if the app goes to background: stop lastStartedViewController,
  • when it goes to foreground: start lastStartedViewController
    ?

If there's sth special about lastActiveAppVC that I miss, I think it's worth adding it in the property comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xgouchet good point.
now i remove lastActiveAppVC along with @ncreated comments below. it was introduced to guard against multiple notifications received but probably not really needed.

NotificationCenter.default.post(name: UIApplication.willResignActiveNotification, object: nil)
}
dateProvider.advance(bySeconds: 1)
for _ in 0..<3 {
Copy link
Member

Choose a reason for hiding this comment

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

It's quite unclear why we call it 3 times - I guess it's just for sanity, no? Maybe it's worth expressing somehow (either with an inline comment or by labelling 3 with a named variable).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed ✅

Comment on lines 39 to 41
if lastActiveAppVC != nil {
return
}
Copy link
Member

Choose a reason for hiding this comment

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

Also, I'm wondering if we need the lastActiveAppVC property at all. As far as I can see, we either set it to lastStartedViewController or to nil, so can't we simply do:

  • if the app goes to background: stop lastStartedViewController,
  • when it goes to foreground: start lastStartedViewController
    ?

If there's sth special about lastActiveAppVC that I miss, I think it's worth adding it in the property comment.

@buranmert buranmert requested review from xgouchet and ncreated May 4, 2021 09:17
buranmert added 2 commits May 4, 2021 14:34
Implementation simplified
UIApplication notifications are listened in two places
Unit tests of both can be running at the same time
That leads to flaky or crashing tests
Now those places listen to different NotificationCenters in unit tests
@buranmert buranmert force-pushed the buranmert/RUMM-1218-stop-view-when-background branch from cbd0491 to e2ddf1d Compare May 4, 2021 12:35
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

LG 👌

@buranmert buranmert merged commit 4044b7d into master May 5, 2021
@buranmert buranmert deleted the buranmert/RUMM-1218-stop-view-when-background branch May 5, 2021 08:07
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.

3 participants