Update Fuse.PushNotifications module to work with Android 8 and keeping backward compatibility #1197

mohammedmanssour · 2018-06-19T13:24:53Z

Hello!
Previously Push Notifications caused app crashes on Android 8 because Android 8 isn't supporting background jobs anymore

I made the change to use JobIntentService that can be scheduled and works well with all version of Android instead of plain Service
Added NotificationChannel logic That is required by Android 8 in order for notifications to function well.
updated dependencies.
and updated Fuse.Maps dependencies to avoid conflict with the new Fuse.PushNotifications
Documentation got updated accordingly.

That's all,
Thanks

kusma · 2018-06-19T13:48:58Z

Source/Fuse.PushNotifications/Android/Impl.uno

 			intent.setAction(PushNotificationReceiver.ACTION);
 			intent.replaceExtras(payload);
-			android.app.PendingIntent pendingIntent = android.app.PendingIntent.getActivity(context, id, intent, android.app.PendingIntent.FLAG_UPDATE_CURRENT);
+			android.app.PendingIntent pendingIntent = android.app.PendingIntent.getActivity(context, 0, intent, android.app.PendingIntent.FLAG_ONE_SHOT);


This seems to undo a recent fix... why is this needed?

would please explain why this important ? I dived in but understood nothing about this code so i got it back to the defaults

getting it back isn't a problem as long as there's a reason behind it
sorry for the inconvenience though

The PR for that change contains an explanation: #1185 (comment) :)

We don't just undo someone else's work without reason.

kusma · 2018-06-19T13:49:14Z

Source/Fuse.PushNotifications/Android/Impl.uno

@@ -296,29 +301,42 @@ namespace Fuse.PushNotifications
 		[Foreign(Language.Java)]
 		static void SpitOutNotification(Java.Object _listener, string title, string body, string bigTitle, string bigBody, string notificationStyle, string featuredImage, string sound, Java.Object _payload)
 		@{
-			int id = PushNotificationReceiver.nextID();
+			Log.d("Android Impl", "SpitOutNotification");


I doubt you want to have this change...

same as previous comment

Maybe I wasn't clear. I meant that introducing a debug-log here seems pretty pointless. The moving of the nextID-call is a separate matter.

kusma · 2018-06-19T13:49:30Z

Source/Fuse.PushNotifications/Android/Impl.uno

+					Uri defaultSoundUri= RingtoneManager.getDefaultUri(RingtoneManager.TYPE_NOTIFICATION);
+					notificationBuilder.setSound(defaultSoundUri);
+				}
+		    }


This block seems to have some indentation issues...

I'm using tabs for Indentation, is it good?

The last level of indentation here is done with spaces, not tabs. I suggest you enable visible white-space in your text-editor, and you'll see what's up...

kusma · 2018-06-19T13:50:37Z

Source/Fuse.PushNotifications/JS.uno

@@ -41,7 +41,7 @@ namespace Fuse.PushNotifications
 				"registrationSucceeded")
 		{
 			if (_instance != null) return;
-			Resource.SetGlobalKey(_instance = this, "FuseJS/Push");
+			Uno.UX.Resource.SetGlobalKey(_instance = this, "FuseJS/Push");


This seems unrelated to this commit...

without Uno.UX part, compiler shows Resource is ambiguous. If you're certain it can work without any error we can revert this change to the old thing

I don't see anything in this commit that introduces this ambiguity, so I don`t think this is the correct commit to do this in. Please correct me if I'm wrong.

This is the error that I'm encountering for Resource

Fuse.PushNotifications/JS.uno(44,5): Error E3109: 'Resource' is ambiguous Candidates are: Uno.UX.Resource Fuse.Reactive.Resource

Hmm, this sounds familiar. Does this also happen before this patch-series? If so, it probably deserves its own fix, and possibly a back-port...

Yes
this happens to me before this patch-series

kusma · 2018-06-26T18:52:31Z

@helilabs: Would you mind squashing in the different parts of 2973ef3 into their respective commits, so we get a clean history?

devadiab · 2018-07-01T11:59:05Z

Please keep aware for my change about intent flags.

devadiab · 2018-07-01T12:02:06Z

Depending to my experience, we have to deprecate this module totally and try to use this one
https://github.com/fuse-compound/Fuse.Firebase/tree/master/src/Firebase.Notifications.Android

mohammedmanssour · 2018-07-01T12:05:02Z

@kusma will be updating the commit soon and get it ready to be merged. but i think you should have a section in the README file the talks a little bit about commit message.

mohammedmanssour · 2018-07-01T12:07:04Z

@devadiab You're changes was kept.

regarding firebase plugin i don't it's not compatible with Android 8 (tested) plus official documentation show's that the compatible version is 11.+ but Fuse.Firebase is using version 9.2.0
https://github.com/fuse-compound/Fuse.Firebase/blob/master/src/Firebase/Core.uno#L15

devadiab · 2018-07-01T12:08:38Z

@helilabs i see that, i was emphasizing for that. you have good efforts but its the time to fully run on Firebase FCM not GCM anymore

devadiab · 2018-07-01T12:14:47Z

Also the Android 8+ not allowing the pending intent for GCM anymore !
See this :
https://stackoverflow.com/questions/46445265/android-8-0-java-lang-illegalstateexception-not-allowed-to-start-service-inten

kusma · 2018-07-02T06:42:45Z

@kusma will be updating the commit soon and get it ready to be merged. but i think you should have a section in the README file the talks a little bit about commit message.

@helilabs: Details about how to commit etc live in CONTRIBUTING.md; feel free to send a PR with clarifications there if you think something is missing. I agree that we're not talking much about that there as-is, so there's some undocumented expectations ATM.

mohammedmanssour · 2018-08-09T15:42:22Z

@kusma commits are now updated and sorry for being late

This seems to have broken around a year ago, yet nobody seems to have noticed until now.

…ard compability

…cation new updates

AndrewEQ · 2018-08-30T16:51:50Z

@devadiab @mohammedmanssour how bout we make a separate module for firebase push notifications? Perhaps call it "Fuse.FirebasePushNotifications"?

AndrewEQ · 2018-09-25T16:08:15Z

Hey guys, what be happening with this?

AndrewEQ · 2018-09-26T08:39:27Z

FYI: for others looking to get push working in Android 8 until new features are fulfilled: https://forums.fusetools.com/t/getting-push-notifications-to-work-in-android-8-with-super-little-change/7423

Both of these files uses both Uno.UX and Fuse.Reactive (which both provides a Resource class), but neither clarify which one they want to use. This leads to a compilation error on usage. So let's fix it, by adding the fully qualified name here. How this has gone unnoticed for this long is beyond me. I guess nobody is using this functionality? This was extracted out of fuse-open#1197, but another case was added as well.

ekkotech

Today, I tested this commit plus a5052b2 and d7bb8eb (this last one not related to Push Notifications but I included it anyway) on an Xperia XZ1 Compact running 8.0 (26). Set up to perform the testing was:

Clean install of Fuse 1.9
Clean install of Android (uno install android)
Update of platforms and build tools using sdkmanager (platforms 16 though 28); build-tools (26.0.3, 27.0.3, 28.0.3)
Replaced NDK 18 with 17c
Local copy of latest fuselibs as of 29th Sept.
Merged these commits into local copy of fuselibs
Project .unoproj:
- BuildToolsVersion: 28.0.3, CompileVersion: 28, MinVersion: 16, TargetVersion: 26
- New notification tags: "Name", "Description", "ChannelID"
Test notifications fired from curl script (to GCM)
Ran the test with uno build mytest.unoproj -t=android -c=release --run

Result: worked without any changes. Icon dot came up as expected, notifications came through to system tray or application depending on whether application was in foreground or not.
Observation: I could not create a situation where the notifications did not work. For any combination of ChannelID (in .unoproj) and channel tag in payload, notifications always came though. Deliberately mis-matching the channel id names or removing channel id completely from .unoproj had no effect - it still worked. I need to test more but it looks to me as if GCM ignores any channel id that you provide - possibly only FCM actually manages that data item. If this is the case, then channel id and all the other options available under 26+ cannot really be tested until fuselibs supports FCM. Having said that, it looks to me that this is a good "baseline" for supporting 26+ with GCM (the background fix is important and the icon dot is nice to have)
I have another test slot coming up on Friday so I'll try more variations then.

ckarmy · 2019-07-30T11:50:07Z

Closed. Fixed in ae566ac

kusma suggested changes Jun 19, 2018

View reviewed changes

mohammedmanssour added 4 commits August 30, 2018 17:00

Fix long-standing amgibgious name.

d7bb8eb

This seems to have broken around a year ago, yet nobody seems to have noticed until now.

Update Push Notification Module to work with android 8 and keep backw…

1aefa73

…ard compability

Updating Documentation to reflect the new changes

6f0d59c

Update Fuse.Maps dependencies to avoid confilict with Fuse.PushNotifi…

a5052b2

…cation new updates

kusma mentioned this pull request Aug 31, 2018

Change version of play services of Maps #1212

Merged

AndrewEQ mentioned this pull request Sep 30, 2018

Push Notification - iOS & Android Updates! (Now includes FCM - Fuse 1.10+) #1220

Merged

3 tasks

kusma mentioned this pull request Oct 2, 2018

Fuse.*Notifications: clarify ambiguous symbol #1227

Merged

ekkotech reviewed Oct 3, 2018

View reviewed changes

ckarmy closed this Jul 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Fuse.PushNotifications module to work with Android 8 and keeping backward compatibility #1197

Update Fuse.PushNotifications module to work with Android 8 and keeping backward compatibility #1197

mohammedmanssour commented Jun 19, 2018

kusma Jun 19, 2018

mohammedmanssour Jun 19, 2018

mohammedmanssour Jun 19, 2018

BeeWarloc Jun 19, 2018

kusma Jun 19, 2018

kusma Jun 19, 2018

mohammedmanssour Jun 19, 2018

kusma Jun 19, 2018

kusma Jun 19, 2018

mohammedmanssour Jun 20, 2018

kusma Jun 20, 2018 •

edited

Loading

kusma Jun 19, 2018

mohammedmanssour Jun 19, 2018

kusma Jun 19, 2018

mohammedmanssour Jun 20, 2018

kusma Jun 20, 2018

mohammedmanssour Jun 20, 2018

kusma commented Jun 26, 2018

devadiab commented Jul 1, 2018

devadiab commented Jul 1, 2018 •

edited

Loading

mohammedmanssour commented Jul 1, 2018

mohammedmanssour commented Jul 1, 2018

devadiab commented Jul 1, 2018

devadiab commented Jul 1, 2018

kusma commented Jul 2, 2018

mohammedmanssour commented Aug 9, 2018

AndrewEQ commented Aug 30, 2018

AndrewEQ commented Sep 25, 2018

AndrewEQ commented Sep 26, 2018

ekkotech left a comment

ckarmy commented Jul 30, 2019

Update Fuse.PushNotifications module to work with Android 8 and keeping backward compatibility #1197

Update Fuse.PushNotifications module to work with Android 8 and keeping backward compatibility #1197

Conversation

mohammedmanssour commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kusma Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kusma commented Jun 26, 2018

devadiab commented Jul 1, 2018

devadiab commented Jul 1, 2018 • edited Loading

mohammedmanssour commented Jul 1, 2018

mohammedmanssour commented Jul 1, 2018

devadiab commented Jul 1, 2018

devadiab commented Jul 1, 2018

kusma commented Jul 2, 2018

mohammedmanssour commented Aug 9, 2018

AndrewEQ commented Aug 30, 2018

AndrewEQ commented Sep 25, 2018

AndrewEQ commented Sep 26, 2018

ekkotech left a comment

Choose a reason for hiding this comment

ckarmy commented Jul 30, 2019

kusma Jun 20, 2018 •

edited

Loading

devadiab commented Jul 1, 2018 •

edited

Loading