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

Single response #445

Merged
merged 5 commits into from
Aug 5, 2021
Merged

Single response #445

merged 5 commits into from
Aug 5, 2021

Conversation

Jeasmine
Copy link
Contributor

@Jeasmine Jeasmine commented Jul 27, 2021

Add single response handler

  • Some calls might end being called twice from the native side. Even if we already cover this from the native side, add safe
    check to avoid more than one call to the same flutter response

Clear handlers commit already approved, review commit "Add single response handler"


This change is Reviewable

@Jeasmine Jeasmine changed the base branch from fix/sms-userid to main July 27, 2021 15:54
@Jeasmine Jeasmine changed the base branch from main to fix/sms-userid-number July 27, 2021 15:55
@Jeasmine Jeasmine changed the title Fix/single response Single response Jul 27, 2021
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.

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @emawby, @Jeasmine, @nan-li, and @tanaynigam)


android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 161 at r1 (raw file):

  private void addObservers() {
    // Clean observers before setting, avoid being call twice

These are already in main. Looks like this branch is branched from fix/sms-userid-number, so once that is merged in this diff will probably be fixed.


android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 390 at r1 (raw file):

  static class OSFlutterEmailHandle extends FlutterRegistrarResponder
          implements OneSignal.EmailUpdateHandler {

Why do we have OSFlutterEmailHandle and then OSFlutterResultHandler for all the other callbacks? This is inconsistent, they should either all be their own class or all under the same one.


android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 405 at r1 (raw file):

    public void onSuccess() {
      if (this.replySubmitted.getAndSet(true))
          return;

Can we also print a warning to the logcat if this happens? This way we have it if we need to debug in the future.

Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Jeasmine, @jkasten2, @nan-li, and @tanaynigam)


android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 390 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Why do we have OSFlutterEmailHandle and then OSFlutterResultHandler for all the other callbacks? This is inconsistent, they should either all be their own class or all under the same one.

It looks like the other callbacks are sharing a success block (email is the only channel sending null in replySuccess(), but aren't sharing failure blocks. Since we already aren't sharing failure blocks I think we should have separate handlers for each channel

Base automatically changed from fix/sms-userid-number to main August 2, 2021 18:46
* Avoid handlers being set more that once, native side will keep reference too all handlers set
* Some calls might end being called twice from the native side. Even if we already cover this from the native side, add safe
check to avoid more than one call to the same flutter response
* Add base handler
* Add log for handler called more than once
Copy link
Contributor

@emawby emawby left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @emawby, @Jeasmine, @jkasten2, @nan-li, and @tanaynigam)


android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 402 at r2 (raw file):

          implements OneSignal.EmailUpdateHandler {

    OSFlutterEmailHandle(PluginRegistry.Registrar flutterRegistrar, MethodChannel channel, Result res, String methodName) {

Typo in the name. This should be OSFlutterEmailHandler

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.

This PR will cover the issue so approving. We probably want to try find a better balance with code reuse where possible but still have consistency for the future with these callbacks.

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Jeasmine, @jkasten2, @nan-li, and @tanaynigam)

@Jeasmine Jeasmine requested a review from emawby August 5, 2021 20:01
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 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Jeasmine, @jkasten2, @nan-li, and @tanaynigam)

@Jeasmine Jeasmine merged commit 8a189c9 into main Aug 5, 2021
@Jeasmine Jeasmine deleted the fix/single-response branch August 5, 2021 20:08
@Jeasmine Jeasmine mentioned this pull request Aug 5, 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.

3 participants