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

Full Support BigPicture(BannerMode) and Fixed issue of largeIcon using url [Android] #1444

Closed
wants to merge 1 commit into from

Conversation

admin2684
Copy link

@admin2684 admin2684 commented May 17, 2020

Now this library (react-native-push-notification) can support these features:

  • BigPicture with "imageUrl" key in Json notification data
  • Finally you can use https:// , http:// in largeIcon but you must using "largeIconUrl" key in Json notification data (Please remove largeIcon key )

IMAGE 2020-05-17 09:14:37

Note: These changes has not effect on other features (Without remove other features)

Thanks to @tidianeb5

@Dallas62 Please merge this pull request

Import Big Image Url and largeIconUrl feature for LocalNotification in android
@admin2684 admin2684 changed the title Support BigPicture(BannerMode) and Fixed issue of largeIcon using url [Android] Full Support BigPicture(BannerMode) and Fixed issue of largeIcon using url [Android] May 17, 2020
@lukhol
Copy link

lukhol commented May 19, 2020

I'm really waiting for this one. Hope it will be merged.

Copy link
Collaborator

@Dallas62 Dallas62 left a comment

Choose a reason for hiding this comment

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

Hi, unfortunately there is too many issues in the code and not only those listed below.
I will take a look to this feature since it might be important for some application.
I will come back to you ASAP.

Comment on lines +840 to +842
channel.setDescription(this.config.getChannelDescription());
channel.enableLights(true);
channel.enableVibration(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is duplicate with L829-831.

@@ -410,6 +449,7 @@ public void sendToNotificationCentre(Bundle bundle) {
notification.setVibrate(vibratePattern);
}

soundUri = RingtoneManager.getDefaultUri(RingtoneManager.TYPE_NOTIFICATION);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line introduce a regression by forcing sounds.

Comment on lines +170 to +171
Log.d(TAG, "sendNotificationWithImage: bitmao log: " + image);
Log.d(TAG, "sendNotificationWithImage: bitmao log: " + largeIconImage);
Copy link
Collaborator

Choose a reason for hiding this comment

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

A LOG_TAG constant already exist.

@@ -57,6 +70,8 @@
private static final long ONE_HOUR = 60 * ONE_MINUTE;
private static final long ONE_DAY = 24 * ONE_HOUR;

private String TAG = "RNPushNotificationHelper";
Copy link
Collaborator

Choose a reason for hiding this comment

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

A LOG_TAG constant already exist.

@@ -145,12 +160,15 @@ public void sendNotificationScheduledCore(Bundle bundle) {
} else {
getAlarmManager().setExact(AlarmManager.RTC_WAKEUP, fireDate, pendingIntent);
}
getAlarmManager().setExact(AlarmManager.RTC_WAKEUP, fireDate, pendingIntent);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is duplicate, result is:

        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
            if(allowWhileIdle && Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
                getAlarmManager().setExactAndAllowWhileIdle(AlarmManager.RTC_WAKEUP, fireDate, pendingIntent);
            } else {
                getAlarmManager().setExact(AlarmManager.RTC_WAKEUP, fireDate, pendingIntent);
            }
            getAlarmManager().setExact(AlarmManager.RTC_WAKEUP, fireDate, pendingIntent);
        } else {
            getAlarmManager().set(AlarmManager.RTC_WAKEUP, fireDate, pendingIntent);
        }

Comment on lines +354 to +358
} else {
if (largeIconResId != 0 && (largeIcon != null || Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP)) {
notification.setLargeIcon(largeIconBitmap);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code is also duplicate with L340-342

@Dallas62
Copy link
Collaborator

Thanks for your contribution, this helped to implement a solution to this. You can test the version on dev for this feature.
https://github.com/zo0r/react-native-push-notification/tree/dev

@Dallas62 Dallas62 closed this May 28, 2020
Dallas62 added a commit that referenced this pull request May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants