[WIP] [Next version] #29

Open
wants to merge 45 commits into
from

Projects

None yet

10 participants

@dagatsoin
Collaborator
dagatsoin commented Apr 11, 2016 edited

ANDROID ONLY FOR THE MOMENT

I have refactored the code (inspired by Google Maps cordova plugin) to be able to display the map behind the webview. So:

  • dom element can be displayed on top of it
  • map scrolls with the webview

Futur development.

  • track overlay elements.
  • to be able to click on DOM overlay elements.

The Mapbox SDK is now iOS 3.2.0 and also fixes a memory leak which add 30-50mo each time the user show/hide the map.
Note the memory usage is now 130mo. But considering that the old SDK leak led to more than 130mo after 5 show/hide, it is not a big deal.

@dagatsoin dagatsoin changed the title from [WIP]Feature/place view behind to [WIP] display map view behind webview Apr 11, 2016
@dagatsoin
Collaborator

@EddyVerbruggen I think I am done for the iOS part of this feature. If you want to test it I modified the index.html with new buttons.

New:

/**
 * Toggle touch events on the map. For exemple if a modal is over the map when opening
 * you should use this to disable touch event on the map.
 * @param options:{clickable:boolean}
*/
  setClickable(options, successCallback, errorCallback, id)

/**
 * Call this function when the map is resized or when a DOM element is added inside the map div.
 * It will recompute the clickable layout.
 * @param options:{divId:string}
*/
  refreshMap(options, successCallback, errorCallback, id)

Update:

  • a map could be initialise by passing a div id. All children elements of this div will be clickable without transmit touch event on the map.

show({div:String}, successCallback, errorCallback, id)

@dagatsoin
Collaborator

Any help on converting this on Java will be appreciated :) (cc @beachygreg)
Next step will be implements offline maps or fuse #22 at some point.

@beachygreg

Happy to help out where I can but how does this merge request relate to #22 ?

@dagatsoin
Collaborator
dagatsoin commented May 1, 2016 edited

@beachygreg
Both PR have same final goals:

  • last MB native api
  • offline maps
  • multimaps

On top of this @tnightingale works on more advanced JS api to stick to his needs and MapboxJS. On my part I want to make a smooth transition from the current cordova plugin API without introduce breaking changes.

I also got inspired from the google map cordova plugin to be able to put overlay dom elements like search bars, navigation, etc... but also advanced markers (DOM element) which is not possible with MapboxGL.

I still have to:

  • expose the api part for the Offline Map features
  • making overlay elements work with mutlimaps

After that, if it is possible I will stick to @tnightingale JS api.

@beachygreg

@dagatsoin cool, I am really after the offline maps. I am a java developer and have done some android programming before so should be able to help out. Only thing is I don't have that much time but sure I can help. Will checkout your branch and take a look.

@dagatsoin
Collaborator
dagatsoin commented May 1, 2016 edited

@beachygreg If you can't wait for offline map, maybe you should first give a try to @tnightingale PR. But if you want to dive in code, you are welcome :)
I have not commented my code. Here is how it works:

  • CDVMapBox.h: retrieve the CDV command, use MapManager to give the right map (in case of multipmap)
  • MapsManager.h: a singleton which stores the maps (in case of multimaps). It does not content logic, juste set, get, remove a specific map in a collection.
  • Map.h: the concrete Map object. Receive the CDV command and communicate to the controller.
  • MapController.h: it is the mapbox controller, expose the Mapbox API and handle the GLView. It is the classical file use in MapboxGL app.
  • PluginLayer.h: "dig a hole" in the web view and place the map behind so that the touch event in the map zone passes through the webview. The cool part, is that you can add DOM elements on top of the map (which is not possible with the actual version of cordova mapbox plugin) enabling advanced markers as vidรฉo, game assets, gl canvas, etc. The main role of this file is to intercept the touch event. If event hits a DOM overlay element (Marker, searchBar, ...) it will be send to the webview (scroll, tap, etc.). If not, it transmit the event to the MapboxGL view (drag, pan, rotate, ect...)
  • Scrollview.h: attach the MapboxGL view to the webview so it follows scroll and transition.

If you want more detail, fill free to ask.

@dagatsoin dagatsoin referenced this pull request in mapbox/mapbox-gl-native Jul 7, 2016
Closed

Crashes at map init with empty CameraPosition option. #5607

@EddyVerbruggen
Member

Hey @dagatsoin I'd like to merge this PR and/or #22 soon since I'd really like to bump the Mapbox SDK's.

Which of these do you feel should take precedence? If it's this one can you please let me know what would be a good time to test and merge it? And also please rebase your fork (just pull in the latest changes from this repo) so it can be automatically merged.

Cheers,
Eddy

@dagatsoin
Collaborator
dagatsoin commented Jul 7, 2016 edited

Hello @EddyVerbruggen

This branch is still in work in progress. Here are the things to consider for merging decision:

  • There will breaking changes concerning the JS part. I will stick to the current MapboxGL JS api as I discussed with @tnightingale.
  • I focus multimaps as @tnightingale, it won't be functional for my final release, but this will ask little extra effort to finish to implement it. @tnightingale multimaps is functionnal.
  • contribution doc does not exist but will be necessary because the layout (heavily inspired by Google CDV plugin) is way more complexe than the master branch.
  • The overlay works now on Android but all case have not been tested yet (such as X scrolling or complexe exemple like tab/slide...)
  • My last change broke iOS version, I will need to rewrite it partially (2 days max)
  • Code is more complexe and maybe need some coverage. (0% today)
  • Offline region are not implemented yet. @tnightingale offline function is functionnal.
  • And finally my code have almost nothing in common with the master branch now (except the demo html). So I don't know if a rebase is possible.(any git expert here?)
  • JS part of @tnightingale is better than mine. I don't have a js map object.
  • I am on 4.1.beta-3 on Android and will update to the last iOS version next week.

I plan at least a week of work. I am 100% on it right now.

@dagatsoin
Collaborator

Ho, and I have not test the install yet. I am working directly on a generated cordova project.

@anothar
Contributor
anothar commented Jul 8, 2016

Hello, @dagatsoin. I've implemented custom marker image support and created pull request. But now I wonder whether your changes implement this feature?

@dagatsoin
Collaborator
dagatsoin commented Jul 8, 2016 edited

hi @anothar

You mean this feature?
If yes, I have not implemented it because I aim to use the DOM overlay layer instead.

Can I get a look to your code?

@anothar
Contributor
anothar commented Jul 8, 2016

Yes, exactly this feature. Here is code in pull request. I've tested it for both android and iOS (for iOS only in emulator).

I aim to use the DOM overlay layer instead.

It looks like more general solution.

@dagatsoin
Collaborator
dagatsoin commented Jul 8, 2016 edited

Ok I see, it seems straight forward to redo the modifs in my branch (I am too distant from the master branch). On the other hand, it will be at the bottom of my todo list, but I will do it.

@anothar
Contributor
anothar commented Jul 8, 2016

Ok!

@dagatsoin
Collaborator

@anothar I am a noob with Java and Android. Where I suppose to put the https://github.com/anothar/Mapbox/blob/features_image_marker/src/android/libs/androidsvg-1.2.1.jar in my development environment ? (it is a cordova generated project from the demo folder.)
Thx :)

@anothar
Contributor
anothar commented Jul 8, 2016

@dagatsoin you should put it in libs folder: platforms->android->libs
gradle will automatically include it during build.

@dagatsoin
Collaborator
dagatsoin commented Jul 8, 2016 edited

Indeed it was quick. I integrated you code to my android branch. I am not use with notice attribution. What do I need to write and where to indicate you made the code ?

@anothar
Contributor
anothar commented Jul 9, 2016 edited

Indeed it was quick.

I'm glad it did't take long time to integrate.

What do I need to write and where to indicate you made the code ?

Just add javadoc comment on the method "createIcon" like below

/**
*Creates image for marker
*@author anothar
 */

Thx :)

// Thanks @anothar !!

))

@petercn
petercn commented Jul 10, 2016

@anothar @dagatsoin Do either of you happen to have a particular stable commit that you can point me too on either of your forks/branches? I've tried @dagatsoin feature branch at various commits but then it fails either when I first "cordova plugin add" or when I try to build iOS after a successful plugin add. It's usually complaining about a missing PluginLayout or PluginLayer file. I'm using the latest Ionic 2 framework with the latest cordova and just trying to get to the point that I can spawn either a blank Ionic 2 and/or blank cordova app with the mapbox demo code and be able to finally see html elements overlay on top of the native map, this is my main goal. Any help would be much appreciated and your work seems to be the closest to achieving this. @EddyVerbruggen When ready, I would love to see this feature finally completed and merged into the official Telerik plugin for a new version release :-) This is a total game changer when it happens

@anothar
Contributor
anothar commented Jul 11, 2016

Hi, @petercn . You can take from my fork (branch features_image_marker) latest commit - there won't be any changes and I tested it. But there is only support for custom image for marker.

and your work seems to be the closest to achieving this.

Actually it's work of @dagatsoin - he makes overlay layer.

@petercn
petercn commented Jul 11, 2016

@anothar Major thanks for the quick reply and that bit of good news, I'll give it a try today to see if the overlay marker works out for me.

@petercn
petercn commented Jul 11, 2016

@dagatsoin I made some progress with @anothar's image markers but what I'm really after are the complete overlays in your codebase and I just have no luck getting a basic build compiled. I tried both a base ionic2 (ionic start) setup and a base cordova setup (cordova create), then I added the plugin at specific commits, and then tried to compile for either OS. But I just keep getting all kinds of errors. Can you provide me with the exact CLI commands that get say the demo project working for you?

@dagatsoin
Collaborator

Hello @petercn
this is still under heavy WIP and I have not tested from the cli command yet. But my guess is that the config.xml does not reflect the last file name changes.
I am on a last bug about scrolling and I will clean the config. But even after that, it will be an alpha version at best, so with potential bugs and breaking changes coming.

@petercn
petercn commented Jul 12, 2016

Thanks so much for the prompt replies and updates and yes, some differences in file names, dependencies, and such have been among the problems I've had with either installing the plugin or doing a build after successful plugin install. At various commits for example, the plugin.xml file had file names specified in it that were no longer there.

@dagatsoin
Collaborator
dagatsoin commented Jul 12, 2016 edited

Give a try with the last one.

@petercn
petercn commented Jul 12, 2016

All of this work when done and officially released in a new telerik mapbox plugin point release, will be extremely beneficial to the hybrid app dev community at large. Before this, only google maps plugin could achieve something similar but then restricted you to just google maps and all of its limits. With webview overlays on top of native maps we'll have the most performant maps for hybrid apps yet still be able to add all sorts of custom controls, markers, UI, etc. on top.

@dagatsoin
Collaborator

Thx you @petercn
I hope this will be useful for a lot of people :)

@dagatsoin
Collaborator

@petercn
@anothar
@EddyVerbruggen
@tnightingale

Hi guys, I think I have a testable Android release, but I also have a f****g java problem to install it on a fresh meteor cordova project (did not test on cordova alone yet, but it should be the same). Any help is welcome ;)

@dagatsoin
Collaborator
dagatsoin commented Jul 13, 2016 edited

http://stackoverflow.com/questions/29386152/override-java-version-when-building-a-cordova-project-with-gradle solved the problem.

I have now a new problem with R in the icon creation. The plugin wants to use import io.cordova.hellocordova.R; which is obviously unknown by other project...

@anothar
Contributor
anothar commented Jul 13, 2016

Hi guys, I think I have a testable Android release

Awesome!

have now a new problem with R in the icon creation. The plugin want to use import io.cordova.hellocordova.R;

Really strange. I will try to help and test java code in the evening.

@dagatsoin
Collaborator

sorry @anothar it is not your code, it is mine. I need to get access to the default icon marker inside the plugin. I have never use R. As I understand it gets data from an XML? If so maybe I can include an xml in my plugin. I will give it a try.

@anothar
Contributor
anothar commented Jul 13, 2016 edited

@dagatsoin

import io.cordova.hellocordova.R

It's just wrong namespace. If you want to take resource from your library - you just should write R.<name_of_resource> without importing.
If from other library- then you can try to import it's namespace. For Mapbox:
import com.mapbox.mapboxsdk.R
But I'm not sure whether it would work. You can also embed resources in library. Here is documentation and how to use them inside your code.

You want to get default Mapbox marker image and reimplement it in overlay?

@dagatsoin
Collaborator

I want to make an invisible marker. The only way I found is too create a default marker and set alpha to 0.
The only way I found is too use this

Drawable iconDrawable = ContextCompat.getDrawable(_activity, R.drawable.default_marker);
iconDrawable.setAlpha(0);
@anothar
Contributor
anothar commented Jul 13, 2016

@dagatsoin Please, try this code:
Drawable iconDrawable = new ColorDrawable(Color.TRANSPARENT);

@dagatsoin
Collaborator
dagatsoin commented Jul 13, 2016 edited

yep that works, thanks you. The Cordova build also passes. I think it is good !
I will clean a bit some dead code and set up the header file for credits and it will be done for now for android.
And also make a bit of doc.
On the other hand I don't think that I will touch iOS before Septembre.

@dagatsoin
Collaborator
dagatsoin commented Jul 13, 2016 edited

@EddyVerbruggen did you read my last message to you? Is this mergeable (I mean stick what do you expect for this plugin in the futur) or should I maintain it in my separate fork ?

@anothar
Contributor
anothar commented Jul 13, 2016

I'm for separate branch (not fork) - it will be easier for me to contribute.

@anothar anothar referenced this pull request Jul 15, 2016
Open

Adding Custom Markers #43

@peterhriser

I need to have a way to show the search bar that is in my footer over the map when the keyboard pops up (thus moving the footer bar upwards to remain above the keyboard that was opened. AS of now, the footer moves upwards, and then gets hidden behind the map. which is in the center of the screen

@dagatsoin
Collaborator
@peterhriser
peterhriser commented Jul 18, 2016 edited

@dagatsoin
screenshot_2016-07-18-14-37-32
https://github.com/peterhriser/Mobile-Application-Ion

This is what it looks like without the map (which would be in the grey area) Here is the link to the front end of the project. In the templates, in tab-location, when you click the button on the bottom, it brings up search bar and keyboard but the search bar gets blocked because it is an html element that I moved to the bottom of the screen with CSS.

@davidxll

I can't load the native map on the demo in Android =(
it suppose to work, it looks fine, no error... seems like it just jumps all Mapbox sdk methods.
I tried to create the simplest map, with 'streets' style, and nothing.

@dagatsoin
Collaborator

@peterhriser could you past me the compiled html code?

@davidxll you are talking about the demo of this pull request right ? I will retry to install the demo from scratch to see if I have the same issue.

@petercn
petercn commented Jul 20, 2016

@dagatsoin If you're able to successful build/run the demo, see a native view based map below webview based UI on top, and still interact with the map below in transparent areas then can you please detail the minimum steps to get there? That's been the main goal I'm trying to achieve for both iOS and Android but I'll take either one at this point :-/

A few months ago I was able to achieve a native map behind webview on top but was never able to get the code to place when transparent webview areas up top could allow touch/gestures events to act on the native map area below.

@dagatsoin
Collaborator
dagatsoin commented Jul 21, 2016 edited

I detected service/manifest/auth stuff issue when I installing from the repo. I am on it.
I wonder if it related to mapbox/mapbox-gl-native#4713 I hope no.

@dagatsoin dagatsoin referenced this pull request in mapbox/mapbox-gl-native Jul 21, 2016
Closed

Unable to register for GPS updates #4713

@dagatsoin
Collaborator

I definitely may have some issue with manifest & co. I won't have the time until next week to dive into that. Sorry guys.

@petercn
petercn commented Jul 21, 2016

@dagatsoin No worries, I appreciate all the great work that's being done towards this UI overlay effort, the custom SVG Markers, and offline maps capability. If all 3 of those eventually merge back into an official Telerik Mapbox plugin release, it'll be epic :-) I'll contribute in any way that I can, whether it's the code itself, testing, feedback, etc.

@dagatsoin
Collaborator
dagatsoin commented Aug 2, 2016 edited

@petercn the bug was not a bug but an outdated css in the demo folder... I spent my day to find that :(

But now the demo works as excepted!

@petercn
petercn commented Aug 2, 2016

Nice, I'll try taking it for another spin then :-) I think I recall what I did last time to try to get the demo running but just in case for me or for anyone new who's trying to run the demo of your branch... can you just detail the CLI commands / steps that you take to get the demo of your branch up if you're starting from nothing? Thanks

@EddyVerbruggen
Member
EddyVerbruggen commented Aug 25, 2016 edited

Hey @dagatsoin I'm taking a look at your work again tomorrow! If it's sufficiently stable I will certainly consider merging it and tagging a new major release.

Just to confirm, you didn't implement offline maps for iOS but you did for Android, right?

@dagatsoin
Collaborator
dagatsoin commented Aug 26, 2016 edited

Hello @EddyVerbruggen, yes I just worked on the Android side. I plan to refactor iOS in September.
Indeed it will be a major release at end, for now I can consider it as an alpha.
I don't have a precise checklist but here are some major milestones:

RC Release

  • Clean the code
  • Write doc

V.1

  • As the api surface is growing I think I need to write test, both on native and JS part.
  • Write iOS part

Next

  • Optimize the view switching (I test on a samsung galaxy s2, and there is a hang of 2-3 s)
  • Finish working on multi maps
@EddyVerbruggen
Member

Hey @dagatsoin thanks for the update. I've got folks breathing down my neck for especially offline map support. Do you think it's wise to merge yours for this (and have me add iOS support for offline maps), or would you recommend merging the other PR which adds offline support for both platforms? That would mean though that you will have a bunch of merge conflicts..

@dagatsoin
Collaborator
dagatsoin commented Aug 26, 2016 edited
  • The two PR have quite different implementations.
  • The main difference is mine proposes an HTML overlay.
  • The good part is both PR want to use the Mapbox.js api.

So if time is running up, I suggest to you to merge the other PR. You will have both multimaps and offline working.
And if people want to have the HTML map overlay we could switch to mine in the futur if you want. As I will stick as close as possible to the Mapbox.js api, the switch will be seamless.

@EddyVerbruggen
Member

@dagatsoin Thanks for that idea. I'd love to merge this later on as well because you're working on a killer feature imo. I'll be looking at PR 22 now. ๐Ÿ‘

@EddyVerbruggen
Member

@dagatsoin I just learned PR22 is also Android only (misread that). So still not sure what to do as I'm also not yet able to show a map yet on Android (crashing during map creation). So the jury's still out on this one.

@dagatsoin
Collaborator

crash on my PR?

@EddyVerbruggen
Member

No, sorry for being a bit vague - on PR 22 which I wanted to merge. Just wanted to let you know I'm still in doubt how to proceed as I also need iOS support for offline maps which PR 22 doesn't (yet) offer either.

@anothar
Contributor
anothar commented Oct 15, 2016 edited

Hi, @dagatsoin . You have added new Mapbox SDK as framework. I remember that I had some problems with frameworks in Cordova plugins (meteor/meteor#6813). Have you tested iOS version? Does it work?
I want to update iOS SDK version in my fork. And I don' t know whether to compile it into *.a file or add as framework as you did.

@AndersBillLinden

Will the possibility to overlay the webview upon the map coming soon? It is a true blocker for us that the webview is covered!

@dagatsoin
Collaborator

Sorry, I am quite busy on another part of my app and I did not get the time to update this.
I will rework on the map in November to finish Android. But I think you can give a try with this release now. I can do some stuff with it.
However, the iOS version is not plan until next year...

@AndersBillLinden

It is of course possible to have the map covering the webview as it is currently doing, setting top and bottom margins to 30. That will make it possible to have a header and footer and the webview in the middle.

What I will miss then is the possibility to put a dialog box over the mapview.

Now, is there some plugin that gives me an extra webview that I can temporarily display when I want a dialog box? Preferrably one that has a transparent background? That webview should be added on top of everything else and be removed when the "dialog" is closed?

@AndersBillLinden

The plugin cordova-second-webview on https://github.com/kunder-lab/cl.kunder.webview was very promising. It is an opaque webview at the moment.

@dagatsoin
Collaborator

I don't know any easy solution for that (that's why I do this PR). But if you are hurry and you don't mind using GoogleMaps, their Cordova plugin do a webview overlay from the box. And it works quite well.

@dagatsoin
Collaborator

Hi, @anothar could you get a look to this question please?

@AndersBillLinden

@dagatsoin Does the google maps cordova plugin perform better than mapboxgl-js?

@dagatsoin
Collaborator

No idea at all, I did not compare.

@anothar
Contributor
anothar commented Nov 4, 2016 edited

Hi, @dagatsoin . Unfortunately I don't know the answer. I even don't fully understand the question. If it is regarding this plugin maybe you will tell what images don't replace the old ones? I just need code example...

@jmordica

When trying to build on iOS with your fork and feature branch, i'm getting linker command failed exit code 1 referring to mapbox

@dagatsoin
Collaborator

@jmordica iOS part is not functional I focus on Android only for now.

@dagatsoin dagatsoin changed the title from [WIP] display map view behind webview to [WIP] [Next version] Dec 30, 2016
@cusspvz cusspvz added this to the 2.0.0 milestone Dec 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment