Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(geo): Set hybrid mode as default to use gv-layers class #2620

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

jolevesq
Copy link
Member

@jolevesq jolevesq commented Nov 27, 2024

Closes #2617

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.

Link the viewer to only the hybrid mode. Remove reference toold code and hybrid.

Fixes #2617

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?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

Add the URL for your deploy!

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 Reviewable

Copy link
Member

@Alex-NRCan Alex-NRCan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 24 of 24 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jolevesq)


packages/geoview-core/src/api/event-processors/event-processor-children/time-slider-event-processor.ts line 131 at r1 (raw file):

    // Cast the layer
    const geoviewLayerCasted = geoviewLayer as AbstractGVLayer | AbstractGVLayer;

Ici, on peut enlever la redondance


packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 337 at r1 (raw file):

  protected createVectorLayer(layerConfig: VectorLayerEntryConfig, vectorSource: VectorSource): VectorLayer<Feature> {
    // GV Time to request an OpenLayers layer!
    const requestResult = this.emitLayerRequesting({ config: layerConfig, source: vectorSource });

There may be some additional enhancements to be done now that we can notice how emitLayerRequesting and emitLayerCreation are getting "close" to each other. Like merging them into one maybe? Food for thought.


packages/geoview-core/src/geo/layer/layer-sets/all-feature-info-layer-set.ts line 113 at r1 (raw file):

      // If layer was found
      if (layer && (layer instanceof AbstractGeoViewLayer || layer instanceof AbstractGVLayer)) {

The AbstractGeoViewLayer can probably be removed here (and in all other similar places in the other layer sets classes).
In fact, a code search for AbstractGeoViewLayer should be made to likely remove pending unnecessary ones.


packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-dynamic.ts line 409 at r1 (raw file):

   */
  // GV Layers Refactoring - Obsolete (in layers)
  protected override getFeatureInfoAtPixel(location: Pixel, layerPath: string): Promise<TypeFeatureInfoEntry[] | undefined | null> {

Should we remove ALL the methods flagged with Obsolete (in layers) functions from all the "old geoview-layers" classes as part of this PR or are we waiting?

There are a whole lot of those.


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

    if (!layerEntryIsGroupLayer(layerConfig)) {
      // Get the layer
      const layer = this.getGeoviewLayer(layerConfig.layerPath) as AbstractGeoViewLayer | AbstractGVLayer;

Should probably remove AbstractGeoViewLayer here


packages/geoview-core/src/geo/layer/layer-sets/feature-info-layer-set.ts line 137 at r1 (raw file):

      // If layer was found
      if (layer && (layer instanceof AbstractGeoViewLayer || layer instanceof AbstractGVLayer)) {

(like here)

Copy link
Member Author

@jolevesq jolevesq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 38 files reviewed, 6 unresolved discussions (waiting on @Alex-NRCan)


packages/geoview-core/src/api/event-processors/event-processor-children/time-slider-event-processor.ts line 131 at r1 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Ici, on peut enlever la redondance

Done.


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

Previously, Alex-NRCan (Alex) wrote…

Should probably remove AbstractGeoViewLayer here

Done.


packages/geoview-core/src/geo/layer/geoview-layers/raster/esri-dynamic.ts line 409 at r1 (raw file):

Previously, Alex-NRCan (Alex) wrote…

Should we remove ALL the methods flagged with Obsolete (in layers) functions from all the "old geoview-layers" classes as part of this PR or are we waiting?

There are a whole lot of those.

Yes, this draft PR was just the first step. I will do part of another commit


packages/geoview-core/src/geo/layer/geoview-layers/vector/abstract-geoview-vector.ts line 337 at r1 (raw file):

Previously, Alex-NRCan (Alex) wrote…

There may be some additional enhancements to be done now that we can notice how emitLayerRequesting and emitLayerCreation are getting "close" to each other. Like merging them into one maybe? Food for thought.

Added a comment

@jolevesq jolevesq force-pushed the 2617-hybrid branch 2 times, most recently from 71a54c5 to d97922f Compare December 2, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REFACTOR] Swap to Hybrid mode
2 participants