GSLUX-704: Save drawn features #135

tkohr · 2024-08-09T14:43:45Z

JIRA issue

https://jira.camptocamp.com/browse/GSLUX-704

Description

This PR implements the logic for saving drawn features and persisting them in the URI. It is still WIP. Persisting and restoring is functional, except for a remaining issue that the restored feature is not yet displayed on the map according to its properties. The persist step can also be tested by copying the features=... queryParam to the prod instance where the features are displayed in the according "style". (Note that the style encoding/decoding in the code is not used for this, but the style should work based on the properties).

To-dos for this PR include:

fix aforementioned restore of feature (correct rendering on map)
potentially some refactoring:
- All content in the utils folder was taken and adapted from v3. It could possibly be better integrated into the v4 architecture.
- ol import syntax is not consistent in FeatureHash
- declaration of modules and typing could be further cleaned
- Note: encode/decodeUriComponent has been removed from FeatureHash and to let state persistor take care of this.
fix & add tests

Future to-dos that depend on other features and can not yet be covered by this PR are:

migrate functionality related to 3D
migrate functionality related to MyMaps (save, check if map id present, refreshProfile => GSLUX-710
selecting features (selectedFeatures) => GSLUX-705
on prod draw tools are deactivated after drawing a feature and the feature is selected

github-actions · 2024-08-09T14:45:04Z

GitHub Pages links: * Luxembourg-geoportail: https://geoportail-luxembourg.github.io/luxembourg-geoportail/GSLUX-704-draw-end/

mki-c2c · 2024-08-16T13:36:26Z

Style decoding form short URL format to mymaps format is done via helper file.
V3 code is still being reused without much refactoring.

User Features with various styling can be tested with this URL:
https://geoportail-luxembourg.github.io/luxembourg-geoportail/GSLUX-704-draw-end/?version=3&X=662256&Y=6359947&zoom=17&features=Fa%28bse9-fs65Hp-w_d%21d_Dm_u%21s_%21a*~display_order*0%27d*%27__refreshProfile__*false%27a*0%27c*%2523ed1c24%27e*1%27i*false%27l*plain%27n*Polygone%25201%27o*0.2%27r*false%27s*circle%27t*10%27u*false%29a%28yxe9-ht65H8Dt_h_g*y%211-~display_order*1%27d*%27__refreshProfile__*false%27a*0%27c*%25231dcaed%27e*1%27i*false%27l*plain%27n*Polygone%25202%27o*1%27r*false%27s*circle%27t*10%27u*false%29a%28yqe9-nq65Hf*sLeEe*n_jCK2_-c_~display_order*2%27d*%27__refreshProfile__*false%27a*0%27c*%25232a1ded%27e*4%27i*false%27l*plain%27n*Polygone%25203%27o*0.48%27r*false%27s*circle%27t*10%27u*false%29a%28hqe9-9n65H.N%21L%21L%21LCJCLEHGHGHIFIDIDKBK*M_K.K.K-M%21IAKCICIEGGGGEGCKAIAK%21K-K-M_K_K*KBKBKDIFIHGHEJEJELAJAN%21L-L-L_N_L*LBJBJFJFHFHHFJDJDL*L*L*L~display_order*5%27__refreshProfile__*false%27d*%27a*0%27c*%2523ed1c24%27e*1%27i*false%27l*plain%27n*Kreis%25206%27o*0.2%27r*false%27s*circle%27t*10%27u*true%29p%28s1e9-1f65H~display_order*6%27__refreshProfile__*false%27d*%27a*0%27c*%2523ed1c24%27e*1.25%27i*true%27l*plain%27n*Mon%2520Label%27o*0.2%27r*false%27s*circle%27t*26%27u*false%29p%281yd9-ut65H~display_order*7%27__refreshProfile__*false%27d*%27a*-0.8726646259971648%27c*%25231ded2a%27e*1.25%27i*true%27l*plain%27n*Mon%2520Label%27o*0.2%27r*false%27s*circle%27t*23%27u*false%29

AlitaBernachot · 2024-08-16T14:40:45Z

src/composables/draw/draw-utils.ts

+// TODO 3D
+// const ARROW_MODEL_URL = import.meta.env.VITE_ARROW_MODEL_URL
+
+export function createStyleFunction(curMap: OlMap) {


We should rework this, seperate default style, style for point and style for label, style for selectiion, etc... the v3 code is not very clear, it is hard to tell what style is for.

partly done,
common state of type DrawnFeature[]

AlitaBernachot · 2024-08-16T14:43:59Z

src/composables/draw/drawn-features.composable.ts

+    feature.set('shape', 'circle')
+    feature.set('isLabel', drawStateActive.value === 'drawLabel')
+    feature.setStyle(createStyleFunction(map))
+    feature.set('display_order', nbFeatures)


TODO: put all this definition in a wrapper dedicated to v4 containing these info and the ol feature geom. To be discussed together.

src/services/state-persistor/utils/FeatureHash.d.ts

src/services/state-persistor/utils/GeometryTypes.ts

src/services/state-persistor/utils/utils.d.ts

src/services/state-persistor/utils/utils.ts

AlitaBernachot

Thanks for the work! working great.

AlitaBernachot

Rquesting changes after a second review. There is rework todoregarding the implementation, we should not use ol objects to store infos (name, width, etc...). Plus, there is a lot of copy/paste from v3, the goal of v4 is to make things clearer and simpler, using vue states/stores etc...

mki-c2c · 2024-08-19T07:36:19Z

for demo, the link below can be used:
github pages demo

It contains some styled features created with v3 and some features created with v4.

When new features are added, they accumulate in the permalink
deletion of features is not yet implemented

AlitaBernachot · 2024-08-19T09:04:35Z

Thanks for the rebase and the link.

I noticed the component for styling the feature is not displayed anymore (probably somethign wrong about the feature type):

src/services/state-persistor/state-persistor-features.mapper.ts

src/services/state-persistor/state-persistor-features.service.ts

src/services/state-persistor/utils/FeatureHash.ts

AlitaBernachot · 2024-08-19T15:32:23Z

src/services/state-persistor/utils/FeatureHash.ts

+import Style from 'ol/style/Style.js'
+import { createStyleFunction } from '@/composables/draw/draw-utils'
+
+const GeometryTypeValues = {


redefining types again? src/services/state-persistor/utils/GeometryTypes.ts

inline because it is a non reusable v3 component

src/services/state-persistor/utils/FeatureHash.ts

AlitaBernachot · 2024-08-19T15:34:02Z

src/services/state-persistor/utils/FeatureHash.ts

+
+/**
+ * @type {Object.<ol.geom.GeometryType, FeatureHashStyleType>}
+ * @private


why @private?

AlitaBernachot · 2024-08-19T15:35:48Z

src/services/state-persistor/utils/FeatureHash.ts

+ * @type {Object.<string, string>}
+ * @private
+ */
+let FeatureHashLegacyProperties: any = {}


can't we provide a better typing than any? what about the jsdoc above saying <string, string>?

for the moment, I made a linter exception because it was a big piece of code to port to TS

…s and in the map layer

mki-c2c · 2024-09-02T17:34:47Z

Thanks for the rebase and the link.

I noticed the component for styling the feature is not displayed anymore (probably somethign wrong about the feature type):

fixed

class DrawnFeature extends Feature

src/components/draw/draw-panel-features.vue.orig

src/services/draw/drawn-feature.ts

* fix circle e2e test * fix the e2e tests * re-fix * fix types

AlitaBernachot · 2024-09-04T09:48:40Z

src/services/state-persistor/utils/FeatureStyleHelper.ts

+   */
+  public getType(feature: Feature) {
+    const geometry = feature.getGeometry()
+    console.assert(geometry, 'Geometry should be thruthy')


why use console.assert in v4?

does is matter ?

it was a goog assert and I wanted to preserve the verification

yes code quality matters

…ding algorithm, to be able to add guards or useful help comments later

tkohr

Thanks for your huge effort on refactoring and finishing this PR @mki-c2c !

tkohr · 2024-09-04T14:56:27Z

src/composables/draw/drawn-features.composable.ts

+    // // TODO: to improve saving
+    // const geomType = feature.getGeometry()?.getType()
+
+    // const newFeature = createDrawnFeature(feature)
+
+    // // if (geomType) {
+    // //   const newFeature = {
+    // //     // TODO: improve feat. creation, move this creation elsewhere
+    // //     id: Math.floor(Math.random() * Date.now()),
+    // //     label: `${geomType as string} ${drawnFeatures.value.length + 1}`,
+    // //     featureType: feature.get('isCircle')
+    // //       ? 'drawnCircle'
+    // //       : feature.get('isLabel')
+    // //       ? 'drawnLabel'
+    // //       : `drawn${geomType.replace('String', '')}`,
+    // //     olFeature: feature,
+    // //   } as unknown as DrawFeature
+    // if (newFeature) {
+    //   drawnFeatures.value = [...drawnFeatures.value, newFeature]
+    // }


Can we get rid of this now?

tkohr · 2024-09-04T14:56:39Z

src/composables/draw/drawn-features.composable.ts

+  // function getName(feature: Feature<Geometry>) {
+  //   if (feature?.getGeometry()?.getType() === 'Circle') {
+  //     const featureGeom = feature.getGeometry() as Circle
+  //     feature.set('isCircle', true)
+  //     feature.setGeometry(fromCircle(featureGeom, 64))
+  //   }
+
+  //   let name
+  //   switch (feature.getGeometry()?.getType()) {
+  //     case 'Point': {
+  //       if (drawStateActive.value === 'drawLabel') {
+  //         name = t('Label', { ns: 'client' })
+  //       } else {
+  //         name = t('Point', { ns: 'client' })
+  //       }
+  //       break
+  //     }
+  //     case 'LineString': {
+  //       const curLineStringGeom = feature.getGeometry() as LineString
+  //       const curLineStringCooridnates = curLineStringGeom.getCoordinates()
+  //       if (curLineStringCooridnates.length < 2) {
+  //         return
+  //       }
+  //       const prevCoord =
+  //         curLineStringCooridnates[curLineStringCooridnates.length - 1]
+  //       const antePrevCoord =
+  //         curLineStringCooridnates[curLineStringCooridnates.length - 2]
+  //       if (
+  //         prevCoord[0] === antePrevCoord[0] &&
+  //         prevCoord[1] === antePrevCoord[1]
+  //       ) {
+  //         curLineStringCooridnates.pop()
+  //         if (curLineStringCooridnates.length < 2) {
+  //           return
+  //         }
+  //         curLineStringGeom.setCoordinates(curLineStringCooridnates)
+  //       }
+  //       name = t('LineString', { ns: 'client' })
+  //       break
+  //     }
+  //     case 'Polygon': {
+  //       if (feature.get('isCircle')) {
+  //         name = t('Circle', { ns: 'client' })
+  //       } else {
+  //         const featureGeom = feature.getGeometry() as Polygon
+  //         if (featureGeom.getLinearRing(0)!.getCoordinates().length < 4) {
+  //           return
+  //         }
+  //         name = t('Polygon', { ns: 'client' })
+  //       }
+  //       break
+  //     }
+  //     default:
+  //       name = feature.getGeometry()?.getType()
+  //       break
+  //   }
+  //   return name
+  // }


Does this remain a to-do?

mki-c2c force-pushed the GSLUX-704-draw-end branch from f6f570c to 46ae810 Compare August 16, 2024 13:20

mki-c2c marked this pull request as ready for review August 16, 2024 13:26

AlitaBernachot reviewed Aug 16, 2024

View reviewed changes

src/services/state-persistor/utils/FeatureHash.d.ts Outdated Show resolved Hide resolved

AlitaBernachot reviewed Aug 16, 2024

View reviewed changes

src/services/state-persistor/utils/GeometryTypes.ts Outdated Show resolved Hide resolved

AlitaBernachot reviewed Aug 16, 2024

View reviewed changes

src/services/state-persistor/utils/GeometryTypes.ts Outdated Show resolved Hide resolved

AlitaBernachot reviewed Aug 16, 2024

View reviewed changes

src/services/state-persistor/utils/GeometryTypes.ts Outdated Show resolved Hide resolved

AlitaBernachot reviewed Aug 16, 2024

View reviewed changes

src/services/state-persistor/utils/utils.d.ts Outdated Show resolved Hide resolved

AlitaBernachot reviewed Aug 16, 2024

View reviewed changes

src/services/state-persistor/utils/utils.d.ts Outdated Show resolved Hide resolved

AlitaBernachot reviewed Aug 16, 2024

View reviewed changes

src/services/state-persistor/utils/utils.ts Outdated Show resolved Hide resolved

AlitaBernachot reviewed Aug 16, 2024

View reviewed changes

src/services/state-persistor/utils/utils.ts Outdated Show resolved Hide resolved

AlitaBernachot approved these changes Aug 16, 2024

View reviewed changes

AlitaBernachot requested changes Aug 18, 2024

View reviewed changes

mki-c2c force-pushed the GSLUX-704-draw-end branch from b75683d to 9fc0e3e Compare August 19, 2024 07:31