WIP: Feat/maplibre migration #849

MAudelGisaia · 2024-08-04T00:28:53Z

Hello,
before switching to maplibre I tried a refacto which includes:

externalization of map logic in a ts ArlasMapgl class
behavior abstraction in AbstractArlasMapGl.

The aim is to:

no longer coupling the core library with a particular framwork,
lighten the mapgl component.
reduce the coupling of other parts of the application with a particular map provider.

I don't know whether what I've started is correct in your eyes. That's why this PR is in wip and open to changes/improvements.

If you think it's going in the wrong direction, it's okay to review the whole thing. This refacto should be seen as a working basis.

Some parts still need to be tested to ensure that everything works as planned.

projects/arlas-components/src/lib/components/mapgl/mapgl.component.ts

projects/arlas-components/src/lib/components/mapgl/model/AbstractArlasMapGL.ts

QuCMGisaia · 2024-08-05T07:57:53Z

projects/arlas-components/src/lib/components/mapgl/model/AbstractArlasMapGL.ts

+  public abstract getLayers(): void;
+  public abstract enableDragPan(): void;
+  public abstract getCenter(): void;
+  public abstract flyTo(center, zoom: number): void;


missing type for center

QuCMGisaia · 2024-08-05T08:03:32Z

projects/arlas-components/src/lib/components/mapgl/model/ArlasMapGL.ts

+ *  - wrap of provider methode could be implemented in another class in middle of
+ *  abstract class.
+ */
+export class ArlasMapGl extends AbstractArlasMapGL {


The capitalisation of GL should be the same for both (Gl vs GL). Since this map is typed according to mapbox, I think it should be specified in its name to avoid any confusion

QuCMGisaia · 2024-08-05T08:05:05Z

projects/arlas-components/src/lib/components/mapgl/model/ArlasMapGL.ts

+    // Disable map pitch and rotation with keyboard
+    this._mapProvider.keyboard.disableRotation();
+
+    // disable box zoom;


disable -> Disable

QuCMGisaia · 2024-08-05T08:13:06Z

I don't know if it is a work in progress or if it is the solution you settled on, but I feel like ArlasMapGl is currently typed for mapbox, and can't be used generically. Other than that, it seems very promising !!

QuCMGisaia · 2024-09-11T08:07:33Z

projects/arlas-components/src/lib/components/mapgl/mapgl.component.ts

 import centroid from '@turf/centroid';
 import limitVertexDirectSelectMode from './model/LimitVertexDirectSelectMode';
 import validGeomDrawPolygonMode from './model/ValidGeomDrawPolygonMode';
 import * as mapboxgl from 'mapbox-gl';
+import { AnyLayer, TransformRequestFunction } from 'mapbox-gl';


You still have some mapboxgl typing here

QuCMGisaia · 2024-09-11T08:11:06Z

projects/arlas-components/src/lib/components/mapgl/mapgl.component.ts

@@ -584,75 +505,22 @@ export class MapglComponent implements OnInit, AfterViewInit, OnChanges, OnDestr

  /** puts the visualisation set list in the new order after dropping */
  public drop(event: CdkDragDrop<string[]>) {
-    moveItemInArray(this.visualisationSetsConfig, event.previousIndex, event.currentIndex);


Since we keep the visualisationSetsConfig, the moveItemInArray can be kept and then you use this.map.reorderLayers to avoid defining another method (drop)

QuCMGisaia · 2024-09-11T08:11:22Z

projects/arlas-components/src/lib/components/mapgl/mapgl.component.ts

@@ -584,75 +505,22 @@ export class MapglComponent implements OnInit, AfterViewInit, OnChanges, OnDestr

  /** puts the visualisation set list in the new order after dropping */
  public drop(event: CdkDragDrop<string[]>) {
-    moveItemInArray(this.visualisationSetsConfig, event.previousIndex, event.currentIndex);
-    this.reorderLayers();
+    this.map.drop(event);
  }

  /** puts the layers list in the new order after dropping */
  public dropLayer(event: CdkDragDrop<string[]>, visuName: string) {


same as above

QuCMGisaia · 2024-09-11T08:14:21Z

projects/arlas-components/src/lib/components/mapgl/mapgl.component.ts

-    moveItemInArray(layers, event.previousIndex, event.currentIndex);
-    this.visualisationSetsConfig.find(v => v.name === visuName).layers = layers;
-    this.reorderLayers();
+    this.map.dropLayer(event, visuName);
  }

  /** Sets the layers order according to the order of `visualisationSetsConfig` list*/
  public reorderLayers() {


I think the method should be kept as it was, and only the map level interactions should be moved to the ArlasMapGl, like getLayer, moveLayer. otherwise we would end up coding the same ArlasMap logic in multiple files, instead of just coding the elementary bricks that allow for more complex actions, like reordering the layers

QuCMGisaia · 2024-09-11T08:15:39Z

projects/arlas-components/src/lib/components/mapgl/mapgl.component.ts

@@ -734,270 +602,230 @@ export class MapglComponent implements OnInit, AfterViewInit, OnChanges, OnDestr
    }
  }

+  public defaultOnZoom(e) {
+    if (e.features[0].properties.cluster_id !== undefined) {
+      // TODO: should check the this.index is set with good value


Agreed, when i looked into refactoring the component, it was not clear where index was set and what it does

QuCMGisaia · 2024-09-11T09:05:59Z

projects/arlas-components/src/lib/components/mapgl/model/ArlasMapGL.ts

+    }
+  }
+
+  public addLayerInWritePlaceIfNotExist(layerId: string): void {


Write -> Right

Should be addLayerAboveBaseLayers or something like that

QuCMGisaia · 2024-09-11T09:06:58Z

projects/arlas-components/src/lib/components/mapgl/model/ArlasMapGL.ts

+
+  public getMapExtend(): MapExtend {
+    const bounds = this.getMapProvider().getBounds();
+    return  { bounds: bounds.toArray(), center: bounds.getCenter().toArray(), zoom: this.getMapProvider().getZoom() };


extra space

QuCMGisaia · 2024-09-11T09:07:22Z

projects/arlas-components/src/lib/components/mapgl/model/ArlasMapGL.ts

+                      event: string; fn: (e?) => void;
+                    }){
+    this.getMapProvider().addControl(control, position);
+    if(control instanceof  ControlButton && eventOverride){


extra space

QuCMGisaia · 2024-09-11T09:33:43Z

projects/arlas-components/src/lib/components/mapgl/model/ArlasMapGL.ts

Since in ArlasMapGL you have access to the _mapProvider, it would be best to use that instead of using the access method

QuCMGisaia · 2024-09-11T09:37:46Z

projects/arlas-components/src/lib/components/mapgl/model/map.type.ts

+ * the change is transparent.
+ */
+
+export interface MapOverride {


typing could be more informative in this interface

MohamedHamouGisaia · 2024-10-02T06:25:29Z

projects/arlas-components/src/lib/components/mapgl-basemap/mapgl-basemap.component.ts

@@ -91,8 +92,8 @@ export class MapglBasemapComponent implements OnInit {
  */
  public setStyle(s: mapboxgl.Style, newBasemap: BasemapStyle) {
    const selectedBasemapLayersSet = new Set<string>();
-    const layers: Array<mapboxgl.Layer> = (<mapboxgl.Map>this.map).getStyle().layers;
-    const sources = (<mapboxgl.Map>this.map).getStyle().sources;
+    const layers: Array<mapboxgl.Layer> = this.map.getStyle().layers;


mapboxgl is not removed for layers ?

MohamedHamouGisaia · 2024-10-02T06:26:40Z

projects/arlas-components/src/lib/components/mapgl/basemaps/basemap.service.ts

@@ -42,7 +43,7 @@ export class MapboxBasemapService {
    this.basemaps = basemaps;
  }

-  public addProtomapBasemap(map: mapboxgl.Map) {
+  public addProtomapBasemap(map: ArlasMapGL) {
    const selectedBasemap = this.basemaps.getSelected();
    if (selectedBasemap.type === 'protomap') {
      const styleFile = selectedBasemap.styleFile as mapboxgl.Style;


mapboxgl is not removed for layers ?

MohamedHamouGisaia · 2024-10-02T06:26:51Z

projects/arlas-components/src/lib/components/mapgl/basemaps/basemap.service.ts

@@ -68,7 +69,7 @@ export class MapboxBasemapService {
    this.protomapBasemapAddedSource.next(true);
  }

-  public removeProtomapBasemap(map: mapboxgl.Map) {
+  public removeProtomapBasemap(map: ArlasMapGL) {
    const selectedBasemap = this.basemaps.getSelected();
    if (selectedBasemap.type === 'protomap') {
      (selectedBasemap.styleFile as mapboxgl.Style).layers.forEach(l => {


mapboxgl is not removed for Style ?

MohamedHamouGisaia · 2024-10-02T06:27:51Z

projects/arlas-components/src/lib/components/mapgl/basemaps/basemap.service.ts

@@ -42,7 +43,7 @@ export class MapboxBasemapService {
    this.basemaps = basemaps;
  }

-  public addProtomapBasemap(map: mapboxgl.Map) {
+  public addProtomapBasemap(map: ArlasMapGL) {
    const selectedBasemap = this.basemaps.getSelected();
    if (selectedBasemap.type === 'protomap') {
      const styleFile = selectedBasemap.styleFile as mapboxgl.Style;


mapboxgl is not removed for styles ?

MohamedHamouGisaia · 2024-10-02T06:28:03Z

projects/arlas-components/src/lib/components/mapgl/basemaps/basemap.service.ts

@@ -68,7 +69,7 @@ export class MapboxBasemapService {
    this.protomapBasemapAddedSource.next(true);
  }

-  public removeProtomapBasemap(map: mapboxgl.Map) {
+  public removeProtomapBasemap(map: ArlasMapGL) {
    const selectedBasemap = this.basemaps.getSelected();
    if (selectedBasemap.type === 'protomap') {
      (selectedBasemap.styleFile as mapboxgl.Style).layers.forEach(l => {


mapboxgl is not removed for styles ?

MohamedHamouGisaia · 2024-10-02T06:31:47Z

projects/arlas-components/src/lib/components/mapgl/mapgl.component.ts

-  private north: number;
-  private east: number;
-  private west: number;
-  private south: number;
  private isDrawingBbox = false;
  private canvas: HTMLElement;
  private box: HTMLElement;
  // points which xy coordinates are in screen referential
  private start: mapboxgl.Point;


You still have some mapboxgl typing here

MohamedHamouGisaia · 2024-10-02T06:33:31Z

projects/arlas-components/src/lib/components/mapgl/mapgl.component.ts

@@ -461,8 +426,9 @@ export class MapglComponent implements OnInit, AfterViewInit, OnChanges, OnDestr
  private drawBboxSubscription: Subscription;

  public constructor(private http: HttpClient, private drawService: MapboxAoiDrawService,
-    private basemapService: MapboxBasemapService,
-    private _snackBar: MatSnackBar, private translate: TranslateService) {
+                     private basemapService: MapboxBasemapService,


The basemap service injected here is mapboxgl typed

MohamedHamouGisaia · 2024-10-02T06:45:40Z

projects/arlas-components/src/lib/components/mapgl/model/ArlasDraw.ts

+  'DRAW_LINE_STRING' | 'DRAW_POLYGON' | 'DRAW_POINT' | 'DRAW_RADIUS_CIRCLE' |
+  'DRAW_STRIP' | 'DIRECT_STRIP';
+
+export class ArlasDraw extends AbstractDraw {


should be named ArlasMapboxGLDraw ?

MohamedHamouGisaia · 2024-10-02T08:22:48Z

Rename ArlasMapGl, ArlasDraw and any other class that is an implementation of mapboxgl to
BasemapService abstrait + 2 implémentations
On utilise une class abstraite avec 2 services en implémentation, provide la classe abstraite + use class (l'implémentation)
Les fonctions qui ne dépendent pas de maplibre/mapbox peuvent soit être utilisé dans le composant ou le service abstrait
Generic type / Interface générique ?
Implémentation maplibre à faire.
On laisse deux implémentations mapbox (1.3) et maplibre à stabiliser
On passe en libs en suivant

sonarqubecloud · 2024-12-20T09:24:25Z

Quality Gate passed

Issues
54 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

MAudelGisaia requested review from sebbousquet, mbarbet, MohamedHamouGisaia and QuCMGisaia August 4, 2024 00:28

QuCMGisaia reviewed Aug 5, 2024

View reviewed changes

MAudelGisaia force-pushed the feat/maplibre-migration branch 7 times, most recently from 5d228ea to 272bf91 Compare September 10, 2024 15:03

QuCMGisaia reviewed Sep 11, 2024

View reviewed changes

MohamedHamouGisaia reviewed Oct 2, 2024

View reviewed changes

MAudelGisaia and others added 14 commits October 18, 2024 15:00

feat: refactor mapgl, maplayer

0fde8bb

fix: typo

6c05c62

feat: draw migration

f9955aa

fix: cleaning

241967b

fix: implement fix from commit 8e8734f

33bc0e3

feat: replace mapbox map into other components

28f5bca

feat: replace mapbox map into other components

71f0d7d

feat: replace mapbox map into other components

d768d52

feat: add provider methode override

3497933

feat: add licence

9f102e5

feat: implement feedback

fc120b2

feat: change to any , export mapOverride

c891013

Rename to mapbox and remove uneeded methods

c6a2f8a

Resolve QMC comments

5ca3d3e

Mohamed Hamou added 2 commits October 18, 2024 15:01

Fix lint

bb72f39

Add missing licence

0ab5adc

MAudelGisaia force-pushed the feat/maplibre-migration branch from 0b4c783 to 0ab5adc Compare October 18, 2024 13:08

feat: add good export

4e1d3c3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Feat/maplibre migration #849

WIP: Feat/maplibre migration #849

MAudelGisaia commented Aug 4, 2024 •

edited

Loading

QuCMGisaia Aug 5, 2024

QuCMGisaia Aug 5, 2024

QuCMGisaia Aug 5, 2024

QuCMGisaia commented Aug 5, 2024

QuCMGisaia Sep 11, 2024

QuCMGisaia Sep 11, 2024

QuCMGisaia Sep 11, 2024

QuCMGisaia Sep 11, 2024

QuCMGisaia Sep 11, 2024

QuCMGisaia Sep 11, 2024

QuCMGisaia Sep 11, 2024

QuCMGisaia Sep 11, 2024

QuCMGisaia Sep 11, 2024

QuCMGisaia Sep 11, 2024

QuCMGisaia Sep 11, 2024

MohamedHamouGisaia Oct 2, 2024

MohamedHamouGisaia Oct 2, 2024

MohamedHamouGisaia Oct 2, 2024

MohamedHamouGisaia Oct 2, 2024

MohamedHamouGisaia Oct 2, 2024

MohamedHamouGisaia Oct 2, 2024

MohamedHamouGisaia Oct 2, 2024

MohamedHamouGisaia Oct 2, 2024

MohamedHamouGisaia commented Oct 2, 2024

sonarqubecloud bot commented Dec 20, 2024

WIP: Feat/maplibre migration #849

Are you sure you want to change the base?

WIP: Feat/maplibre migration #849

Conversation

MAudelGisaia commented Aug 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuCMGisaia commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohamedHamouGisaia commented Oct 2, 2024

sonarqubecloud bot commented Dec 20, 2024

Quality Gate passed

MAudelGisaia commented Aug 4, 2024 •

edited

Loading