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

Remove Google Play Services dependency #4

Open
nicolas-raoul opened this issue Aug 13, 2016 · 15 comments
Open

Remove Google Play Services dependency #4

nicolas-raoul opened this issue Aug 13, 2016 · 15 comments
Labels

Comments

@nicolas-raoul
Copy link
Member

and all of the unused features like Geofence.

@misaochan
Copy link
Member

misaochan commented Aug 19, 2016

The specific dependencies that we absolutely cannot use are just these two, right? So I'm guessing our best bet is to try and find an open source alternative for them?

compile 'com.google.android.gms:play-services-location:8.4.0'
compile 'com.google.maps.android:android-maps-utils:0.3.4'

What do you think of using https://github.com/mapzen/LOST instead?

@nicolas-raoul
Copy link
Member Author

Why not, looks like an interesting project!

But we can probably scrap a lot of things before, though. Actually, what do we really need that is not already provided by the standard (open source) Android API? I think we only really need to get the user's location, and as you have implemented it in Commons you know it does not rely on Play Services.

@misaochan
Copy link
Member

Yeah, we can use the inbuilt location services to get user location.

I'm trying to figure out how to duplicate this method without Google Play and Google Maps though:


    public static String getClosestCity(LatLng curLatLng) {
        if (curLatLng == null) {
            // If location is unknown return test city so some data is shown
            return TEST_CITY;
        }

        double minDistance = 0;
        String closestCity = null;
        for (Map.Entry<String, LatLng> entry: CITY_LOCATIONS.entrySet()) {
            double distance = SphericalUtil.computeDistanceBetween(curLatLng, entry.getValue());
            if (minDistance == 0 || distance < minDistance) {
                minDistance = distance;
                closestCity = entry.getKey();
            }
        }
        return closestCity;
    }

SphericalUtil seems to be a Google Maps class, and LatLng (which is an easy way of passing the coords around) is GMS.

@nicolas-raoul
Copy link
Member Author

nicolas-raoul commented Aug 19, 2016

We can scrap cities.

The sample app was tourism-centric showed you all attractions in the city you are in. If you are near Sydney, it shows you all attractions in Sydney.

Our users are not concentrated in touristic locations, they are in suburbs, in the countryside, in mountains, etc. So we should scrap the concept of cities, like I started to do in some places of the code.

I tested the app with a lot of places (I pushed now), and it is not slow, so I don't think we have to worry for now and can display all data, like the webapp does.

When copy-pasting the code into the Commons app, I guess the Android Wear part can be scraped too, as the Commons app is not usable on Wear. Notifying via Wear when requested pictures are around, à la Pokémon Go, would sound fun but there are not enough points to make it worth the development effort for now, I would say :-)

@misaochan
Copy link
Member

Oh, I have already removed Geofence and Wearables haha. Shall I submit a PR?

@nicolas-raoul
Copy link
Member Author

Good! :-)
There is a small conflict, would you mind pulling then re-PRing?

@misaochan
Copy link
Member

Hm, not sure how to resolve the merge conflict. Will try and find out...

@nicolas-raoul
Copy link
Member Author

You have already handled many Git conflicts, right?
In the present case it is simple, mostly just remove my CSV URL modification to replace with yours.

@misaochan
Copy link
Member

I have not actually had to deal with merge conflict before, heh...

So we want to replace the URL in your repo with the URL in mine, right? I guess I will need write access to commons-app/ShootMe for that?

@nicolas-raoul
Copy link
Member Author

Oh I did not know, good opportunity to learn with a simple one, then :-)

  1. Run git pull [email protected]:commons-app/ShootMe.git
  2. Git will say that a conflict occurred, and place <<< markers in the files.
  3. Fix all of these marked places
  4. Commit, push, send PR again

@misaochan
Copy link
Member

Yeah, I've modified the part with the <<< markers, to remove your URL and put mine in. But it still says merge conflict exists. Should I be doing the fixing on my forked repo or on the commons-app repo?

@nicolas-raoul
Copy link
Member Author

Oh, I forgot one step.
To mark a file as merged, you have to git add it.

On Fri, Aug 19, 2016 at 4:47 PM, Josephine Lim [email protected]
wrote:

Yeah, I've modified the part with the <<< markers, to remove your URL and
put mine in. But it still says merge conflict exists. Should I be doing the
fixing on my forked repo or on the commons-app repo?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#4 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGFBssDqWC2AFYX73UnaraZwv88fLZhks5qhV-PgaJpZM4JjuVO
.

@nicolas-raoul
Copy link
Member Author

Merging is best done on one's forked repo.

On Fri, Aug 19, 2016 at 4:49 PM, Nicolas Raoul [email protected]
wrote:

Oh, I forgot one step.
To mark a file as merged, you have to git add it.

On Fri, Aug 19, 2016 at 4:47 PM, Josephine Lim [email protected]
wrote:

Yeah, I've modified the part with the <<< markers, to remove your URL and
put mine in. But it still says merge conflict exists. Should I be doing the
fixing on my forked repo or on the commons-app repo?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#4 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGFBssDqWC2AFYX73UnaraZwv88fLZhks5qhV-PgaJpZM4JjuVO
.

@misaochan
Copy link
Member

Sorry, I think I'm a bit confused about Step 3:

The conflict is marked at TouristAttraction.java:

**<<<<<<< HEAD

                //URL file = new URL("http://nicolas.raoul.free.fr/lab/wikishootme-test.csv");
                URL file = new URL("https://tools.wmflabs.org/wiki-needs-pictures/data/data.csv");

=======
                //URL file = new URL("http://nicolas.raoul.free.fr/lab/wikishootme-test.csv");
                URL file = new URL("http://nicolas.raoul.free.fr/lab/wikishootme-japan.csv");
>>>>>>> refs/remotes/commons-app/master**

So what I should do is remove all of that except URL file = new URL("https://tools.wmflabs.org/wiki-needs-pictures/data/data.csv");, is that correct? Then I can commit, add, and push?

@misaochan
Copy link
Member

I think it works now, could you check #11 and see? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants