2611 visibility refresh

[MatthewMuehlhauserNRCan]

Description

Added functionality to update the visibility and remove layers as necessary on language change. Also cleaned up the setLanguage method in the map-viewer.ts.

Fixes # 2611

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Turned off a layer and removed a child layer and then changed the language.

GeoCore Custom

Checklist:

• I have build (rush build) and deploy (rush host) my PR
• I have connected the issues(s) to this PR
• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have created new issue(s) related to the outcome of this PR is needed
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

---

This change is 

[DamonU2]

Reviewed 1 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Alex-NRCan, @jolevesq, and @MatthewMuehlhauserNRCan)

---

packages/geoview-core/src/geo/map/map-viewer.ts line 941 at r2 (raw file):

        // Reload just the Geocore Layers instead of the entire map
        this.layer.reloadGeocoreLayers();
        // this.reloadWithCurrentState();

Need to keep?

---

packages/geoview-core/src/geo/layer/layer.ts line 589 at r2 (raw file):

  /**
   * Refreshes GeoCore Layers
   * @returns {void} A promise which resolves when done refreshing

No longer a promise - update comment

[MatthewMuehlhauserNRCan]

packages/geoview-core/src/geo/map/map-viewer.ts line 941 at r2 (raw file):

Previously, DamonU2 (Damon Ulmi) wrote…

Need to keep?

I wasn't sure if wanted that as a reference there that it was tested? It is an alternative to the current solution, although it would refresh the entire map and not just the GeoCore layers. Which one is better would depend on if there are layers in the map that aren't GeoCore layers since they would be reloaded as well.

[MatthewMuehlhauserNRCan]

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @Alex-NRCan, @DamonU2, and @jolevesq)

---

packages/geoview-core/src/geo/layer/layer.ts line 589 at r2 (raw file):

Previously, DamonU2 (Damon Ulmi) wrote…

No longer a promise - update comment

Done.

[jolevesq]

Reviewed 1 of 2 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @Alex-NRCan and @DamonU2)

---

packages/geoview-core/src/geo/map/map-viewer.ts line 941 at r2 (raw file):

Previously, MatthewMuehlhauserNRCan wrote…

I wasn't sure if wanted that as a reference there that it was tested? It is an alternative to the current solution, although it would refresh the entire map and not just the GeoCore layers. Which one is better would depend on if there are layers in the map that aren't GeoCore layers since they would be reloaded as well.

If the relaod geocore works, we do not need the refresh state. It was another solution if the geocore switch as not working good.

[jolevesq]

Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @Alex-NRCan and @DamonU2)

---

packages/geoview-core/src/geo/map/map-viewer.ts line 941 at r2 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

If the relaod geocore works, we do not need the refresh state. It was another solution if the geocore switch as not working good.

We can remove or put in comment that we reload instead of relaod with current state to not reload the whole map

[MatthewMuehlhauserNRCan]

packages/geoview-core/src/geo/map/map-viewer.ts line 941 at r2 (raw file):

Previously, jolevesq (Johann Levesque) wrote…

We can remove or put in comment that we reload instead of relaod with current state to not reload the whole map

I did that. I removed the code and then added a little bit to the comment. I also combined the two comments there since it's such a small section.

[DamonU2]

Reviewed 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Alex-NRCan)

[Alex-NRCan]

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @DamonU2 and @MatthewMuehlhauserNRCan)

---

packages/geoview-core/src/geo/layer/layer.ts line 660 at r5 (raw file):

          }
          // Tried with onLayerLoaded, but didn't work as expected
          this.onLayerAdded(setLayerVisibility);

I was curious why the onLayerLoaded couldn't be used here and I think I've figured it out. 2 reasons:

1- There's an issue with how the LayerApi registers the onLayerLoaded event which we have an inbound fix for in another big PR. I'd suggest to cherry pick the fix right away. I'll create a small PR for it. It'll save a lot of code here and remove the need for a 'whenThisThen'.

2- In the layers classes (old and new), in their onLoaded() function (in which they raise their loaded event), there's code that sets the visibility right after triggering the event. I believe this is because one can't set the visibility of a layer until it's in an OpenLayers loaded state on the map (iirc I've read a comment about something like this when doing the layers refactoring). If we want our own 'layerLoaded' event to be raised after we've set the visibility flag, which I think makes sense, we should switch these 2 lines order:

Switching those 2 lines will set the visibility as expected, by default using the initialSettings, and only then emit the event, which only then will be caught by the code here, re-adjusting the visibility to what we actually want. Otherwise, the visibility of the layer would get changed here (when caught by the onLayerLoaded), then re-changed in the screenshotted function to its initialSettings, which isn't what we want in this case.

[Alex-NRCan]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @DamonU2, @jolevesq, and @MatthewMuehlhauserNRCan)

---

packages/geoview-core/src/geo/layer/layer.ts line 660 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

I was curious why the onLayerLoaded couldn't be used here and I think I've figured it out. 2 reasons:

1- There's an issue with how the LayerApi registers the onLayerLoaded event which we have an inbound fix for in another big PR. I'd suggest to cherry pick the fix right away. I'll create a small PR for it. It'll save a lot of code here and remove the need for a 'whenThisThen'.

2- In the layers classes (old and new), in their onLoaded() function (in which they raise their loaded event), there's code that sets the visibility right after triggering the event. I believe this is because one can't set the visibility of a layer until it's in an OpenLayers loaded state on the map (iirc I've read a comment about something like this when doing the layers refactoring). If we want our own 'layerLoaded' event to be raised after we've set the visibility flag, which I think makes sense, we should switch these 2 lines order:

Switching those 2 lines will set the visibility as expected, by default using the initialSettings, and only then emit the event, which only then will be caught by the code here, re-adjusting the visibility to what we actually want. Otherwise, the visibility of the layer would get changed here (when caught by the onLayerLoaded), then re-changed in the screenshotted function to its initialSettings, which isn't what we want in this case.

The hot fix PR is: #2634

After these 2 points addressed. I believe the code here becomes the following and the setLayerVisibility function can be removed:

Code snippet:

// Catch the layer loaded event
this.onLayerLoaded((sender, event) => {
  const { visible } = originalMapOrderedLayerInfo.filter((info) => info.layerPath === event.layerPath)[0];
  event.layer?.setVisible(visible, path);
});

[Alex-NRCan]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @DamonU2, @jolevesq, and @MatthewMuehlhauserNRCan)

---

packages/geoview-core/src/geo/layer/layer.ts line 660 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

The hot fix PR is: #2634

After these 2 points addressed. I believe the code here becomes the following and the setLayerVisibility function can be removed:

Rather, this, most likely:

Code snippet:

// Catch the layer loaded event
this.onLayerLoaded((sender, event) => {
  const { visible } = originalMapOrderedLayerInfo.filter((info) => info.layerPath === event.layerPath)[0];
  event.layer?.setVisible(visible, event.layerPath);
});

[MatthewMuehlhauserNRCan]

packages/geoview-core/src/geo/layer/layer.ts line 660 at r5 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Rather, this, most likely:

I've switched to the onLayerLoaded after the rebase. It seems to work well and simplifies the code a lot. I did keep the callback function so I was able to remove the listener afterwards so they don't continue to build up. Although, they could potentially build up if someone continues to switch back and forth really fast. Maybe clearing the handlers array could make sure that doesn't happen, but I don't want to remove anything important that may also use this, although when I currently check, it is an empty array.

[Alex-NRCan]

Reviewed 2 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @MatthewMuehlhauserNRCan)