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

[WIP] MapboxGL Android 4.x.x #22

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

tnightingale
Copy link
Contributor

I have need of the new API Mapbox recently released in their 4.0.0-beta for android so I have updated my fork of this plugin to support it. While doing so I have made significant changes to the plugin's API to allow for multiple map instances and to more closely mirror the mapbox-gl-js API.

I will continue to work on adding features to this branch as I need them; next will be offline map support and bringing existing work from my geojson branch.

I am interested to hear your thoughts on this approach and whether it might make a good candidate for a future version of this plugin's API.

@dagatsoin
Copy link

Hi @tnightingale I have also need to use offline map in Mapbox on Android and iOS. Have you already implemented both or I can help you? Note I am very new to Cordova plugin but I can be full time on it.

I plan also to expose some useful callbacks like onRegionWillChange/onRegionChanged in my fork. Unless you have already done it?

@tnightingale
Copy link
Contributor Author

Hi @dagatsoin,
Yes this branch contains a refactored javascript API for creating maps and offline regions. My proposed javascript api for offline regions currently looks like this:

var cabo = {
        name: "Cabo San Lucas",
        style: "emerald",
        minZoom: 0,
        maxZoom: 16,
        bounds: {
            north: 22.891,
            east: -109.919,
            south: 22.879,
            west: -109.905
        }
    };

Mapbox.listOfflineRegions(function (err, regions) {
    if (err) return onError(err);
    console.log('listOfflineRegions()', regions);

    var region = regions[cabo.name];

    // First load will download region.
    if (!region) {
        region = Mapbox.createOfflineRegion(cabo);

        region.on("error", function (e) {
            console.error("OfflineRegion onError", e);
        });

        region.on("progress", function (progress) {
            console.log("OfflineRegion download onProgress", progress);
        });

        region.on("complete", function (progress) {
            console.log("OfflineRegion download onComplete", progress);
        });

        region.download();
    }
    // Subsequent loads will display offline region download status.
    else {
        region.getStatus(function (err, status) {
            if (err) return onError(err);
            console.log("OfflineRegion getStatus()", status);
        });
    }
});

At this point I have only implemented this for Android as that is where my needs are. It would be great to see an iOS implementation too.

@tnightingale
Copy link
Contributor Author

Here's an example of what the refactored javascript API looks like for creating a new map:

var map = new Mapbox.Map({
        style: 'emerald',
        zoom: 15,
        center: [-109.912, 22.885],
        showUserLocation: true,
        margins: {
            left: 0,
            right: 0,
            top: 0,
            bottom: 0
        },
        markers: [
            {"title": "Marker 1", "lng": -109.912, "lat": 22.885}
        ],
        // NOTE: the options below are broken...
        hideAttribution: true, // default false
        hideLogo: true, // default false
        hideCompass: false, // default false
        disableRotation: false, // default false
        disableScroll: false, // default false
        disableZoom: false, // default false
        disablePitch: false // default false
    });

map.on('load', function (e) {
    map.addMarkers(
        [
            {"title": "Marker 2", "lng": -109.910, "lat": 22.886},
            {"title": "Marker 3", "lng": -109.913, "lat": 22.883}
        ],
        function () { console.log("Markers added!"); },
        function (e) { console.error("Error adding markers:", e); }
    );

    map.addMarkerCallback(printMarker);

    function printMarker(selectedMarker) {
        alert("Marker selected: " + JSON.stringify(selectedMarker));
        map.addMarkerCallback(printMarker);
    }
});

Note this is also only implemented in Android at this point.

@tnightingale tnightingale force-pushed the mapboxgl-android-4.x.x branch from 09f1e44 to ff8b3f4 Compare March 26, 2016 21:54
@dagatsoin
Copy link

Thx @tnightingale, I like the way you have arranged the Mapbox.js.
I will get a look on the iOS side but I prefer to start when your pull will be checked and merged.
@EddyVerbruggen what do you think to first merge this?

@tnightingale
Copy link
Contributor Author

The difficulty with merging this is that it is a significant breaking change to the javascript API so it can't really co-exist with the iOS version in its current state. For this to be merged I would suggest a new release would need to be made that bumps the major version and would ideally have a functioning iOS version too.

@EddyVerbruggen
Copy link
Contributor

What @tnightingale says is what I'm thinking too. It would be too much of a hassle for users to have two different API's. Perhaps we can merge the PR in a new branch and add iOS support there, or @dagatsoin could send a PR to @tnightingale's fork which in turn updates this PR?

Loving that you're trying to work together on this btw ❤️

@tnightingale
Copy link
Contributor Author

Sounds good to me @EddyVerbruggen :)

@dagatsoin
Copy link

Ok for me too. May I suggest that we roughly list the features we need first to see if the resulting plugin architecture and API match both our needs ?

My short term needs:

  • be able to put an overlay on top of the map for all the complex stuff (search box, 2D markers clustering, 3D markers sticking map tilt)

Not so short term:

  • work offline

So my 2 mains features are:

  • offline map
  • advanced hits tests (as Google Maps plugin). Need to be able to drag the map even with the cordova view is on top on it and keep the overlay DOM elements touchable.

I think the feature that has the main impact on the architecture is the hit testing part. There is a little (and I hope understandable) ULM diagram here strongly inspired from the Google Maps cordova plugin. Note that I removed all the plugins and debug parts of Google Map plugin.

Google Map plugin seems to be a great futur proof architecture (seems to use a component architecture for features like marker, polygon, etc.).
It seems also decoupled from the JS API part so maybe we could use yours seamlessly.

For now here is my road map:

  1. refactor my fork with Google Map plugin architecture with the current JS API.
  2. refactor my fork to switch to your JS API.
  3. implement IOS offline part

What do you think of this @tnightingale?

@tnightingale
Copy link
Contributor Author

My main motivating factor here is offline support. If mapbox-gl-js supported offline (or I could get Service Worker running in Cordova - has anyone tried this??) then I would probably just use the JS library and be done with it.

Embedding the map in a way that plays nice with DOM elements (gracefully handles reflow, resizing, input events etc..) and allows for html element overlays is something I have been thinking about. The approach used by the googlemaps plugin is pretty nasty but I cannot think of a better method at this time.

Minor tangent: It seems to me that the problem of mixing Android Views with WebView DOM isn't a problem limited to just Cordova map plugins (or just Cordova for that matter). It would be great to see a generic solution. I have been planning to look into the feasibility of streaming rendered Android Views into a html5 canvas element via something like [WebView.postWebMessage()](https://developer.android.com/reference/android/webkit/WebView.html#postWebMessage%28android.webkit.WebMessage, android.net.Uri%29) (more info here). Unfortunately I suspect that there would be too much latency for it to be viable and I don't know if there is an iOS equivalent API but it might be worth a try.

So for the time being if the googlemaps plugin approach works well enough I'm for it. I will find some time to familiarize myself with its code and see what we can use for inspiration.

Your roadmap looks good, looking forward to seeing this all to come together. :)

…d to bind offline progress callbacks upon creation.
@tnightingale
Copy link
Contributor Author

Also worth noting is that the googlemaps treats the map as a singleton; it doesn't have support for multiple maps in a page. The implementation on my branch currently does. There's probably no technical reason the googlemaps plugin couldn't implement multiple maps, it just requires a little bit more state management.

@dagatsoin
Copy link

We have the offline feature in common. Good!

I agree it would be much easier if the natives APIs were as mature as the JS one.

I hope I will have the iOS overlay part functional next week. (It is the first time I dive into the (not so) Objective world) so I am very slooooow)

@dagatsoin
Copy link

@tnightingale I think I will manage to directly adopt your api by copying the Mapbox and MapManager class. Then I will refactor the Map class so it will have a ViewController, a PluginLayer, a ScollLayer and finally a MapboxView.
So it will keep the multimaps features.

I got a question here
I can't find info in the sdk but showUserLocation seems to be a bool representing the user GPS activation, is not? So why showUserLocation at false must prevent to create a map?

@tnightingale
Copy link
Contributor Author

That is just checking to determine whether it is necessary to request
location permissions (In Android 6). If showUserLocation is false then
requestPemission() is not called.

On Fri, Apr 1, 2016 at 6:53 AM Daniel Neveux [email protected]
wrote:

@tnightingale https://github.com/tnightingale I think I will manage to
directly adopt your api by copying the Mapbox and MapManager class. Then I
will refactor the Map class englobe the ViewController, the PluginLayer,
the ScollLayer and finally the MapboxView.
So it will keep the multimaps features.

I got a question here
https://github.com/tnightingale/Mapbox/blob/mapboxgl-android-4.x.x/src/android/Mapbox.java#L82-L85
I can't find info in the sdk but showUserLocation seems to be a bool
representing the user GPS activation, is not? So why showUserLocation at
false must prevent to create a map?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#22 (comment)

@bleege
Copy link

bleege commented Apr 20, 2016

We at Mapbox are so excited to see this work progress. So that everyone is on the same page, the final version of Android 4.0.0 shipped on on 30-March-2016 while iOS 3.2.0 launched on 6-April-2016. They're both fairly similar feature wise so it'd likely be best to build towards them.

@beachygreg
Copy link

Hi Guys is this branch still being worked on? is any help needed on Android?

@dagatsoin dagatsoin mentioned this pull request Apr 30, 2016
@tnightingale
Copy link
Contributor Author

Hi @beachygreg,

Currently I have other projects that need attention but I do plan to continue working on this branch when I get the time.

Offline support is mostly functional and stable. This branch also contains a complete rewrite of the Javascript API for map creation and manipulation. The goal is to mirror the MapboxGL JS API as closely as possible so it can function as a drop-in replacement / polyfill. I have been adding map methods as I need them but there is still quite a bit of work needed to get full functionality.

If you have time and are inclined it would be great to get some feedback on whether the javascript API works for your needs. If you have any questions while looking at/using this branch post them here. I am happy to discuss the motivation behind some of the design decisions.

@EddyVerbruggen
Copy link
Contributor

Hey @tnightingale are you still planning on supporting iOS for offline maps? I'm being chased a bit for offline maps support and would like to know whether or not I need to add it myself after merging this PR.

In any case: thanks a lot for your work!

@tnightingale
Copy link
Contributor Author

I've been meaning to get back to this but have been pulled off into other
things. iOS is definitely on the radar but at this point in time I have no
timeline for when I'll get to it.

If you want to / have time to make a start then by all means go ahead. I
will keep an eye on this thread and help out where/when I can.
On Mon, Aug 29, 2016 at 12:16 PM Eddy Verbruggen [email protected]
wrote:

Hey @tnightingale https://github.com/tnightingale are you still
planning on supporting iOS for offline maps? I'm being chased a bit for
offline maps support and would like to know whether or not I need to add it
myself after merging this PR.

In any case: thanks a lot for your work!


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#22 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKWo0_AuD-1xHOqgEAFBuWzNgu2bsEVks5qkzATgaJpZM4H1sDm
.

@EddyVerbruggen
Copy link
Contributor

Hey @tnightingale I'm struggling drawing a map on Android with Mapbox SDK 4.1.1 with the PR's code. Perhaps it's because the JS to create the map isn't what's expected by the updated plugin code.. but the result is I haven't been able to create a map.. Have your tried with this SDK version?

@tnightingale
Copy link
Contributor Author

@EddyVerbruggen I haven't touched this since I last worked on it... Judging from mapbox.gradle it looks like I was running Mapbox SDK 4.0.0 at the time. Have you tried it with that?

@EddyVerbruggen
Copy link
Contributor

Yeah you're right, just confirmed 4.0.0 doesn't have this problem. I wish I had time to dig into it but I'm afraid we're a bit in the same both in that respect.

@tnightingale
Copy link
Contributor Author

Ok that makes sense.

I'm hoping to come back to this very soon. Ideally we can consolidate with @dagatsoin's map layout work too. We still definitely need this feature and it is past time for us to get off of the old Mapbox API + offline hacks we currently using.

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.

5 participants