Skip to content
This repository has been archived by the owner on Sep 4, 2020. It is now read-only.

allow forceShow option to always notify (android only atm) #178

Merged
merged 4 commits into from
Oct 2, 2015
Merged

allow forceShow option to always notify (android only atm) #178

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0aa884c
allow forceShow option to always notify
AdriVanHoudt Sep 29, 2015
2b842d9
use actual boolean
AdriVanHoudt Sep 30, 2015
5f1b59b
add documentation
AdriVanHoudt Oct 1, 2015
4c2f224
take care of more cases
AdriVanHoudt Oct 2, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ Parameter | Description
`options.android.sound` | `Boolean` Optional. If `true` it plays the sound specified in the push data or the default system sound. Default is `true`.
`options.android.vibrate` | `Boolean` Optional. If `true` the device vibrates on receipt of notification. Default is `true`.
`options.android.clearNotifications` | `Boolean` Optional. If `true` the app clears all pending notifications when it is closed. Default is `true`.
`options.android.forceShow` | `Boolean` Optional. If `true` will always show a notification, even when the app is on the foreground. Default is `false`.
`options.ios` | `JSON Object` iOS specific initialization options.
`options.ios.alert` | `Boolean` Optional. If `true` the device shows an alert on receipt of notification. Default is `false`.
`options.ios.badge` | `Boolean` Optional. If `true` the device sets the badge number on receipt of notification. Default is `false`.
Expand Down
6 changes: 5 additions & 1 deletion src/android/com/adobe/phonegap/push/GCMIntentService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,12 @@ protected void onMessage(Context context, Intent intent) {
// Extract the payload from the message
Bundle extras = intent.getExtras();
if (extras != null) {

SharedPreferences prefs = context.getSharedPreferences(PushPlugin.COM_ADOBE_PHONEGAP_PUSH, Context.MODE_PRIVATE);
boolean forceShow = prefs.getBoolean(FORCE_SHOW, false);

// if we are in the foreground, just surface the payload, else post it to the statusbar
if (PushPlugin.isInForeground()) {
if (!forceShow && PushPlugin.isInForeground()) {
Copy link
Member

Choose a reason for hiding this comment

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

Since we have two variables we need to account for 4 different cases.

  1. forceShow = false & PushPlugin.isInForeground() = true
  • should set foreground boolean to true and then fire the notification event.
  1. forceShow = true & PushPlugin.isInForeground() = true
  • should set foreground boolean to true, fire the notification event and show the notification in the shade
  1. forceShow = false & PushPlugin.isInForeground() = false
  • should set foreground boolean to false and then show the notification in the shade
  1. forceShow = true & PushPlugin.isInForeground() = false
  • should set foreground boolean to false and then show the notification in the shade

Obviously 3 and 4 are the same so we can handle that in an else but we need something like this un-optimized code:

            if (!forceShow && PushPlugin.isInForeground()) {
                extras.putBoolean(FOREGROUND, true);
                PushPlugin.sendExtras(extras);
            } else if (forceShow && PushPlugin.isInForeground()) {
                extras.putBoolean(FOREGROUND, true);
                PushPlugin.sendExtras(extras);

                // Send a notification if there is a message
                String message = this.getMessageText(extras);
                String title = getString(extras, TITLE, "");
                if ((message != null && message.length() != 0) ||
                        (title != null && title.length() != 0)) {
                    createNotification(context, extras);
                }
            } else {
                extras.putBoolean(FOREGROUND, false);

                // Send a notification if there is a message
                String message = this.getMessageText(extras);
                String title = getString(extras, TITLE, "");
                if ((message != null && message.length() != 0) ||
                        (title != null && title.length() != 0)) {
                    createNotification(context, extras);
                }
            }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't case 2) cause redundant behaviour? The notification event would be possibly triggered twice (one by the receipt of the message, and another by the click handler on the notificaton itself). I'm not saying that's wrong, I just think something else is needed to differ from both events, as both data objects will have the same properties as of right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Case 2 is wrong imo, the notification event should only be fired when actually clicked on the notification, as @fredgalvao said this could cause double events atm

Copy link
Member

Choose a reason for hiding this comment

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

I see what you both mean. Remember that was vaguely pseudo code I posted. I still think we'd need to take care of case 2 when the information pushed to the device does not include a title or message parameter. Perhaps the developer wants to silently push some info to the device and still trigger the notification event. So more like:

            if (!forceShow && PushPlugin.isInForeground()) {
                extras.putBoolean(FOREGROUND, true);
                PushPlugin.sendExtras(extras);
            } else if (forceShow && PushPlugin.isInForeground()) {
                extras.putBoolean(FOREGROUND, true);

                // Send a notification if there is a message
                String message = this.getMessageText(extras);
                String title = getString(extras, TITLE, "");
                if ((message != null && message.length() != 0) ||
                        (title != null && title.length() != 0)) {
                    createNotification(context, extras);
                } else {
                    PushPlugin.sendExtras(extras);
                }
            } else {
                extras.putBoolean(FOREGROUND, false);

                // Send a notification if there is a message
                String message = this.getMessageText(extras);
                String title = getString(extras, TITLE, "");
                if ((message != null && message.length() != 0) ||
                        (title != null && title.length() != 0)) {
                    createNotification(context, extras);
                }
            }

Copy link
Collaborator

Choose a reason for hiding this comment

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

That fixes it well enough for me, but not for all cases... Well, if anything the user can always rely on the notId and check for duplicate events, I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works, my train of thought was that when there is an actual notification shown don't fire the event but, forceShow would not affect this when there is nothing to show
@fredgalvao what cases aren't covered?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure of the practical cases, but in theory one could send a push notification with message === null and still want it to be shown with something else (like a big picture, or a summaryText, or whatever else he might provide other than message).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I am correct at least a message is required to show any notification

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right, the snippet @macdonst provided already contains the "message OR title" piece.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update today to match these comments

extras.putBoolean(FOREGROUND, true);
PushPlugin.sendExtras(extras);
}
Expand Down
1 change: 1 addition & 0 deletions src/android/com/adobe/phonegap/push/PushConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,5 @@ public interface PushConstants {
public static final String COUNT = "count";
public static final String FROM = "from";
public static final String COLLAPSE_KEY = "collapse_key";
public static final String FORCE_SHOW = "forceShow";
}
1 change: 1 addition & 0 deletions src/android/com/adobe/phonegap/push/PushPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ public void run() {
editor.putBoolean(SOUND, jo.optBoolean(SOUND, true));
editor.putBoolean(VIBRATE, jo.optBoolean(VIBRATE, true));
editor.putBoolean(CLEAR_NOTIFICATIONS, jo.optBoolean(CLEAR_NOTIFICATIONS, true));
editor.putBoolean(FORCE_SHOW, jo.optBoolean(FORCE_SHOW, false));
editor.commit();
}

Expand Down