Skip to content

[Firebase_Database] Fix possible NullPointerException #30

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

Merged
merged 4 commits into from
Aug 26, 2019
Merged

[Firebase_Database] Fix possible NullPointerException #30

merged 4 commits into from
Aug 26, 2019

Conversation

juliocbcotta
Copy link
Contributor

Description

This PR fixes a possible NullPointerException that happens when the plugin is registered without a valid Activity.
I took the opportunity to add some nullability annotations that the IDE was suggesting, those are in the interfaces that are being implemented in the plugin, so should be safe to use.

Related Issues

#19

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@collinjackson collinjackson merged commit 4157ee3 into firebase:master Aug 26, 2019
@audkar
Copy link
Contributor

audkar commented Aug 26, 2019

@BugsBunnyBR @collinjackson isn't this PR changes FirebaseDatabase plugin behavior contract

from:

Always emit events on Main thread

to:

Always emit events on thread which on which plugin was instantiated

To be compatible with prievious behaviour this should be changed:

private final Handler handler = new Handler();

to this:

Handler handler = new Handler(Looper.getMainLooper());

?

@juliocbcotta
Copy link
Contributor Author

@audkar, Indeed this would break things, but when is plugin registration happening in any other thread than Main Thread? I think that registering plugins in any thread other than the Main Thread is a mistake and plugins should break if that happens as there is no support to do so in Flutter. (JNI stuff needs to happen in Main Thread as far as I know)

@audkar
Copy link
Contributor

audkar commented Aug 26, 2019

I manually create&register plugins in my app context :)

Of course I do that on main thread, but still I if would make mistake, than previous implementation would work and current not.

@collinjackson
Copy link
Contributor

collinjackson commented Aug 26, 2019

I don't feel strongly either way. I would be ok with @audkar's Looper.getMainLooper() approach, or we could enforce that we're on the main thread during plugin registration.

Salakar added a commit that referenced this pull request Aug 11, 2020
* feat(firebase_auth): v1 rework (#30)

Co-authored-by: Salakar <[email protected]>
Co-authored-by: Helena Ford <[email protected]>
Co-authored-by: ehesp <[email protected]>
Co-authored-by: Kirsty Williams <[email protected]>
Co-authored-by: Greg Hesp <[email protected]>
@firebase firebase locked and limited conversation to collaborators Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants