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

FIRMessagingRemoteNotificationsProxy updated to use GULAppDelegateSwizzler #2683

Merged
merged 31 commits into from Apr 4, 2019

Conversation

@maksymmalyhin
Copy link
Contributor

@maksymmalyhin maksymmalyhin commented Mar 29, 2019

Changes required for #2591

  • GULAppDelegateSwizzler - remote notifications methods support
  • FIRMessagingRemoteNotificationsProxy - swizzling of Application delegate updated to use GULAppDelegateSwizzler

NOTE: FIRMessagingRemoteNotificationsProxy still swizzles UNUserNotificationDelegate methods in its own way. I think, we should refactor it as a separate PR once we finish refactoring of app delegate swizzling in all libraries.

maksymmalyhin added 21 commits Mar 26, 2019
…public interface with no assumptions on the implementation details
Conflicts:
	Example/Messaging/Tests/FIRMessagingRemoteNotificationsProxyTest.m
…s applied to all targets. Move the hook to the top level to avoid confusion.
…etProxyOriginalDelegateOnceToken] at the beginning of each test.
@@ -272,3 +261,14 @@ target 'InstanceID_Example_tvOS' do
end
end

# Set define to turn on GUL_UNSWIZZLING and GUL_APP_DELEGATE_TESTING for the unit tests
post_install do |installer_representation|
Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Mar 29, 2019

Choose a reason for hiding this comment

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

Now (Cocoapods 1.6.1) there is only post_install hook supported for entire Podfile. It affects all targets anyways, so I moved it to the upper level to avoid confusion.

Loading

Copy link
Contributor

@tejasd tejasd Apr 3, 2019

Choose a reason for hiding this comment

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

Cool, thanks!

Loading

@@ -376,6 +459,18 @@ + (void)createSubclassWithObject:(id<UIApplicationDelegate>)anObject {
}
objc_setAssociatedObject(anObject, &kGULOpenURLOptionsSourceAnnotationsIMPKey,
openURLSourceAppAnnotationIMPPointer, OBJC_ASSOCIATION_RETAIN_NONATOMIC);
objc_setAssociatedObject(anObject, &kGULRealDidRegisterForRemoteNotificationsIMPKey,
Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Mar 29, 2019

Choose a reason for hiding this comment

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

@tejasd I used the same way of storing the original implementations as for the old methods. But it looks like it can be optimized to avoid boilerplate (e.g. store the implementations in a dictionary).

Also, I think, some repeated code may be moved to separate methods.

@tejasd What do you think?

Loading

Copy link
Contributor

@tejasd tejasd Mar 30, 2019

Choose a reason for hiding this comment

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

Yup, that's definitely an option! Feel free to refactor that in a separate PR :)

Loading

@maksymmalyhin
Copy link
Contributor Author

@maksymmalyhin maksymmalyhin commented Mar 29, 2019

@chliangGoogle I tested basic use-cases like receiving messages sent from the console. Are there any cases I need to pay a specific attention to.

Loading

@chliangGoogle
Copy link
Contributor

@chliangGoogle chliangGoogle commented Mar 29, 2019

Receiving messages are not enough for testing swizzling, if you look at the implementations, we swizzle because for a few reason. A. to be able to track analytics events, so would be great to check if Analytics is able to track notification_open or notification_foreground event; You can use the "debug mode" on Analytics console to see if a notification is opened or in foreground; B. when APNS token is updated, we need to notify FCM, so would be great if we can also make sure that logic is called. Basically we need to make sure all the "swizzled logic" is actually getting triggered. Does that make sense?

Loading

Loading

@interface FIRMessagingRemoteNotificationsProxy ()
@interface FIRMessagingRemoteNotificationsProxy () <UIApplicationDelegate>
Copy link
Contributor

@chliangGoogle chliangGoogle Mar 29, 2019

Choose a reason for hiding this comment

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

Is there an alternative than following this protocol? This makes macOS support very hard .

Loading

Copy link
Contributor

@tejasd tejasd Mar 30, 2019

Choose a reason for hiding this comment

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

Refer comment below.

Loading

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Apr 1, 2019

Choose a reason for hiding this comment

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

@chliangGoogle Does FCM support MacOS now? If not, I can take care of MacOS support for GULAppDelegateSwizzler in a separate PR. What do you think?

Loading

Copy link
Member

@paulb777 paulb777 Apr 1, 2019

Choose a reason for hiding this comment

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

Loading

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Apr 1, 2019

Choose a reason for hiding this comment

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

@paulb777 It looks like tvOS has no difference in the App Delegate, so supporting tvOS by GULAppDelegateSwizzler should be trivial. I'll update it in this PR

Loading

Copy link
Contributor

@chliangGoogle chliangGoogle Apr 2, 2019

Choose a reason for hiding this comment

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

Will we make GULAppDelegateSwizzler macOS support in this PR? If not, we can add a todo and fix in another PR.

Loading

Copy link
Contributor Author

@maksymmalyhin maksymmalyhin Apr 2, 2019

Choose a reason for hiding this comment

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

I created a ticket for this #2706

Loading

@@ -466,39 +389,36 @@ id getNamedPropertyFromObject(id object, NSString *propertyName, Class klass) {
return property;
}

#pragma mark - Swizzled Methods
#pragma mark - UIApplicationDelegate
Copy link
Contributor

@chliangGoogle chliangGoogle Mar 29, 2019

Choose a reason for hiding this comment

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

@tejasd Is this required that if I use GULAppDelegateSwizzler and I need to swizzle some methods under UIApplicationDelegate, my class must follow the protocol and implement the methods here?

Loading

Copy link
Contributor

@tejasd tejasd Mar 30, 2019

Choose a reason for hiding this comment

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

With the current design, yes. If that's a blocker we can create our own protocol, say GULAppDelegate which can be platform agnostic.

Loading

Copy link
Member

@paulb777 paulb777 left a comment

LGTM

Needs approval from @chliangGoogle

Loading

Copy link
Contributor

@chliangGoogle chliangGoogle left a comment

LGTM on FCM part.

Also need approval from @tejasd

Loading

Loading

@interface FIRMessagingRemoteNotificationsProxy ()
@interface FIRMessagingRemoteNotificationsProxy () <UIApplicationDelegate>
Copy link
Contributor

@chliangGoogle chliangGoogle Apr 2, 2019

Choose a reason for hiding this comment

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

Will we make GULAppDelegateSwizzler macOS support in this PR? If not, we can add a todo and fix in another PR.

Loading

Loading
tejasd
tejasd approved these changes Apr 3, 2019
@@ -107,6 +122,10 @@ - (void)userNotificationCenter:(UNUserNotificationCenter *)center
@end
#endif // __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_10_0

@interface GULAppDelegateSwizzler (FIRMessagingRemoteNotificationsProxyTest)
Copy link
Contributor

@tejasd tejasd Apr 3, 2019

Choose a reason for hiding this comment

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

Consider importing the private header.

Loading

@@ -272,3 +261,14 @@ target 'InstanceID_Example_tvOS' do
end
end

# Set define to turn on GUL_UNSWIZZLING and GUL_APP_DELEGATE_TESTING for the unit tests
post_install do |installer_representation|
Copy link
Contributor

@tejasd tejasd Apr 3, 2019

Choose a reason for hiding this comment

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

Cool, thanks!

Loading

Conflicts:
	GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m
	GoogleUtilities/Example/Tests/Swizzler/GULAppDelegateSwizzlerTest.m
@maksymmalyhin maksymmalyhin merged commit 267e9a6 into master Apr 4, 2019
2 checks passed
Loading
@maksymmalyhin maksymmalyhin deleted the mm/messaging-swizzling branch Apr 4, 2019
@chliangGoogle
Copy link
Contributor

@chliangGoogle chliangGoogle commented Apr 4, 2019

@maksymmalyhin Please make sure to test it with Analytics to ensure the notification tracking still works.

Loading

@firebase firebase locked and limited conversation to collaborators Oct 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants