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

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.

…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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, thanks!

@@ -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

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?

Copy link
Contributor

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 :)

@maksymmalyhin
Copy link
Contributor Author

@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.

@charlotteliang
Copy link
Contributor

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?


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

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 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Refer comment below.

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

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 created a ticket for this #2706

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

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

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?

Copy link
Contributor

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.

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

LGTM

Needs approval from @chliangGoogle

Copy link
Contributor

@charlotteliang charlotteliang left a comment

Choose a reason for hiding this comment

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

LGTM on FCM part.

Also need approval from @tejasd


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

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.

@@ -107,6 +122,10 @@ - (void)userNotificationCenter:(UNUserNotificationCenter *)center
@end
#endif // __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_10_0

@interface GULAppDelegateSwizzler (FIRMessagingRemoteNotificationsProxyTest)
Copy link
Contributor

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.

@@ -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

Choose a reason for hiding this comment

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

Cool, thanks!

Conflicts:
	GoogleUtilities/AppDelegateSwizzler/GULAppDelegateSwizzler.m
	GoogleUtilities/Example/Tests/Swizzler/GULAppDelegateSwizzlerTest.m
@maksymmalyhin maksymmalyhin merged commit 267e9a6 into master Apr 4, 2019
@maksymmalyhin maksymmalyhin deleted the mm/messaging-swizzling branch April 4, 2019 13:26
@charlotteliang
Copy link
Contributor

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

@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
5 participants