maplibre / flutter-maplibre-gl

Customizable, performant and vendor-free vector and raster maps, flutter wrapper for maplibre-native and maplibre-gl-js (fork of flutter-mapbox-gl/maps)
https://pub.dev/packages/maplibre_gl
Other
186 stars 106 forks source link

Refactor maplibre gl lib #396

Closed XanderD99 closed 3 weeks ago

XanderD99 commented 3 months ago

WIP

Let me start by saying that these changes are not necessary but seemed like they could help in the long run of the plugin.

It will look like a lot but a lot of the changes are just moving files around :)

This pull request is still in progress but I wanted to share this before going even further into it.

I already went through all the pages in the example and it all seemed to work fine without any issues. Apart from the web implementation, that one works on the first page but if you leave the page and go to another page it stops working as the mapCreated function is not called anymore and by that, the controller used in most pages is not initiated.

Packages

I split the package up into more different sub-packages than already present. Now there is 1 package that holds the basic logic of creating the widget and other packages have the platform-specific code present.

In the future, more platform-specific packages can be added if they are ever needed.

maplibre_gl

this is the main package, the one 99% of users will use without ever looking deeper.

It now automatically detects what platform it is being built for and creates the correct platform interface using plugin_platform_interface.

maplibre_gl_platform_interface

All shared code that is used throughout the sub-packages.

maplibre_gl_mobile

the mobile implementation of the maplibre_gl_platform_interface. This could be split into iOS and Android but they are so similar that I did not think it would yield any benefits. The biggest benefit would be easier updating of the underlying maplibre native packages.

Now in the mobile implementations, I have put the maplibre native version to the latest available ones. Android v11.0.0-pre4 and iOS 6.3.0. Together with those updates, I updated most of the variables to use Maplibre instead of Mapbox.

maplibre_gl_web

The web implementation is like it always has been.

The only change is that is now connected with the maplibre_gl package and that, that package handles the UI view. So no need to install maplibre_gl_web separately.

TODO

all GitHub workflows will most likely be broken so will need to be looked at.

update documentation of all packages

update pub.dev?

probably some more things...

XanderD99 commented 3 months ago

downloading offline regions also does not seem to work yet:

I/mapboxglexampl(13495): Waiting for a blocking GC ProfileSaver
D/OfflineManagerUtils(13495): maxZoom
D/OfflineManagerUtils(13495): 16.0
D/OfflineManagerUtils(13495): includeIdeographs
D/OfflineManagerUtils(13495): false
D/OfflineManagerUtils(13495): bounds
D/OfflineManagerUtils(13495): [[-33.5597, -70.49102], [-33.33282, -153.74267]]
D/OfflineManagerUtils(13495): minZoom
D/OfflineManagerUtils(13495): 10.0
D/OfflineManagerUtils(13495): mapStyleUrl
D/OfflineManagerUtils(13495): https://demotiles.maplibre.org/style.json
I/TetheringManager(13495): registerTetheringEventCallback:com.mapbox.mapboxglexample
V/Mbgl-ConnectivityReceiver(13495): connected - true
V/Mbgl-ConnectivityReceiver(13495): connected - true
I/OfflineManagerUtils(13495): Region download progress = 0.0
E/libc++abi(13495): terminating with uncaught exception of type std::__ndk1::regex_error: The expression contained an invalid range in a {} expression.
F/libc    (13495): Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 13603 (DatabaseFileSou), pid 13495 (mapboxglexample)
*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
Build fingerprint: 'google/sdk_gphone_arm64/emulator_arm64:11/RSR1.210722.003/7604015:user/release-keys'
Revision: '0'
ABI: 'arm64'
Timestamp: 2024-04-04 08:56:34+0200
pid: 13495, tid: 13603, name: DatabaseFileSou  >>> com.mapbox.mapboxglexample <<<
uid: 10169
signal 6 (SIGABRT), code -1 (SI_QUEUE), fault addr --------
Abort message: 'terminating with uncaught exception of type std::__ndk1::regex_error: The expression contained an invalid range in a {} expression.'
    x0  0000000000000000  x1  0000000000003523  x2  0000000000000006  x3  00000071f477ea60
    x4  fefefefeff2d6d6e  x5  fefefefeff2d6d6e  x6  fefefefeff2d6d6e  x7  7f7f7f7f7f7f7f7f
    x8  00000000000000f0  x9  4f4806e318b69ffa  x10 0000000000000000  x11 ffffffc0fffffbdf
    x12 0000000000000001  x13 00001396769ad648  x14 0008c30acf525d00  x15 0000000029aaaaab
    x16 000000749a51dc80  x17 000000749a4ff3f0  x18 000000717f29a000  x19 00000000000034b7
    x20 0000000000003523  x21 00000000ffffffff  x22 00000071f477eb90  x23 00000071f477ebd0
    x24 00000071f477ecb0  x25 00000071f477ebf8  x26 00000071f477f3f8  x27 00000071f477f400
    x28 00000071f477f279  x29 00000071f477eae0
    lr  000000749a4b2e20  sp  00000071f477ea40  pc  000000749a4b2e4c  pst 0000000000001000
backtrace:
      #00 pc 000000000004de4c  /apex/com.android.runtime/lib64/bionic/libc.so (abort+164) (BuildId: ac8f7173c886715a2f2fec67246e21da)
      #01 pc 00000000009b82dc  /data/app/~~cFg8yu9ylC73d6ul46kmTw==/com.mapbox.mapboxglexample-dVqeEigkny1VoYgv7WJ9Hw==/lib/arm64/libmapbox-gl.so (BuildId: 9557a41addee8d176f4fcb4b63c924f7447eb570)
Lost connection to device.
josxha commented 2 months ago

Moving the content of the maplibre_gl package to its own directory is a good idea since currently the code of the other packages would get uploaded to pub.dev too. There is a way around this using a .pubignore file but this seems to be the better way for me!

As a suggestion: Many packages that consists of multiple sub-packages use melos to orchestrate them. Maybe it's beneficial for here too? E.g. it can be used to run tests, fetch or upgrade dependencies of all packages and so on. After configured, it can shorten CI pipelines by quite a bit and help local development.

XanderD99 commented 2 months ago

@josxha I added melos into the mix as it does seem useful. I did just add in a basic analyze script. This should be extended in the future, but I don't have the knowledge of melos and releasing packages yet to integrate it fully.

XanderD99 commented 2 months ago

Just an FYI to add. I have a branch with android version v10.3.0 of maplibre SDK as well. In this PR it is already updated to v11

kuhnroyal commented 1 month ago

I like this, it is just way too big :) Maybe start with moving the main package into a subfolder?

XanderD99 commented 1 month ago

@kuhnroyal understand that it is big but, what do you mean exactly by move just the main folder? like this then:

packages/
    flutter_maplibre_gl/
    flutter_maplibre_gl_platform_interface/
    flutter_maplibre_gl_web/

flutter_maplibre_gl is then everything in lib/

kuhnroyal commented 1 month ago

Yeah, that looks like a good start. We get the CI and deployment working for this and then take the next step.

XanderD99 commented 1 month ago

Okay let's perhaps close this PR then and I'll create a new one with previously mentioned structure asap.

josxha commented 1 month ago

It's not necessarily required to use a /package subfolder (for melos). It's possible to have all packages directly in the root directory:

/flutter_maplibre_gl/
/flutter_maplibre_gl_platform_interface/
/flutter_maplibre_gl_web/

I'd recommend to have all other pull requests solved tho, since fixing merge conflicts afterwards seems like an unfeasable big task.

XanderD99 commented 1 month ago

/packages is indeed not strictly needed but it does seem to be a thing a lot of other packages do that use melos. Also makes the root feel a little les cluttered when adding more subfolders that are used for all sub packages.

Personaly I would suggest a structure like:

/.devcontainer/ 
/.github/
/docs/
/example/ (this perhaps optional, could be just in flutter_maplibre_gl)
/packages/
    /flutter_maplibre_gl/
    /flutter_maplibre_gl_platform_interface/
    /flutter_maplibre_gl_web/
...
README.md and other files
...

I do agree that it would be best to first resolve all other current open PRs.

josxha commented 3 weeks ago

Okay let's perhaps close this PR then and I'll create a new one with previously mentioned structure asap.

Hi @XanderD99, are you still interested to create this pull request? Pull requests come and go, so I don't think that we have no open PRs any time soon. @kuhnroyal and I had a quick chat and we prefer to put all packages directly in / for now.

/example/
/flutter_maplibre_gl/
/flutter_maplibre_gl_platform_interface/
/flutter_maplibre_gl_web/

This will keep the amount of merge conflicts and required changes to a minimum. I hope that's fine for you. As you have already done some work with melos in this pull request I'd be happy if we can merge it. (:

XanderD99 commented 3 weeks ago

Yeah that's fine for me, I'll try to do it asap.