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

Add disableGMSMissingPrompt public method #1332

Merged
merged 2 commits into from
May 18, 2021

Conversation

Jeasmine
Copy link
Contributor

@Jeasmine Jeasmine commented May 13, 2021

  • Add public setter for disableGMSMissingPrompt, delay setter until remote params are set, do not override dashboard configuration
  • Use param under showUpdateGPSDialog

Fixes #1288


This change is Reviewable

@Jeasmine Jeasmine requested review from jkasten2 and emawby May 13, 2021 18:43
* Add public setter for disableGMSMissingPrompt, delay setter until remote params are set, do not override dashboard configuration
* Use param under showUpdateGPSDialog
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Jeasmine)


OneSignalSDK/onesignal/src/main/java/com/onesignal/GooglePlayServicesUpgradePrompt.java, line 47 at r1 (raw file):

         public void run() {
            final Activity activity = OneSignal.getCurrentActivity();
            if (activity == null || OneSignal.getDisableGMSMissingPrompt())

Some suggestions to improve this.

  1. Put this check in it's own if statement. (or add it to an existing one there are related checks)
  2. This kind of disable check would be better fitting at the top ofshowUpdateGPSDialog or with userSelectedSkip instead of calling it late inside this thread.

OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java, line 2623 at r1 (raw file):

    * This method will be replaced by remote params set
    */
   public static void setDisableGMSMissingPrompt(final boolean promptDisable) {

I think disableGMSMissingPrompt would be a better name. Having both set and disable is redundant.

* Rename from setDisableGMSMissingPrompt to disableGMSMissingPrompt
* Move disableGMSMissingPrompt check before changing thread
* Remove delay from disableGMSMissingPrompt, since this is data is needed immediately after setting remote params
@Jeasmine Jeasmine force-pushed the feature/add-disablegmsmissingprompt branch from acb55c0 to c514712 Compare May 17, 2021 22:43
@Jeasmine Jeasmine requested a review from jkasten2 May 17, 2021 22:43
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Jeasmine)

@Jeasmine Jeasmine merged commit dee9282 into master May 18, 2021
@Jeasmine Jeasmine deleted the feature/add-disablegmsmissingprompt branch May 18, 2021 14:43
@jkasten2 jkasten2 changed the title Add setDisableGMSMissingPrompt public method Add disableGMSMissingPrompt public method May 25, 2021
@jkasten2 jkasten2 mentioned this pull request May 25, 2021
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.

What happened to disableGmsMissingPrompt in version 4.X
3 participants