iOS FCM binding & Android 8+ FCM fix issues #82
Conversation
devadiab
force-pushed
the
upgrade-FCM-iOS
branch
from
53bf8da to
459eddb
Compare
|
@LuisRodriguezLD or @jesusmartinoza could you review this PR ? |
kusma
left a comment
kusma
left a comment
There was a problem hiding this comment.
I really don't get the iOS-specific code in Fuse.Firebase.Android here, but I see that there was already some precedence for it... Seems strange to me.
| "google.ttl", | ||
| "collapse_key"})); | ||
|
|
||
|
|
| public void DoIt(RemoteMessage remoteMessage) | ||
| { | ||
| Map<String,String> map = remoteMessage.getData(); | ||
|
|
| } | ||
|
|
||
| NotificationCompat.Builder notificationBuilder = new NotificationCompat.Builder(context) | ||
| mBuilder = new NotificationCompat.Builder(context) |
There was a problem hiding this comment.
Please keep the old name here, as it'll keep the delta smaller...
| mBuilder = new NotificationCompat.Builder(this,channel_id); | ||
| } | ||
| else{ | ||
| mBuilder = new NotificationCompat.Builder(this); |
There was a problem hiding this comment.
Why go through this dance to allocate mBuilder, only to overwrite it a few lines further down?
| .setColor((int)0x@(Project.Android.NotificationIcon.Color)) | ||
| #endif | ||
| .setContentIntent(pendingIntent); | ||
| #endif .setContentIntent(pendingIntent); |
There was a problem hiding this comment.
This seems like a mistake. Don't mix preprocessor directives with code. Please put these on separate lines... wasn't the old way right anyway?
|
|
||
| static void OnRegistrationSucceedediOS(string message) { | ||
| //_onRegistrationSucceedediOS.RaiseAsync(message); | ||
| debug_log "OnRegistrationSucceedediOS :"+message; |
There was a problem hiding this comment.
please drop random debug-code
|
|
||
| - (void)messaging:(FIRMessaging *)messaging didReceiveRegistrationToken:(NSString *)fcmToken { | ||
| NSLog(@"FCM registration token: %@", fcmToken); | ||
| // NSLog(@"FCM registration token: %@", fcmToken); |
There was a problem hiding this comment.
Rather delete than comment out, please
| @{Firebase.Notifications.NotificationModule.OnRegistrationSucceedediOS(string):Call(fcmToken)}; | ||
| - (void)messaging:(nonnull FIRMessaging *)messaging | ||
| didReceiveMessage:(nonnull FIRMessagingRemoteMessage *)remoteMessage{ | ||
| // NSLog(@"Received data message: %@", remoteMessage.appData); |
There was a problem hiding this comment.
Please don't add commented out code
| // NSLog(@"Received data message: %@", remoteMessage.appData); | ||
| } | ||
| /* | ||
| TODO: Not fired but it should be fired ! |
There was a problem hiding this comment.
What implication does this TODO have? Does the code work?
| FireNotificationCallbacks* fireCB = [[FireNotificationCallbacks alloc] init]; | ||
| @{_iosDelegate:Set(fireCB)}; | ||
| [FIRMessaging messaging].delegate = (id<FIRMessagingDelegate>)fireCB; | ||
| */ |
There was a problem hiding this comment.
please don't add commented out code
|
Lol, I'm just busy a bit, i have fixed most of those review points. I need some extra time ;) |
|
I closed it by mistake, sorry. I am actually testing this right now 👍 |
|
It have some issues, i have to send a commit soon so you can test ! |
|
Yeah, notifications are complicated. I'll try to make it work |
devadiab
force-pushed
the
upgrade-FCM-iOS
branch
7 times, most recently
from
bfccaac to
54e8c85
Compare
kusma
left a comment
kusma
left a comment
There was a problem hiding this comment.
Only minor complaints, looking pretty good!
I wouldn't mind a litte blit more explanation in the commit message, and maybe factor out some stuff like code-moving to a separate commit, so it's easier to follow?
| { | ||
| android.net.Uri defaultSoundUri= RingtoneManager.getDefaultUri(RingtoneManager.TYPE_NOTIFICATION); | ||
| notificationBuilder.setSound(defaultSoundUri); | ||
| notificationBuilder .setSound(defaultSoundUri); |
There was a problem hiding this comment.
Please don't add spaces link this. There's more of this below...
| x(null, message); | ||
| } |
There was a problem hiding this comment.
While I'm not a fan of these braces in the first place, could pure code-style changes like this be done as a separate PR?
There was a problem hiding this comment.
I'm trying to say that removing the braces, even if it's a nice cleanup, is out of scope for this PR. It makes it harder to read what the change is about, and if we need to revert the intended change for some reason, we'd lose the cleanup,
There was a problem hiding this comment.
Same as below, I don't feel strongly about this, and I'd be fine with landing this as-is also, to get things working on Android 8.
| if( jsonStr != "{}") | ||
| { | ||
| @{OnRecieve(string,bool):Call(jsonStr, false)}; | ||
| } |
There was a problem hiding this comment.
This looks like a change that could be factored out into it's own commit with an explanation in the commit message. It looks very reasonable IMO, but I'd love to hear why :)
There was a problem hiding this comment.
I really didn't what you need to know really, but this code snippet handle the following case :
To distignuesh between Normal app open and Open from notification tray, because of we are listening android.intent.action.MAIN due to Android 8+ app start new policy.
The android.intent.action.MAIN filter catch each app start, while the getIntent() on java provides me the data that attached with app launch. Normal open holds nothing while the open from Notification tray holds the notification data ;)
Hope this goes clear
There was a problem hiding this comment.
Thanks for explaining. If I understand correctly, you're saying that this prevents us from calling OnReceive (with empty data) when the app was started normally? If so, I think it would be nice to factor that out into a separate commit that explains that (in the commit message), so future generations will understand what's going on.
I'm not feeling very strongly about this, though. If it's a huge hassle, we can just leave it. Perhaps add a comment? Dunno...
| notificationManager.createNotificationChannel(mChannel); | ||
| } | ||
| // Android > 8 support for Channels because it not supported on previous versions | ||
| notificationBuilder = new NotificationCompat.Builder(this,channel_id); |
There was a problem hiding this comment.
space between comma and channel_id.
| if(android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.O) | ||
| { | ||
| NotificationChannel mChannel = notificationManager.getNotificationChannel(channel_id); | ||
| if (mChannel == null) { |
There was a problem hiding this comment.
I know this project isn't completely consistent when it comes to brace-style, but it'd be nice if these two if-blocks were consistent, at least. I'm no Java expert, but whatever is idiomatic there is probably the right choice...
|
But, this PR now seems to conflict. Could you rebase it, so it could be merged once CI pass? |
|
@kusma, those files are deleted now, can you resolve those conflicts ? |
|
@devadiab: I don't know what the right resolution for these files are... |
|
I have no access to resolve those conflict ! |
|
@devadiab: Yes, you do. You rebase your branch on master... |
|
To my master or to upstream/master ? |
|
@devadiab: You need to rebase onto the upstream master, yes. |
devadiab
force-pushed
the
upgrade-FCM-iOS
branch
from
8baa04e to
0adb49a
Compare
devadiab
force-pushed
the
upgrade-FCM-iOS
branch
from
0adb49a to
28b71a4
Compare
| */ | ||
| ======= | ||
|
|
||
| >>>>>>> iOS FCM binding & Android 8+ FCM fix issues:src/Firebase.Notifications/iOS/iOSFirebaseNotificationCallbacks.mm |
devadiab
force-pushed
the
upgrade-FCM-iOS
branch
from
98a9193 to
2040d46
Compare
|
Context on Once this is in we could probably mark |
|
@devadiab has the APNS stuff definitely been working for you? I noticed a few things that confused me compared to the documentation.
|
|
Also the callback method here is empty. IIRC this is used for data messages (which are the kind we care most about). Are the APNS messages using the AppDelegate callback instead? |
|
@cbaggers, thanks for this rich feedback it covers all of my contribution ;)
About the I'd like also to improve this project |
|
Can't wait for this to be on |
|
@LuisRodriguezLD: well, from a short peek, it looks like something went wrong in a rebase here. This PR used to delete rename the package from Fuse.Notifications.Android to Fuse.Notifiations, but now it seems to just add a modified copy of the code. Unless there's an explicit "yeah, we want to keep the old code as-is for [good reasons]"-explanation (I can't see one), I'm skeptical to duplicating code. As for the actual code, I think this PR is still way too dense. Several changes are needlessly baked into a single commit. Here's what I think would be a good way of doing this:
In addition, there's some thing in the code that doesn't quite look right:
I also think @cbaggers's ack here would be great. |
kusma
left a comment
kusma
left a comment
There was a problem hiding this comment.
Here's some more direct code-review, in addition to the stuff above.
| extern(Android) | ||
| static NotificationService() | ||
| { | ||
| Firebase.Core.Init(); |
There was a problem hiding this comment.
This seems to undo what @LuisRodriguezLD did in c8542d9. You guys need to decide on what the right code to initialize here is. Or make the code able to initialize correctly from multiple locations or something...
There was a problem hiding this comment.
This was actually my fault. But I've done exactly that, the core now initializes the core and makes sure it does not happen twice.
| _receivedNotification += value; | ||
| foreach (var n in _pendingNotifications) | ||
| { | ||
| debug_log "_pendingNotifications : "+n.Key+" fromNotificationBar:"+n.Value; |
There was a problem hiding this comment.
This probably shouldn't be left in.
| public static class NotificationService | ||
| { | ||
| extern(Android) | ||
| static NotificationService() |
There was a problem hiding this comment.
Android and iOS aren't the only two platforms; removing the "Firebase Notifications are not yet available on this platform"-message here doesn't seem right. You should probably update the extern for that platform to be something like extern(!Android && !iOS).
| @@ -0,0 +1,3 @@ | |||
| <Extensions Backend="CPlusPlus" Condition="iOS"> | |||
| <Define AppDelegate.PushNotificationMethods="1" /> | |||
| </Extensions> No newline at end of file | |||
There was a problem hiding this comment.
This could use a newline at the end of the file.
|
|
||
| String channel_name = "my_package_channel"; | ||
| String channel_id = "my_package_channel_1"; | ||
| String channel_description = "my_package_first_channel"; |
There was a problem hiding this comment.
channel_description is unused, please remove it.
|
Sorry I've been off for a while. I have Firebase APNS working in my fork (along with topics). It includes the fixes I was talking about previously and has been tested in a soon to ship app which has been through Apple's app store review. One big caveat is that APNS does not like data notifications, so I recommend using the new topics feature (also in my fork) to send data messages to android & notification messages to iOS. Using this we could deliver to all our devices in 2 http requests. This is a limitation that is outside of our control, apple does like data messages but our android integration requires them to provide a consistent experience. I think I'd like to clean up my fork and make a new PR for that if it's ok with you. We could port the work over to this PR but we risk introducing bugs. [EDIT] woops, forgot to link the branch |
4 participants
Hi There,
This is the final complete solution used on my enterprise projects contains the following:
1- FCM iOS binding it was only contains the ID registration code.
2- Upgrade the library so it will work 100% correct with Android 8+.
The key for changes on Android 👍
1- Upgrade com.google.firebase:firebase-messaging to version 17.
2- Change the Action on Intent receiver because Android 8+ run the notification as the launcher (android.intent.action.MAIN).
3- Fix Intent and Pending Intent flags so all notifications are clickable and each one contains it's data.
Regards,