titanium-sdk icon indicating copy to clipboard operation
titanium-sdk copied to clipboard

feat(android): remoteNotificationsEnabled for Android 13

Open m1ga opened this issue 2 years ago • 0 comments

  • add parity for remoteNotificationsEnabled

This will return true is you enable push notifications in the app settings: Screenshot_20220604_122358

even if it is false you will still get a token from FCM! It just won't display a push notification.

To test it:

  • ~use https://github.com/tidev/titanium_mobile/pull/13473 to have the latest support libraries~ (merged)
  • all target and compile SDKs have to be set to "Tiramisu" (value has to be a string, so add " " around it)
  • change compileSdkVersion/targetSdkVersion to compileSdkPreview/targetSdkPreview
  • build apps and install them by hand with adb install -r -t app.apk since it is a test version

TODO:

  • other Android 13 push stuff

m1ga avatar Jun 04 '22 10:06 m1ga

Tested it in my firebase test app

console.log("remoteNotificationsEnabled", Ti.Network.remoteNotificationsEnabled);

The new push implementation is strange :smile: Even when I have it off I was able to send a push notification :thinking:

You can request the permission with:

var permissions = ['android.permission.POST_NOTIFICATIONS'];
Ti.Android.requestPermissions(permissions, function(e) {
  if (e.success) {
    Ti.API.info('SUCCESS');
  } else {
    Ti.API.info('ERROR: ' + e.error);
});

and after that Ti.Network.remoteNotificationsEnabled is true. I'll add that to fcm.registerForPushNotifications() so it will ask for permissions. Thought about using Ti.Network.registerForPushNotifications() but we don't get a token, just the permission so we still need the fcm part.

m1ga avatar Aug 16 '22 20:08 m1ga

Tested it on an Andorid 8 device. Ti.Network.remoteNotificationsEnabled returns true (as expected) :+1:

m1ga avatar Aug 18 '22 12:08 m1ga

This PR is great. But it does much more than mentioned in the title. We should therefore either split it up in Android 13 support and push notification changes or change the title to make it more expressive. Also more testing from the community would be great!

hansemannn avatar Aug 18 '22 12:08 hansemannn

you're right! the remoteNotificationsEnabled is only one part since we have the new Android 13 permission now. I've change the title.

m1ga avatar Aug 18 '22 12:08 m1ga

Looks fine from my end! Should we also bump some library versions? For example, material components and some other patch-version bumps? And yeah, I would also prefer having the permission check in registerForPushNotifications, even if we don't get a token on Android. It simplifies the cross platform permission handling a lot already.

hansemannn avatar Sep 10 '22 08:09 hansemannn

Sorry, missed the notification! Will have a look at it :+1:

material-components is the latest version already https://mvnrepository.com/artifact/com.google.android.material/material?repo=google (1.6.1). The rest is still alpha

m1ga avatar Sep 10 '22 19:09 m1ga

@m1ga I added the parity, updated the docs and fixed the version check (I suppose - as Manifest.permission.POST_NOTIFICATIONS should be checked for Android 13+, not Android 12+). Maybe I missed something and it was on purpose, so it would be great if you could check the commit in detail. After that, we should be good to go!

Here is my test case:

const window = Titanium.UI.createWindow();
window.addEventListener('open', () => refreshPermissionState());
const label = Ti.UI.createLabel({ top: 100 });

const btn = Ti.UI.createButton({ title: 'Request permissions', top: 200 });
btn.addEventListener('click', () => requestPermissions());

window.add([label, btn]);
window.open();

function refreshPermissionState() {
	const enabled = Ti.Network.remoteNotificationsEnabled;
	label.text = `Push Notifications enabled: ${enabled}`;
}

function requestPermissions() {
	Ti.Network.registerForPushNotifications({
		success: () => {
			alert('Push notifications ready!');
			refreshPermissionState();
		},
		error: () => {
			alert('Push permissions denied!');
			refreshPermissionState();
		}
	});
}

hansemannn avatar Sep 10 '22 20:09 hansemannn

Found two libs that have a newer version. Will test that together. Rest looks update to date (I've checked those in a different PR before)

m1ga avatar Sep 10 '22 20:09 m1ga

First quick tests (firebase push test app, my climbing app) looked good: permission check was shown, app works fine (I have a version of my app in store already with the first part of this PR :wink: ) But I'll do some more tests tomorrow.

BTW: since most people will use the FCM module in combination with this PR it will add POST_NOTIFICATIONS permission automatically. But it is still good to mention it in the docs :+1:

m1ga avatar Sep 10 '22 20:09 m1ga

Ready to merge from my end. If there are no other objections, this is good to go!

hansemannn avatar Sep 11 '22 11:09 hansemannn

Merge it :rocket: Hopefully more people will test the next RC to get some more feedback from other apps.

m1ga avatar Sep 11 '22 12:09 m1ga