fix(details/table): No table for pyGeoApiProcess and CSV #2146
Conversation
There was a problem hiding this comment.
Reviewed 8 of 9 files at r1, all commit messages.
Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @Alex-NRCan, @DamonU2, and @ychoquet)
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 84 at r1 (raw file):
// #endregion FEATURE SELECTION // const unsubOrderedLayerInfo = store.subscribe(
Is this good to remove and use the one from state instead?
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @DamonU2, @jolevesq, and @ychoquet)
packages/geoview-core/src/api/event-processors/event-processor-children/map-event-processor.ts line 84 at r1 (raw file):
Previously, jolevesq (Johann Levesque) wrote…
Is this good to remove and use the one from state instead?
Hmm not sure either, see other comment
packages/geoview-core/src/core/stores/store-interface-and-intial-values/map-state.ts line 635 at r1 (raw file):
}); // GV subscrite from map-eent-processor is unstable for layer coming from PyGeoApiProcess, even if layer is added
Couple of typos here.
Also, this is getting a bit too heavy for a 'setterActions'. The 'setterActions' were created to focus on setting a single state to distinguish between logic and setter, as things were getting out of hand. Could it be moved to a regular action to do this maybe?
Also, I'm curious why the subscribe wasn't working as I think it made more sense in the design to have it there.
A problem (or good thing?) by moving the code here means the visibility stuff isn't adjusted when other actions happen that were setting ordered layer info (like setHoverable, setQueryable, etc). Again, not sure if it's a problem or not, but it's definitely quite different.
packages/geoview-core/src/geo/layer/geoview-layers/vector/csv.ts line 277 at r1 (raw file):
// process the feature info configuration and attach the config to the instance for access by parent class CSV.#processFeatureInfoConfig(headers, csvRows[1], [latIndex, lonIndex], layerConfig); this.layerMetadata[layerConfig.layerPath] = layerConfig as unknown as TypeJsonObject;
Isn't the metadata supposed to be handle via commonProcessLayerMetadata and processLayerMetadata functions?
I feel like this is cheating/shortcutting a couple things! 😮
I even wanted to "privatize" this in an upcoming PR to not further confuse when/where things happen.
packages/geoview-core/src/geo/layer/layer-sets/all-feature-info-layer-set.ts line 47 at r1 (raw file):
// TD.CONT: if not specified to false by default, we will set it to true const queryable = layerConfig?.source?.featureInfo?.queryable; return !!(queryable || queryable === undefined);
Is that really only for CSV? If so, wouldn't it better to fix the issue on CSV side?
This change will apply to all layers, so for all layers, with this code, when there's no source, it means it's queryable.
packages/geoview-core/src/geo/layer/layer-sets/all-feature-info-layer-set.ts line 55 at r1 (raw file):
* @param {TypeLayerEntryConfig} layerConfig - The layer config */ protected override async onRegisterLayer(geoviewLayer: AbstractGeoViewLayer, layerConfig: TypeLayerEntryConfig): Promise<void> {
This changes the signature of the function to return a Promise, and since this function overrides the one in mother class, the mother class signature should be changed (along with all other layer-sets siblings).
packages/geoview-core/src/geo/layer/layer-sets/all-feature-info-layer-set.ts line 68 at r1 (raw file):
try { // Check if the layer name is define in configuration, wait for it await whenThisThen(
Let's talk about what you're trying to achieve here
packages/geoview-core/src/geo/layer/layer-sets/feature-info-layer-set.ts line 77 at r1 (raw file):
// TD.CONT: if not specified to false by default, we will set it to true const queryable = layerConfig?.source?.featureInfo?.queryable; return !!(queryable || queryable === undefined);
Same comment as other class
packages/geoview-core/src/geo/layer/layer-sets/feature-info-layer-set.ts line 98 at r1 (raw file):
try { // Check if the layer name is define in configuration, wait for it await whenThisThen(
Same comment
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, and @jolevesq)
packages/geoview-core/src/geo/layer/geoview-layers/vector/csv.ts line 277 at r1 (raw file):
// process the feature info configuration and attach the config to the instance for access by parent class CSV.#processFeatureInfoConfig(headers, csvRows[1], [latIndex, lonIndex], layerConfig); this.layerMetadata[layerConfig.layerPath] = layerConfig as unknown as TypeJsonObject;
Just for information, theCast<newType>(anObject) does the same thing than anObject as unknown as newType.
Code quote:
as unknown as TypeJsonObjectpackages/geoview-core/src/geo/layer/geoview-layers/vector/csv.ts line 277 at r1 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Isn't the metadata supposed to be handle via
commonProcessLayerMetadataandprocessLayerMetadatafunctions?
I feel like this is cheating/shortcutting a couple things! 😮
I even wanted to "privatize" this in an upcoming PR to not further confuse when/where things happen.
In the old implementation, the metadata and layerMetadata properties of a geoview layer was kept for debugging purpose (to find out what went wrong when we translated the external metadata to the layer config). The goal is to translate all flavors of external metadata into an internal configuration that is always the same and only use the internal config to display the layer.
packages/geoview-core/src/geo/layer/layer-sets/all-feature-info-layer-set.ts line 47 at r1 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Is that really only for CSV? If so, wouldn't it better to fix the issue on CSV side?
This change will apply to all layers, so for all layers, with this code, when there's no source, it means it's queryable.
In fact, the default value for queryable is false. If the metadata or the user doen't set this flag to true, we cannot assume the layer is queryable.
There was a problem hiding this comment.
Reviewable status: 4 of 10 files reviewed, 9 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, and @ychoquet)
packages/geoview-core/src/geo/layer/geoview-layers/vector/csv.ts line 277 at r1 (raw file):
Previously, ychoquet (Yves Choquette) wrote…
In the old implementation, the metadata and layerMetadata properties of a geoview layer was kept for debugging purpose (to find out what went wrong when we translated the external metadata to the layer config). The goal is to translate all flavors of external metadata into an internal configuration that is always the same and only use the internal config to display the layer.
Moved and Cast
packages/geoview-core/src/geo/layer/layer-sets/all-feature-info-layer-set.ts line 47 at r1 (raw file):
Previously, ychoquet (Yves Choquette) wrote…
In fact, the default value for queryable is false. If the metadata or the user doen't set this flag to true, we cannot assume the layer is queryable.
Will keep so layer are queryable unless it is false
packages/geoview-core/src/geo/layer/layer-sets/all-feature-info-layer-set.ts line 55 at r1 (raw file):
Previously, Alex-NRCan (Alex) wrote…
This changes the signature of the function to return a Promise, and since this function overrides the one in mother class, the mother class signature should be changed (along with all other layer-sets siblings).
Moved to abstract
packages/geoview-core/src/geo/layer/layer-sets/all-feature-info-layer-set.ts line 68 at r1 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Let's talk about what you're trying to achieve here
Moved to abstract
packages/geoview-core/src/geo/layer/layer-sets/feature-info-layer-set.ts line 77 at r1 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Same comment as other class
Same comment as above
packages/geoview-core/src/geo/layer/layer-sets/feature-info-layer-set.ts line 98 at r1 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Same comment
Moved to abstract
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 4 of 10 files reviewed, 9 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, and @ychoquet)
packages/geoview-core/src/core/stores/store-interface-and-intial-values/map-state.ts line 635 at r1 (raw file):
Previously, Alex-NRCan (Alex) wrote…
Couple of typos here.
Also, this is getting a bit too heavy for a 'setterActions'. The 'setterActions' were created to focus on setting a single state to distinguish between logic and setter, as things were getting out of hand. Could it be moved to a regular
actionto do this maybe?Also, I'm curious why the subscribe wasn't working as I think it made more sense in the design to have it there.
A problem (or good thing?) by moving the code here means the visibility stuff isn't adjusted when other actions happen that were setting ordered layer info (like setHoverable, setQueryable, etc). Again, not sure if it's a problem or not, but it's definitely quite different.
Modified the behavior of this as mentioned. May not be the best part for spreading array but added a TODO
There was a problem hiding this comment.
Reviewed 3 of 6 files at r2.
Reviewable status: 7 of 10 files reviewed, 9 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, and @ychoquet)
packages/geoview-core/src/geo/layer/geoview-layers/vector/csv.ts line 277 at r1 (raw file):
Previously, ychoquet (Yves Choquette) wrote…
Just for information, theCast<newType>(anObject) does the same thing than anObject as unknown as newType.
Done.
There was a problem hiding this comment.
Reviewed 5 of 6 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @DamonU2, @jolevesq, and @ychoquet)
jolevesq
merged commit 63c6bbc
into
Canadian-Geospatial-Platform:develop
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #1935
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
hosted on May 16th, 10AM:
https://jolevesq.github.io/geoview/pygeoapi-processes.html
https://jolevesq.github.io/geoview/demos-navigator.html?config=./configs/navigator/23-csv.json
https://jolevesq.github.io/geoview/raw-add-layers.html
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