new method getActiveNotifications and fix problems

[netoramalho]

The new method getActiveNotifications return the payload of all active notifications in status bar, this way the developer can use the data to display indicators and get the id of the notifications to be able to cancel at any time.

I removed the TiGooshActivityLifecycleCallbacks file and onAppCreate method of TiGooshModule. It makes no sense to run the last notification automatically as the developer now can get them when you prefer.

When force_show_in_ foreground is true the sendMessage does not run, only when the user clicks on the notification.

http://stackoverflow.com/questions/3168484/pendingintent-works-correctly-for-the-first-notification-but-incorrectly-for-the
http://stackoverflow.com/questions/27946312/how-to-get-payload-of-each-and-every-notification

[progress44]

We were thinking of adopting the same approach after trying different solutions. Thanks for your contribution. We will make some tests and then we'll go on with accepting the pull request

[kopiro]

Hi @netoramalho, thank you for this huge PR! If you come and visit us at our headquarters in Parma we will offer you a coffee! ☕

By the way, the first target of this module is to keep the same syntax and behaviour of Ti.Network for iOS notifications.

For this reason, we (and you can help us to accomplish that) have to edit your PR a little bit to keep the behaviour that when the app is launched and a registerForPushNotifications is called, the clicked notification must be sent over the callback.

We can anyway image to keep the new method you expose.

Personally, I don't like this approach, but we have to remember that the main target of Titanium is to simplify the API across the platforms, and the best way to do that is to uniform behaviours.

What do you think?

[netoramalho]

Hi guys!
Thanks for the feedback, I thought about it and I agree with you.
Tomorrow I will start notifications tests for the iOS version of the app I'm working on. I will try to adapt this PR to maintain the same Ti.Network behavior.
It would be interesting to keep an option to disable it, for developers who do not want to get notifications when starting the app.

Soon I update this PR, thanks guys!

@kopiro Thanks for the invitation to take a coffee, you can be sure if one day go through Parma I will gladly accept the invitation =)

[netoramalho]

Hi guys!

I realized notifications testing for iOS and I could observe the following behavior: the callback is executed only when the user clicks on the notification or if the application receives the notification while in the foreground.

This is exactly the same behavior I am suggesting that PR. Perhaps I did not understand very well, at that point you do not observe the same behavior?

I added another commit this PR, now the payload received by the callback comes as json. Avoiding the developer to do things like this

var notification = (OS_ANDROID) ? JSON.parse(e.data) : e.data;

[DFoxinator]

@netoramalho I'm still playing with it, but this pull request is AWESOME! It seems to address most of the major issues with Android notifications in Appcelerator that I've yet to be able to solve. It's a massive step forward over what's being used elsewhere and also takes this module to the next level.

Things I tested (still need to test a lot more):

1. Simply opening a notif that has some data when the app is in the background - this works beautifully now. There used to be many problems where the callbacks would fire at weird times (ex. when you opened the app but not from one of the active notifs). Data gets passed correctly which was routinely an issue for me with other Android push notif modules.
2. Opening a notif when the app is closed and not in the background - works correctly, data gets passed
3. Opening a notif when there's multiple notifs existing - this worked perfectly to and was a problem in this module and all other ones. When you tap one of the notifs, the data for the correct notif gets passed (this is huge).

Sorry for how excited I am about this :) My app has been plagued with Android notif issues since we launched and they seemed unsolvable.

Awesome, awesome work on this. Please let me know if you'd like me to test anything specific or need any help with anything. I'm happy to contribute in any way possible. I think a lot of the present issues with Titanium notifications on Android get lost and aren't articulated often enough.

[DFoxinator]

Just to add - from my observations, @netoramalho is correct about iOS behavior. iOS notifications function perfectly in my app and this PR is the only Android implementation I've seen so far that seems to be equal to iOS behavior. Like I said before I haven't tested completely yet, but so far very promising.

[trogus]

@netoramalho awesome job on this! Android notifs not going where a user would expect was such an awkward friction point for us 🐩 🏆

[netoramalho]

@DFoxinator @trogus Thanks guys! This is my first contribution in an open source project and your comments motivate me a lot!

[DFoxinator]

We've had this in the production version of our app for over a week no and haven't had any issues. IMO it should be merged unless there are any concerns.

This also fixes #13

[kopiro]

Hi @DFoxinator and @netoramalho ,
we tried the PR but its behaviour is not correct so we couldn't merge it.

Specifically, by setting the flags of the launcher intent to:

launcherIntent.setFlags(Intent.FLAG_ACTIVITY_RESET_TASK_IF_NEEDED | Intent.FLAG_ACTIVITY_SINGLE_TOP);

the app is RESTARTED every time the user clicks on a banner.

This is not absolutely what we want, because we don't want to loose the context where the user was in.

Anyway, we are working on to find a right solution.

[progress44]

Moreover, the data can't be parsed to JSON by the module itself because it can crash the app in some occasions such as when the vibrate property is passed as an array

[DFoxinator]

The problem is right now there probably is no better solution because Titanium is inherently broken when it comes to the ability to create properly functioning push notifications. This ticket has all of the information: https://jira.appcelerator.org/browse/TIMOB-20459

It seems they almost have it corrected, but right now it's only a PR in 6.0.

Although this PR does restart the app, in our case, and I would think most normal use-cases, that doesn't really matter too much. Usually when a user is tapping a push notification, you're trying to re-engage them. Once an app is backgrounded, I don't think there's much expectation that navigating to the app through a push notification will leave the app as-is.

In my opinion it comes down to this: this PR offers the best solution for Android push notifications for the current state of Titanium. The behavior is consistent, you can tell which notif was tapped, and it's reliable. Without this PR, the behavior is very inconsistent, and literally the only usable functionality is navigating a user to the general app when they tap a notif. You cannot navigate them to a specific piece of content or rely on the data passed in the notification since you can't tell which notif was tapped.

I really think that while it's not ideal, it's far better than the current behavior of this module and the most popular Titanium Androud push notif module, https://github.com/morinel/gcmpush (which has the same major issue that you can't act on the specific notification that was tapped).

[kopiro]

Hi everybody, i made a new version that should fix all of your problems (ec5127a)

@netoramalho Could you submit a new PR (to do not loose the ownership of the commits) that includes the getActiveNotifications method you write, we want to expose it.

[DFoxinator]

@kopiro awesome, I'll test it out later!

[DFoxinator]

@kopiro I just got around to testing the new build. Unfortunately, it doesn't seem to really work for my app. I haven't tested it extensively, but just plugging in this version over the previous version that took advantage of this PR, a lot of functionality has been lost. I don't think the data from the notifications is getting passed through correctly. Also, if the app is opened and I tap a notification, the callback does not get called and when my app opens it doesn't actually act on the notification that was tapped.

[progress44]

Hi @DFoxinator, could you provide us some more details on the tests such as device model, android version and stuff like that. Did you build the module yourself or did you use the build from the repository?

We've tested the module on different devices and it's been working great.

[DFoxinator]

@progress44 apologies for my lack of information in my last comment. And sorry for the slow reply. Since that comment I've done a lot of testing and have found a few things.

1. I'm using the compiled 1.6.1 module provided in the dist of this repo
2. I was wrong about the data not getting passed through. I forgot that the JSON wasn't getting parsed automatically anymore so I had to call JSON parse manually to test it. Sorry about that.
3. For the most part, in my initial testing, it seems to work well so great job on that.
4. I found one case where I can't seem to get it to work correctly. If you force close the app, and then trigger a notification, tapping on the notification works as expected. It opens the app and the callback gets fired and the app properly acts on the notification. However, from that point, if I background the app and fire another notification, when I tap that notification the app opens but the callback doesn't seem to get fired. It's either not getting into the callback, or e.inBackground is false. I haven't figured out which one because I have to put some more debugging in but hoping you might know off hand. If I force close the app and open it normally for the initial open, rather than from a notif, the callback for push notifs fires every time (when coming from backgrounded to foreground) so it works as expected.

Thanks a bunch for putting this together and please let me know if what I said there makes sense or if you need any more information.

[DFoxinator]

I did some more testing and that second notif after the app was initially opened for a notif does not trigger the receive notif callback at all.

[kopiro]

@DFoxinator solved in 1.6.2

The solution has been to use this flags combinations:
948158e#diff-fe7b8fa1018505d17a4fc344737a619aL124

Cheers.

[DFoxinator]

@kopiro awesome, thanks for the quick fix! I will test a little bit later and confirm.

[DFoxinator]

@kopiro it seems to work perfectly, thanks a bunch! I'll continue to test and prepare to push out an update with this new version and let you know if I find any issues.