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

[TIMOB-24504] Android: Implement bundled notifications #9142

Merged
merged 8 commits into from Jul 28, 2017

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Jun 14, 2017

  • Implement bundled notifications for Android N
TEST CASE
var win = Ti.UI.createWindow({title: 'TIMOB-24504', layout: 'vertical'}),
    btn_normal = Ti.UI.createButton({title: 'CREATE NOTIFICATIONS', height: 50, width: Ti.UI.FILL}),
    btn_group = Ti.UI.createButton({title: 'CREATE GROUP NOTIFICATIONS', height: 50, width: Ti.UI.FILL}),
    id = 100;

function createNotifications(num, group) {
    for (var i = num; i > 0; i--) {
        var n = Ti.Android.createNotification({
            icon: Ti.Android.R.drawable.ic_dialog_info,
            contentTitle: 'NOTIFICATION #' + i,
            contentText : 'Lorem ipsum dolor sit amet, facer eruditi omittantur cu pri, nibh nonumy putant eam eu.'
        });
        if (group) {
            n.setGroup(group);
        }
        Ti.Android.NotificationManager.notify(id++, n);
    }
    if (group) {
        var n = Ti.Android.createNotification({
            icon: Ti.Android.R.drawable.ic_dialog_info,
            contentTitle: 'NOTIFICATIONS',
            contentText : 'You have ' + num + ' notifications.',
            group: group,
            groupSummary: true
        });
        Ti.Android.NotificationManager.notify(id++, n);
    }
}

btn_normal.addEventListener('click', function() {
    createNotifications(3);
});

btn_group.addEventListener('click', function() {
    createNotifications(3, 'group_key');
});

win.add([btn_normal, btn_group]);
win.open();
NOTE: this requires #9027

JIRA Ticket

@jquick-axway
Copy link
Contributor

@garymathews, your doc changes state that this feature is only supported on Android 7 and higher, but since we're using Google's NotificationCompat, the Java setGroup() and setGroupSummary() methods are (according to Google; link below) supposed to be supported on older Android OS versions as well.
https://android-developers.googleblog.com/2016/06/notifications-in-android-n.html

You may want to double check. Although, a stackoverflow post I've seen suggests that notification bundles via this compat library doesn't work on Android 4.0.
https://stackoverflow.com/questions/31483772/android-grouped-notifications-and-summary-still-shown-separately-on-4-4-and-bel

@jquick-axway
Copy link
Contributor

Also, I think the "group" property should be renamed to "groupKey".

This makes it clear to the reader that this property must be given a string key. Versus a name like "group" implies that it stores/provides a collection of items. I know what I'm suggesting doesn't match Google's equivalent Java method, but it does match how they document it.

@garymathews
Copy link
Contributor Author

@jquick-axway Updated PR.

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

Looks good. And definitely a great feature to add.

Just 1 question and 1 minor change request left.

/**
* @module.api
*/
public static final String PROPERTY_GROUP_ALERT_BEHAVIOUR = "groupAlertBehaviour";
Copy link
Contributor

Choose a reason for hiding this comment

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

Our APIs use American English spelling rules. So, we should rename "Behaviour" to "Behavior", without the 'u'. (Sorry)

This would match our spelling with iOS' notification behavior property...
http://docs.appcelerator.com/platform/latest/#!/api/Titanium.App.iOS.UserNotificationAction-property-behavior

// remove FLAG_AUTO_CANCEL as this will prevent group notifications
this.flags &= ~Notification.FLAG_AUTO_CANCEL;
}
notification.flags |= this.flags;
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding notification.flags |= this.flags, is there a reason why you are changing it from an assignment to a bitwise-or operation? (Just curious in case there is something I'm not seeing here.)

I ask because the NotificationBuilder might set bits that can no longer be removed and the developer would have no means of clearing them. Versus the developer used to be able to set the exact flags he/she wanted before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind. The "Notification.Builder" sets flags (such as group summary) that we don't want to clear with an assignment.

@garymathews
Copy link
Contributor Author

@jquick-axway Updated

@lokeshchdhry lokeshchdhry self-assigned this Jul 28, 2017
@lokeshchdhry
Copy link
Contributor

FR Passed.

The groupkey & the groupSummary properties works as expected. Also, notifications with this property group/bundle into a single notification.

Studio Ver: 4.9.1.201707200100
SDK Ver: 6.2.0 local build
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.3
Appc NPM: 4.2.9
Appc CLI: 6.2.3-21
Ti CLI Ver: 5.0.14
Alloy Ver: 1.9.13
Node Ver: 6.10.1
Java Ver: 1.8.0_101
Devices: Android 7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants