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

Web implementation #223

Merged
merged 28 commits into from
Jun 3, 2020
Merged

Web implementation #223

merged 28 commits into from
Jun 3, 2020

Conversation

andrea689
Copy link
Collaborator

@andrea689 andrea689 commented Feb 12, 2020

I'm working on web implementation. This is next steps:

  • migrate to platform interface
  • add web implementation
    • initPlatform
    • buildView
    • updateMapOptions
    • animateCamera
    • moveCamera
    • updateMyLocationTrackingMode
    • matchMapLanguageWithDeviceDefault
    • setMapLanguage
    • setTelemetryEnabled
    • getTelemetryEnabled
    • addSymbol
    • updateSymbol
    • removeSymbol
    • addLine
    • updateLine
    • removeLine
    • addCircle
    • updateCircle
    • getCircleLatLng
    • removeCircle
    • queryRenderedFeatures
    • queryRenderedFeaturesInRect
    • invalidateAmbientCache
    • requestMyLocationLatLng
    • getVisibleRegion
    • setAttributionButtonMargins
    • setCameraTargetBounds
    • setCompassEnabled
    • setCompassGravity
    • setCompassViewMargins
    • setLogoViewMargins
    • setMinMaxZoomPreference
    • setMyLocationEnabled
    • setMyLocationRenderMode
    • setMyLocationTrackingMode
    • setRotateGesturesEnabled
    • setScrollGesturesEnabled
    • setStyleString
    • setTiltGesturesEnabled
    • setTrackCameraPosition
    • setZoomGesturesEnabled
    • onInfoWindowTappedPlatform
    • onSymbolTappedPlatform
    • onLineTappedPlatform
    • onCircleTappedPlatform
    • onCameraMoveStartedPlatform
    • onCameraMovePlatform
    • onCameraIdlePlatform
    • onMapStyleLoadedPlatform
    • onMapClickPlatform
    • onCameraTrackingChangedPlatform
    • onCameraTrackingDismissedPlatform
  • update Readme
    • accessToken
    • supported API

@andrea689 andrea689 mentioned this pull request Feb 12, 2020
@tobrun
Copy link
Collaborator

tobrun commented Feb 15, 2020

@andrea689 cool seeing this move forward! Refs #26

- initPlatform
- buildView
- updateMapOptions
- animateCamera
- moveCamera
- setTelemetryEnabled
- getTelemetryEnabled
- getVisibleRegion
- setCameraTargetBounds
- setMinMaxZoomPreference
- setRotateGesturesEnabled
- setScrollGesturesEnabled
- setStyleString
- setTiltGesturesEnabled
- setTrackCameraPosition
- setZoomGesturesEnabled
add instruction in README for add accessToken in web platform
@rpk98c
Copy link

rpk98c commented Feb 25, 2020

@andrea689 Great to see this being worked on. Let me know if need help

andrea689 added 12 commits March 3, 2020 11:58
handle addSymbol, updateSymbol and removeSymbol
refactor symbolManager to use abstract featureManager
add onMapStyleLoadedPlatform
add requestMyLocationLatLng
add onCameraTrackingChangedPlatform
add onCameraTrackingDismissedPlatform
add setMyLocationEnabled
add setMyLocationTrackingMode
myLocationRenderMode not available
invalidateAmbientCache not available
add setCompassGravity
setCompassViewMargins not available
setLogoViewMargins not available
setAttributionButtonMargins not available
* master:
  Support setting map's content insets (flutter-mapbox-gl#215)
  Provide `onMapIdle` callback (flutter-mapbox-gl#214)
  [example] add full page map example (flutter-mapbox-gl#201)
  Fix missing location indicator on iOS (flutter-mapbox-gl#176)
  Returned a nil result so the future completes. (flutter-mapbox-gl#216)
  Update README: Access Tokens for self-hosted tiles (flutter-mapbox-gl#217)
  Change annotation priority (flutter-mapbox-gl#222)
@andrea689 andrea689 marked this pull request as ready for review March 13, 2020 15:40
@lkuich
Copy link

lkuich commented Apr 1, 2020

Trying to get symbols and lines to work, noticed onMapCreated gets called before onStyleLoadedCallback. So the call to lineManager is null in mapbox_map_controller.dart:addLine. I fixed this by adding onStyleLoadedCallback in my MapboxMap implementation and adding my lines there instead of in onMapCreated.

Works fine in onMapCreated on Android.

@lkuich
Copy link

lkuich commented Apr 2, 2020

Symbols and lines are not working in release builds or profile builds. Building with flutter run --profile -d chrome / flutter run --release -d chrome. Works fine in debug.

profile:
image

release:
image

Flutter 1.15.17 • channel beta • https://github.com/flutter/flutter.git
Framework • revision 2294d75bfa (4 weeks ago) • 2020-03-07 00:28:38 +0900
Engine • revision 5aff311948
Tools • Dart 2.8.0 (build 2.8.0-dev.12.0 9983424a3c)

fix setData in release
@andrea689
Copy link
Collaborator Author

@lkuich the right place to call these methods is just onStyleLoadedCallback

@lkuich @rpk98c I fixed the problem to add symbols in release mode

* master:
  Updated location package and code to cope with a breaking change. (flutter-mapbox-gl#239)
@@ -1,5 +1,5 @@
# Generated by pub
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can gitignore this file, as it's generated

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file contains all packages version used.. if we don't commit this file, and a new incompatible version of a package is released, a new user may have problems

@@ -0,0 +1,7 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can probably also gitignore these AndroidManifest.xml files in /debug and /profile as I think they are generated, but it's probably also not an issue to commit them

Copy link
Collaborator

@TimothySealy TimothySealy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation still has the Page component that conflict with a new Page component in the Flutter SDK.

I think the strong typed interface is a great improvement. Great job!

Comment on lines +42 to +45
'icon-allow-overlap': true,
'icon-ignore-placement': true,
'text-allow-overlap': true,
'text-ignore-placement': true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a todo here for the tail work in making these options configurable. Also see #206 .

Comment on lines +64 to +80
Future<void> _addStylesheetToShadowRoot() async {
int index = -1;
while (index == -1) {
index = document.getElementsByTagName('flt-platform-view').length - 1;
await Future.delayed(Duration(milliseconds: 10));
}
HtmlElement e = document.getElementsByTagName('flt-platform-view')[index]
as HtmlElement;

LinkElement link = LinkElement();
link.href =
'https://api.tiles.mapbox.com/mapbox-gl-js/v1.6.1/mapbox-gl.css';
link.rel = 'stylesheet';
e.shadowRoot.append(link);

await link.onLoad.first;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the CSS added to the shadow root of this component? What is the reason why is it not added to the HEAD? As this is attached to a specific version of Mapbox GL JS this could lead to inconsistencies in the UI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because the shadow DOM not see the css added to head section.. I found this workaround but I am opened to new solution!

@override
Future<CameraPosition> updateMapOptions(
Map<String, dynamic> optionsUpdate) async {
// FIX: why is called indefinitely? (map_ui page)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the updates are triggered due to mapChanged events. Probably due to a combination of tracking options passed to the MapboxMap.

@m0nac0
Copy link
Collaborator

m0nac0 commented May 22, 2020

@andrea689 I'm currently getting errors in the examples because of the changes we made in #258, could you rebase to solve those?
But in general: really great job, thank you for the hard work 👍

@fotiDim
Copy link

fotiDim commented May 27, 2020

Not sure if it is indeed an issue but pointing my pubspec to the repo & brach of @andrea689 like so:

  mapbox_gl:
    git: 
      url: git://github.com/andrea689/flutter-mapbox-gl.git
      ref: av-web

gives me this error:

Error on line 12, column 11: Invalid description in the "mapbox_gl" pubspec on the "mapbox_gl_platform_interface" dependency: "./mapbox_gl_platform_interface" is a relative path, but this isn't a local pubspec.

I fixed it in a fork like this:

  mapbox_gl_platform_interface:
    git:
      url: https://github.com/fotiDim/flutter-mapbox-gl
      path: mapbox_gl_platform_interface
  mapbox_gl_web:
    git:
      url: https://github.com/fotiDim/flutter-mapbox-gl
      path: mapbox_gl_web

When this PR is merged it would be nice to have a 0.6 release of this package.

@m0nac0 m0nac0 mentioned this pull request May 28, 2020
@m0nac0
Copy link
Collaborator

m0nac0 commented May 28, 2020

@andrea689 I just did a thorough testing and everything is looking really fine already.
Just three minor things:

  • On the Place Symbol/Circle/Line pages and the scrolling map page in the example app I'm getting lot's of Expected value to be of type ..., but found null instead warnings and a Could not parse color from value 'null' warning
  • Adding a custom image throws a warning, I think the styleimagemissing listener here:
    https://github.com/tobrun/flutter-mapbox-gl/blob/c7e364e2bffdeef063f5a0c8708541b02ad6cc2c/mapbox_gl_web/lib/src/feature_manager/symbol_manager.dart#L61
    probably always logs that warning. It would be nice if there was a way to prevent the warning, but it's not a big issue if there isn't.
  • On the scrolling map page I'm getting an Uncaught Error: There is already a source with this ID and both maps behave identically, scrolling and zooming at the same time. Is this feature even supported on web?

Copy link
Collaborator

@tobrun tobrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome work @andrea689! Could you rebase on head of master and resolve conflicts. Really excited getting support for web on this project 😄

* master:
  [flutter] release v0.6.0
  Change gradle version to 3.5.0 instead of 3.6.1
  Update Mapbox plugins and gradle
  iOS: add long press handler (flutter-mapbox-gl#268)
  [android] Implemented long click for Android (flutter-mapbox-gl#198)
  [fix] Change default MyLocationTrackingMode to None (flutter-mapbox-gl#285)
  Add First-Class Support For Listening to `onCameraIdle` events (flutter-mapbox-gl#280)
  CI: use stable flutter channel also on iOS (flutter-mapbox-gl#270)
  Implement addImage() to style (flutter-mapbox-gl#269)
  Add animation duration for animateCamera (flutter-mapbox-gl#259)
  Document onStyleLoadedCallback (flutter-mapbox-gl#257)
  example: rename Page to ExamplePage (flutter-mapbox-gl#258)
@tobrun
Copy link
Collaborator

tobrun commented Jun 3, 2020

@andrea689:

The bots are reporting, 1 small thing, we are close:

Lint analysis16s
##[error]Process completed with exit code 1.
Run flutter analyze
Analyzing flutter-mapbox-gl...                                  

   info • This function has a return type of 'Future<LatLngBounds>', but doesn't end with a return statement • mapbox_gl_web/lib/src/mapbox_map_controller.dart:262:24 • missing_return

1 issue found. (ran in 14.5s)
##[error]Process completed with exit code 1.

README.md Outdated Show resolved Hide resolved
@andrea689
Copy link
Collaborator Author

I added sdf on iOS addImage

onStyleLoadedCallback not called on iOS, I will investigate on it.

The scrolling map page not working because with actual implementation only one map for page is working.. I will refactor the code to work with more the one map for page

@tobrun
Copy link
Collaborator

tobrun commented Jun 3, 2020

I added sdf on iOS addImage
onStyleLoadedCallback not called on iOS, I will investigate on it.
The scrolling map page not working because with actual implementation only one map for page is working.. I will refactor the code to work with more the one map for page

@andrea689 That is fine for first version, let's merge and keep iterating. Can you ticket out all the remaining work?

edit: I can only squash merge this PR, rebase merging won't be possible.

@tobrun tobrun dismissed TimothySealy’s stale review June 3, 2020 20:13

dismissing review to merge the PR, please ticket out any tailwork you want to see adressed!

@tobrun tobrun merged commit ebef5db into flutter-mapbox-gl:master Jun 3, 2020
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.

8 participants