geofire-java icon indicating copy to clipboard operation
geofire-java copied to clipboard

Fixes #134 and #28

Open jwiesmann opened this issue 6 years ago • 9 comments

Using keyRef.updateChildren instead of keyRef.setValue also fixes a huge amount of unnecessary 'onDataExited(DataSnapshot dataSnapshot)' events, which has not been reported yet. keyRef.updateChildren understands if the data needs to be inserted or updated. No need to use setValue here. Since priorities are not relevant anymore (https://stackoverflow.com/questions/31577915/what-does-priority-mean-in-firebase) i decided to not consider them anymore.

jwiesmann avatar Sep 01 '18 09:09 jwiesmann

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

googlebot avatar Sep 01 '18 09:09 googlebot

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

googlebot avatar Sep 01 '18 09:09 googlebot

I signed it!

jwiesmann avatar Sep 01 '18 09:09 jwiesmann

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot avatar Sep 01 '18 09:09 googlebot

lets try again :D

jwiesmann avatar Sep 01 '18 09:09 jwiesmann

..

jwiesmann avatar Sep 01 '18 09:09 jwiesmann

@jwiesmann thanks for sending this! The core (using update) LGTM, I just want to understand your reasoning on some minor points.

samtstern avatar Sep 04 '18 16:09 samtstern

Also don't worry about the CLA, I manually checked and you're good to go: image

samtstern avatar Sep 04 '18 16:09 samtstern

@hiranya911 would love your input here.

It seems like in the past, geofire was relying on the coincidence of the Android and Java RTDB SDKs having the same API surface. Then we use provided dependencies to provide the right code at runtime.

The recent Java changes break this (which is fine) but I wonder how we should proceed here. Should we lock geofire into the 5.x version of the Admin SDK forever?

samtstern avatar Sep 04 '18 17:09 samtstern