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

Remove old app delegate, rename states and introduce Resuming (a.k.a willEnterForeground) #3809

Merged
merged 9 commits into from
Jan 16, 2025

Conversation

jaceklyp
Copy link
Contributor

Task/Issue URL: https://app.asana.com/0/0/1209022783720243/f
Tech Design URL: https://app.asana.com/0/0/1209144186921382/f
CC: @bwaresiak

Description:

  • remove old app delegate code,
  • rename states and events, so they better reflect what is going on
  • introduce some additional very much needed transitions
  • introduce Resuming state

Steps to test this PR:

  • smoke test the app, make sure handleUnexpectedEvent is never called

@jaceklyp jaceklyp requested a review from dus7 January 15, 2025 15:01
Copy link

github-actions bot commented Jan 15, 2025

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 9483c31

Copy link
Contributor

@dus7 dus7 left a comment

Choose a reason for hiding this comment

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

Works fine for me. Didn't hit any unexpected events.

I left two comments. They are not critical, but would be good to plan for resolving those in your project.

}

func application(_ app: UIApplication, open url: URL, options: [UIApplication.OpenURLOptionsKey: Any] = [:]) -> Bool {
realDelegate.application?(app, open: url, options: options) ?? false
appStateMachine.handle(.openURL(url))
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we ok with always returning true here? I saw there's false supposed to be returned when onboarding is present (based on the Old Delegate).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw this and AFAIU the documentation correctly we can always return true, just like we do inside didFInishLaunching but we do not necessarily have to handle the opening url logic, hence the code inside Foreground state:

guard mainViewController.needsToShowOnboardingIntro() == false else {
return
}

}

/// It's public in order to allow refreshing on demand via Debug menu. Otherwise it shouldn't be called from outside.
func refreshRemoteMessages() {
realDelegate.refreshRemoteMessages()
// part of debug menu, let's not support it in the first iteration
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll need this now, won't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it doesn't work now, as we already use new app delegate by default, but you are correct, this should be brought back, I'll make it work in next PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it in this PR!

@jaceklyp jaceklyp merged commit a1293c6 into main Jan 16, 2025
13 checks passed
@jaceklyp jaceklyp deleted the jacek/refactor-app-delegate-m1.5 branch January 16, 2025 16:38
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.

2 participants