Optimization post release and cleanup of multiple TODOs #3209
Conversation
Fixing the deletion of a layer with an layer status of error Fixes Canadian-Geospatial-Platform#3175 Better parsing of the date values in date-mgt.ts Better fetching of the supported output formats for WFS layers Cleaner way to check for loaded plugins for the processors
Privatized the emitLayerEntryRegisterInit function More cleanup
Improved the typing in GeoviewVector.#convertCsv
Refactoring 'esriChildHasDetectedAnError' function for EsriDynamic and EsriFeature to better make sense of its purpose and also renamed it to checkForWarningOnTheLayerMetadata Refactoring abstract-geoview-layers.ts.#processListOfLayerEntryConfig function to better make sense of its purpose
Optimized the getFeatureInfo for the WMS More cleanup and useage of 'getOutfieldsPKNameOrDefault()' function
…r visibility when querying (also checking parent(s))
…ome logic from getLayerBounds() to better represent when the bounds are actually 'get' vs when they are 'calculated' Better getGeometryType
…preparing to review the queryable flags
…he logic into the layer itself, moved from the layer api
…taAccessPath and some dataAccessPath in the abstract-geoview-layers.ts and all inherited classes to be able to follow the flow better when properties are get, set, initialized and updated.
…ating some logic in the overall process between the sibling classes Better support of some WFS metadata where the attribute featureType.OutputFormats?.Format is a string instead of an array of string or an array of objects with a string inside, OGC standard thing.. Centralizing some features reading inside the GeoUtilities class, like 'readFeaturesFromWFS', 'readFeaturesFromWKB', 'readFeaturesFromKML', etc
jolevesq
left a comment
jolevesq
left a comment
There was a problem hiding this comment.
@jolevesq partially reviewed 6 files and made 4 comments.
Reviewable status: 83 of 107 files reviewed, 20 unresolved discussions (waiting on @Alex-NRCan).
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
// If no visibility by default has been configured and there's a defaultVisibility found in the layer metadata, apply the latter if (layerConfig.getInitialSettings()?.states?.visible === undefined && layerMetadata?.defaultVisibility) {
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
params: { LAYERS: `show:${layerConfig.layerId}`, ...(layerConfig.getSource().transparent !== undefined && { transparent: layerConfig.getSource().transparent }),
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
layer: EsriDynamic | EsriFeature, listOfLayerEntryConfig: ConfigBaseClass[], callbackWhenRegisteringConfig: RegisterLayerEntryConfigDelegate
Need param callback in JSDOC
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
#handleLayerQueryableChanged(layer: AbstractBaseGVLayer, event: LayerQueryableChangedEvent): void { // Redirect MapEventProcessor.setMapLayerQueryable(this.getMapId(), layer.getLayerPath(), event.queryable);
Should we set in store at the same place were we change actual value?
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 24 files and all commit messages, made 13 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
jolevesq
left a comment
jolevesq
left a comment
There was a problem hiding this comment.
@jolevesq reviewed 6 files and made 3 comments.
Reviewable status: 87 of 107 files reviewed, 23 unresolved discussions (waiting on @Alex-NRCan).
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
// Assign projection from config if present, otherwise use fallback (e.g., map's current projection) sourceOptions.projection = layerConfig.source.projection ? `EPSG:${layerConfig.source.projection}` : `${fallbackProjection}`;
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
); case CONST_LAYER_TYPES.ESRI_FEATURE: {
Now all this are managed within their own config type? That would make sense....
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
// GV Order of these keywords matter, preference will be given in this order static readonly EXCLUDED_HEADERS_LAT = ['latitude', 'lat', 'y', 'ycoord', 'latitude|latitude', 'latitude | latitude'];
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
Alex-NRCan
force-pushed
the
fix-cleanup
branch
from
414acbc to
c809c99
Compare
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 25 files and all commit messages, made 16 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
Yes, I was finally able to remove that fallbackProjection legacy code!
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
I don't think there's a rule, but I think it's more elegant that way and more easy when importing.
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Now all this are managed within their own config type? That would make sense....
Yes, basically!
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 25 files and all commit messages, made 16 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
Yes, I was finally able to remove that fallbackProjection legacy code!
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
I don't think there's a rule, but I think it's more elegant that way and more easy when importing.
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Now all this are managed within their own config type? That would make sense....
Yes, basically!
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 25 files and all commit messages, made 16 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
Yes, I was finally able to remove that fallbackProjection legacy code!
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
I don't think there's a rule, but I think it's more elegant that way and more easy when importing.
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Now all this are managed within their own config type? That would make sense....
Yes, basically!
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 25 files and all commit messages, made 16 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
Yes, I was finally able to remove that fallbackProjection legacy code!
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
I don't think there's a rule, but I think it's more elegant that way and more easy when importing.
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Now all this are managed within their own config type? That would make sense....
Yes, basically!
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 25 files and all commit messages, made 16 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
Yes, I was finally able to remove that fallbackProjection legacy code!
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
I don't think there's a rule, but I think it's more elegant that way and more easy when importing.
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Now all this are managed within their own config type? That would make sense....
Yes, basically!
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 25 files and all commit messages, made 16 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
Yes, I was finally able to remove that fallbackProjection legacy code!
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
I don't think there's a rule, but I think it's more elegant that way and more easy when importing.
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Now all this are managed within their own config type? That would make sense....
Yes, basically!
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 25 files and all commit messages, made 16 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
Yes, I was finally able to remove that fallbackProjection legacy code!
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
I don't think there's a rule, but I think it's more elegant that way and more easy when importing.
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Now all this are managed within their own config type? That would make sense....
Yes, basically!
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 25 files and all commit messages, made 16 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
Yes, I was finally able to remove that fallbackProjection legacy code!
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
I don't think there's a rule, but I think it's more elegant that way and more easy when importing.
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Now all this are managed within their own config type? That would make sense....
Yes, basically!
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 25 files and all commit messages, made 16 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
Yes, I was finally able to remove that fallbackProjection legacy code!
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
I don't think there's a rule, but I think it's more elegant that way and more easy when importing.
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Now all this are managed within their own config type? That would make sense....
Yes, basically!
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 25 files and all commit messages, made 16 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
Yes, I was finally able to remove that fallbackProjection legacy code!
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
I don't think there's a rule, but I think it's more elegant that way and more easy when importing.
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Now all this are managed within their own config type? That would make sense....
Yes, basically!
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 25 files and all commit messages, made 16 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
Yes, I was finally able to remove that fallbackProjection legacy code!
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
I don't think there's a rule, but I think it's more elegant that way and more easy when importing.
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Now all this are managed within their own config type? That would make sense....
Yes, basically!
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 25 files and all commit messages, made 16 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
Yes, I was finally able to remove that fallbackProjection legacy code!
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
I don't think there's a rule, but I think it's more elegant that way and more easy when importing.
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Now all this are managed within their own config type? That would make sense....
Yes, basically!
jolevesq
left a comment
jolevesq
left a comment
There was a problem hiding this comment.
@jolevesq reviewed 23 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @Alex-NRCan).
packages/geoview-core/src/geo/layer/gv-layers/raster/gv-wms.ts line 202 at r6 (raw file):
const initialSettingsBounds = wmsLayerConfig.getInitialSettingsBounds(); // TODO: CHECK - Do we want that kind of check for EsriDynamic as well?
We may... technically depending the query for a layer outside the bound should just return an empty array. It would be for performance purpose...
packages/geoview-core/src/geo/layer/geoview-layers/vector/esri-feature.ts line 190 at r6 (raw file):
// Check if feature count is too large if (responseDataCount.count > AbstractGeoViewVector.MAX_ESRI_FEATURES) {
Should we thrown an error or just loop to gat all features. Some layers have max record to 1000 but have 20 000 features.... We need to query them not throwing an error.
The fetchEsriFEatureByChunk is doing this...
packages/geoview-core/src/geo/map/map-viewer.ts line 845 at r6 (raw file):
}; // Update store... this will not emit the event becaus only when WCAG mode is enable
Typo becaus
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 25 files and all commit messages, made 17 comments, and resolved 5 discussions.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
You are right, it is not working like this now?
I've let the code with the || this.getServiceMetadata()?.serverType; and written the TODO to eventually remove it. Do you want me to do it now?
packages/geoview-core/src/api/event-processors/event-processor-children/drawer-event-processor.ts line 98 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does these static variables should be in overrides region?
They are static privates, so no overrides.
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 829 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Does it return the boolean for hoverable? If so should it be getMapHoverable?
It returns the TypeHoverFeatureInfo here, not a boolean? (I think reviewable is acting funny)
packages/geoview-core/src/api/types/layer-schema-types.ts line 341 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Like comment before, bounds are layers physical extent and extent is the restricted view... we may need a demo for this to show
I'll leave the TODO: CHECK here for now, because I'm still unsure personally.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is it trying to create time dimension later to set it time aware or not?
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
packages/geoview-core/src/core/exceptions/layer-exceptions.ts line 115 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
When does this error happen?
This error is only logged as a warning for now and it's when an Esri Dynamic has metadata with supportsDynamicLayers to false.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Have you seen performance improvement?
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
The worker script is mostly for data table export but could be use everywhere and be default esri fetch... it would improve performance when geometry is set to true
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to EsriUtilities.queryRecordsByUrlObjectIds which accept a returnGeometryparameter.
packages/geoview-core/src/geo/layer/layer.ts line 1290 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Add the throw to JSDOC
Done.
packages/geoview-core/src/geo/layer/layer.ts line 1992 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we set in store at the same place were we change actual value?
In this handler, the LayerApi listens for any layer that's changing its queryable property and when it happens it calls MapEventProcessor.setMapLayerQueryable() which takes care of updating the store.
Similar pattern to when we change the layer name and the layer api updates the store or when the layer status changes and the layer sets upd
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 58 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Need param callback in JSDOC
Done.
packages/geoview-core/src/geo/layer/geoview-layers/esri-layer-common.ts line 298 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this still work, not to override the visibility from initialSetting from metadata. It is done before in config validation?
Yes here we're in the metadata processing and that's when the defaultVisibility is checked and here the initialSettings states visible has to be undefined if we want the metadata to override it. The issue was that since a couple weeks, we were assigning states.visible to true by default prior to reaching the metadata processing. Therefore, the metadata didn't know if it had to override the config or not.
packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-image.ts line 242 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Minor comment but instead of calling getSource many time, should we call it before outside the const and reuse?
Yes, both work. Performance wise it's under 1ms and basically no impact.
packages/geoview-core/src/geo/layer/geoview-layers/raster/vector-tiles.ts line 288 at r5 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Projection is now always set so no need for the fallback? If so, it is the way to go.... reduce the undefined...
Yes, I was finally able to remove that fallbackProjection legacy code!
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 27 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is there a rule for this as we use const outside class many places, should we use this pattern? If they are not exported we should use this?
I don't think there's a rule, but I think it's more elegant that way and more easy when importing.
packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 250 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Now all this are managed within their own config type? That would make sense....
Yes, basically!
packages/geoview-core/src/geo/layer/geoview-layers/vector/esri-feature.ts line 190 at r6 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we thrown an error or just loop to gat all features. Some layers have max record to 1000 but have 20 000 features.... We need to query them not throwing an error.
The fetchEsriFEatureByChunk is doing this...
Yes, the fetchEsriFeaturesByChunk is doing the fetching 1000 at a time for example (20x), but the recordCount is another check which happens on MAX_ESRI_FEATURES which right now is hardcoded to 200,000 features.
Alex-NRCan
force-pushed
the
fix-cleanup
branch
from
c809c99 to
ba31f59
Compare
… is expected to be at certain position when calling the tests
Alex-NRCan
force-pushed
the
fix-cleanup
branch
from
ba31f59 to
c2cebb0
Compare
jolevesq
left a comment
jolevesq
left a comment
There was a problem hiding this comment.
@jolevesq made 5 comments and resolved 16 discussions.
Reviewable status: 103 of 107 files reviewed, 5 unresolved discussions (waiting on @Alex-NRCan).
packages/geoview-core/src/api/config/validation-classes/config-base-class.ts line 474 at r4 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Hmmm, I'm not sure this is what happens and it's the reason why I've written this TODO CHECK note. After optimizing the code, I've noticed that initInitialSettingsExtentFromMetadata was never called in the layer processing, and it seems like the replacement function initInitialSettingsExtentAndBoundsFromConfig is called. There's also initInitialSettingsBoundsFromMetadata that's called in 4-5 layer types, but not initInitialSettingsExtentFromMetadata. Therefore, I'm confused on how both properties are really managed.
We need a new issue to create a demo with bound and extent to showcase the difference and see if it works
packages/geoview-core/src/api/config/validation-classes/raster-validation-classes/ogc-wms-layer-entry-config.ts line 131 at r4 (raw file):
Previously, Alex-NRCan (Alex) wrote…
I've let the code with the
|| this.getServiceMetadata()?.serverType;and written the TODO to eventually remove it. Do you want me to do it now?
I think we can let it like this but at then end layer should be set... wehn we remove the || we can throw an error is not set?
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Here, it's making it 'isTimeAware' by default when adding a layer via its url. true is the default behavior anyways. isTimeAware only has an impact when set to false and when we explicitely want the layer to not care about the time awareness.
We'd need something in the add-new-layer UI itself to toggle it off if we would like to send isTimeAware false.
I dont think we need to set time aware false for the add panel. What it is really needed is to automatically add to time slider when layer isTimeAware. It may be false by default when we add a config layer with isTimeAware false. This flag was for OSDP... In RAMP there is no check for dimension to see if it good so many WMS are tag with bad time dimension and does not work whit time slider.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, Alex-NRCan (Alex) wrote…
For what I was doing, I needed it or it was looping, probably because of a spreading in a useEffect somewhere. This is an implementation of the shallow equal sometimes suggested by Zustand (and often suggested by ChatGPT). The way the geoview store is implemented it was hard to write the code with the zustand.shallow so this is the manual equivalent.
Should we encapsulate our store selector to use this?
packages/geoview-core/src/core/stores/store-interface-and-intial-values/layer-state.ts line 142 at r4 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Oof this one is hard to read with reviewable, because the code has moved to getRecordsByOIDs and we could easily open a parameter there for the call to
EsriUtilities.queryRecordsByUrlObjectIdswhich accept areturnGeometryparameter.
The FetchEsriWorker is in gv-esri-dynamic.... fetchAllFeatureInfoWithWorker -> getAllFeatureInfo...
We should try to use more worker for for WFS, esri queries as it may becaome huge and freeze the viewer
Alex-NRCan
left a comment
Alex-NRCan
left a comment
There was a problem hiding this comment.
@Alex-NRCan reviewed 4 files and all commit messages, made 3 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jolevesq).
packages/geoview-core/src/api/config/validation-classes/config-base-class.ts line 474 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
We need a new issue to create a demo with bound and extent to showcase the difference and see if it works
Yeah, I'll leave the TODO there for now so we don't forget.
packages/geoview-core/src/core/components/layers/left-panel/add-new-layer/add-new-layer.tsx line 416 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
I dont think we need to set time aware false for the add panel. What it is really needed is to automatically add to time slider when layer isTimeAware. It may be false by default when we add a config layer with isTimeAware false. This flag was for OSDP... In RAMP there is no check for dimension to see if it good so many WMS are tag with bad time dimension and does not work whit time slider.
Done.
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Should we encapsulate our store selector to use this?
We could review our current hook selectors to use this when appropriate, right now it's used only for the useMapSelectorLayerQueryable hook. I didn't want to start doing this in this PR.
jolevesq
left a comment
jolevesq
left a comment
There was a problem hiding this comment.
@jolevesq reviewed 6 files and all commit messages, and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Alex-NRCan).
jolevesq
left a comment
jolevesq
left a comment
There was a problem hiding this comment.
@jolevesq reviewed 27 files and made 1 comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Alex-NRCan).
packages/geoview-core/src/geo/utils/utilities.ts line 1093 at r8 (raw file):
* @returns number representing the closest scale for the given zoom number */ // TODO: Cleanup - This function doesn't seem to be used anywhere?
This is utilities function accessible from the api cgpv. Should we keep them as it may be use by dev?
jolevesq
left a comment
jolevesq
left a comment
There was a problem hiding this comment.
@jolevesq made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Alex-NRCan).
packages/geoview-core/src/core/stores/geoview-store.ts line 151 at r4 (raw file):
Previously, Alex-NRCan (Alex) wrote…
We could review our current hook selectors to use this when appropriate, right now it's used only for the
useMapSelectorLayerQueryablehook. I didn't want to start doing this in this PR.
Ok, just take a note
…d couple comments adjustments
2 participants
Description
Additional:
Additional, additional:
Type of change
How Has This Been Tested?
Hosted: Dec 22 @ 17h : https://alex-nrcan.github.io/geoview/Hosted: Jan 7 @ 17h : https://alex-nrcan.github.io/geoview/
Checklist:
I have made corresponding changes to the documentationI have added tests that prove my fix is effective or that my feature worksNew and existing unit tests pass locally with my changesThis change is