Skip to content
This repository was archived by the owner on Dec 29, 2022. It is now read-only.

Made location object creation from snapshot function public #57

Merged
merged 8 commits into from
Sep 13, 2017

Conversation

jonahbron
Copy link
Contributor

My particular application is watching location changes by location key, and querying for nearby locations. The ability to take the snapshot from the query and directly convert that into a location object for the next query would be very handy. This change makes that possible.

@googlebot
Copy link

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

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

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


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

1 similar comment
@googlebot
Copy link

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

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

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


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@jonahbron
Copy link
Contributor Author

I signed it!

@samtstern
Copy link
Contributor

@jwngr thoughts? This does seem useful but I assume you had a reason for making this private in the first place.

@jwngr
Copy link

jwngr commented Dec 12, 2016

I'm a bit confused as to the exact use case. The intent of the library is to use a GeoQuery's onKeyEntered() method to query for nearby locations. That listener returns the location is a digestible format, as can be seen in the example here.

I guess I am not fully against making this public, but I would like to understand the use case more before we do that.

BTW @jdimond was the one who wrote this code, not me. He may have some opinion on the matter as well.

@jonahbron
Copy link
Contributor Author

@jwngr Well, my intent was to have a bunch of users, each of which had a location attached to them. The code was querying for users nearby to each other one, based on ID. I wanted to query the location object directly to keep the GeoQuery up-to-date, but that didn't work, apparently because of a thread issue (#40).

So since that doesn't work, I guess my use-case for this has evaporated. If it's not possible to query the locations from the application in parallel with GeoFire, there's no real reason to need to perform that conversion. 😢

@googlebot
Copy link

CLAs look good, thanks!

@jonahbron
Copy link
Contributor Author

Actually, my use-case is still valid. I believe my app isn't working in AppEngine because of a bug (#40), not because of an inherent design flaw in my application. Once that's fixed, my use-case will again be valid.

@samtstern
Copy link
Contributor

@jonahbron looks like some of your AppEngine code got mixed up in this PR, is that intentional?

@jonahbron
Copy link
Contributor Author

@samtstern Oops, my bad. Let me fix that.

@jonahbron
Copy link
Contributor Author

@samtstern Fixed. Moved some stuff into the wrong branch.

@melashkov
Copy link

@jonahbron would you be able to squash commits?

@jonahbron
Copy link
Contributor Author

@melashkov I could probably figure out how to do that, but I'm pretty sure you can choose that in Github when you merge.

@vanniktech
Copy link
Contributor

Friendly ping here. @melashkov you can do this now with GitHub (Squash & merge)

@melashkov
Copy link

melashkov commented Sep 13, 2017

@anniktech I am not a owner of the repo, cant merge 😢

@samtstern
Copy link
Contributor

Thanks for the ping! I will merge this now.

@samtstern samtstern merged commit abf8421 into firebase:master Sep 13, 2017
@vanniktech
Copy link
Contributor

Could you also have a look at #76?

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.

6 participants