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

mapp.layer.styleParser() method #1132

Merged
merged 139 commits into from
Jun 12, 2024
Merged

Conversation

dbauszus-glx
Copy link
Member

@dbauszus-glx dbauszus-glx commented Feb 13, 2024

Theme methods require an excessive amount of object merging.

This can be significantly reduced by processing the style object and its themes when a layer is decorated.

mapp.layer.styleParser() method processes all themes on a layer and merges the default style into the category style.

Other issues such as missing style / icon definitions can be fixed within this process method.

This will also allow us to update the style object definition without breaking backwards compatibility.


@dbauszus-glx dbauszus-glx changed the title mapp.layer.themeStyle() method mapp.layer.styleParser() method Feb 14, 2024
@dbauszus-glx dbauszus-glx marked this pull request as draft February 26, 2024 10:49
@dbauszus-glx dbauszus-glx marked this pull request as ready for review March 4, 2024 19:37
@dbauszus-glx
Copy link
Member Author

dbauszus-glx commented Mar 5, 2024

A layer.styleParser method has been introduced.

The styleParser is called from the vector and mvt format methods and checks the layer.style configuration before the layer format method is called. Warnings in regards to the layer style have been removed from the decorator method and added to the styleParser method. Each theme category is parsed and the style keys are moved into a style object. Each style object is then parsed and the icon methods are moved into an icon object. Style and icon arrays are assumed to be valid and are not parsed as style arrays are not merged with the default style.

The featureFields method erroneously checks for ownProperty instead of hasOwn for the theme distribution.

The method to create feature styles has been renamed to layer.featureStyle [from layer.Style]. The featureStyle method returns a function which returns a style for each feature to the OL layer render.

A feature is processed in following order by the featureStyle method.

  1. Check whether the style is cached as Style object.
  2. Assign feature properties, ie. from a featureObject or featureLookup.
  3. Check whether a style filter is applied.
  4. Assign the geometryType. To be reviewed whether necessary.
  5. Assign theme style or default style.
  6. Assign cluster style.
  7. Assign highlight style.
  8. Assign label style.
  9. Assign selected style.

Theme styles are parsed and must be assigned without the need to clone style objects.

The utils.style method assigns a style zIndex to vector styles. This prevents the highlight style being rendered in between.

The wkt template must check for the fields length.

The addLayer method assigns a default zIndex if not specified.

Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

Found a configuration that's not working. I tested this is not working as you can see in the value 0 category wrapping in icon:{} correctly colours the points. But just style:{} should work.

 "style": {
    "default": {
      "icon": {
        "type": "target",
        "fillColor": "#939496"
      }
    },
    "cluster": {
      "icon": {
        "type": "target",
        "fillColor": "#454545"
      },
      "clusterScale": 5
    },
    "themes": {
      "theme": {
        "type": "graduated",
        "field": "field",
        "graduated_breaks": "less_than",
        "cat_arr": [
          {
            "value": 0,
            "label": "No Data",
            "style": {
              "icon": {
                "fillColor": "#939496"
              }
            }
          },
          {
            "value": 20,
            "label": "<20",
            "style": {
              "fillColor": "#f46d43"
            }
          },
          {
            "value": 25,
            "label": "20-25",
            "style": {
              "fillColor": "#fdae61"
            }
          },
          {
            "value": 30,
            "label": "25-30",
            "style": {
              "fillColor": "#fee08b"
            }
          },
          {
            "value": 35,
            "label": "30-35",
            "style": {
              "fillColor": "#ffffbf"
            }
          },
          {
            "value": 40,
            "label": "35-40",
            "style": {
              "fillColor": "#d9ef8b"
            }
          },
          {
            "value": 45,
            "label": "40-45",
            "style": {
              "fillColor": "#a6d96a"
            }
          },
          {
            "value": 9999,
            "label": "45+",
            "style": {
              "fillColor": "#1a9850"
            }
          }
        ]
      }
    }
  }

@dbauszus-glx
Copy link
Member Author

@simon-leech There isn't much we can do about this.

Here is nothing which indicates that this is an icon.

"style": {
  "fillColor": "#1a9850"
}

The only option we have is to warn if the default style has an icon and the class style has no icon at the end of the styleObject method.

    // Assign default icon if no cat style icon could be created.
    if (!cat.style.icon && defaultStyle.icon) {

      console.warn(`Layer:${layer.key}, Failed to evaluate icon: ${JSON.stringify(cat)}. Default icon will be assigned.`)

      cat.style.icon = defaultStyle.icon
    }

@simon-leech
Copy link
Contributor

Do themes categories not inherit from the default style anymore?
The default style is this

 "default": {
          "strokeWidth": 0.2,
          "strokeColor": "#000000",
          "fillColor": "#d9d9d9",
          "fillOpacity": 0.8
        }

If i apply a style.theme of this -

 "Population": {
        "title": "Population",
        "type": "graduated",
        "field": "population",
        "graduated_breaks": "greater_than",
        "cat_arr": [
          {
            "value": 0,
            "label": "0 to 4,000,000",
            "style": {
              "fillColor": "#edf8fb"
            }
          },
          {
            "value": 4000000,
            "label": "4,000,000 to 6,000,000",
            "style": {
              "fillColor": "#b2e2e2"
            }
          },
          {
            "value": 6000000,
            "label": "6,000,000 to 7,000,000",
            "style": {
              "fillColor": "#66c2a4"
            }
          },
          {
            "value": 7000000,
            "label": "7,000,000 to 9,000,000",
            "style": {
              "fillColor": "#2ca25f"
            }
          },
          {
            "value": 9000000,
            "label": "9,000,000 and over",
            "style": {
              "fillColor": "#006d2c"
            }
          }
        ]
      }

All the geometries loaded on the map have no opacity

@simon-leech
Copy link
Contributor

simon-leech commented May 24, 2024

Thanks @dbauszus-glx - that's fixed the issue with the merging of styles to Default.

I'm having an issue again with the basic style now.

example config - this shows nothing on the map

    "default": {
      "strokeColor": "#ef5350",
      "strokeWidth": 2,
      "fillOpacity": 0.1,
      "fillColor": "#FFFFFF"
    },
    "highlight": {
      "strokeWidth": 3,
      "strokeColor": "#87919E"
    },
    "themes": {
      "Outline": {
        "type": "basic",
        "title": "Outline",
        "style": {}
      }

@dbauszus-glx
Copy link
Member Author

Thanks @dbauszus-glx - that's fixed the issue with the merging of styles to Default.

I'm having an issue again with the basic style now.

example config - this shows nothing on the map

    "default": {
      "strokeColor": "#ef5350",
      "strokeWidth": 2,
      "fillOpacity": 0.1,
      "fillColor": "#FFFFFF"
    },
    "highlight": {
      "strokeWidth": 3,
      "strokeColor": "#87919E"
    },
    "themes": {
      "Outline": {
        "type": "basic",
        "title": "Outline",
        "style": {}
      }

@simon-leech should be fixed in this commit. 4bed91d

@dbauszus-glx dbauszus-glx self-assigned this May 31, 2024
Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

I noticed a bug on this PR but its not isolated to this PR so edited this comment and attaching the issue that should be looked at in a separate PR -
#1311

Marked my review as stale as this isn't a change we should apply on this PR but rather a separate one.

@simon-leech simon-leech self-requested a review June 8, 2024 17:33
Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

I'm happy with this one now! 🥳🥳

Tested across a range of instances and I see no missing styles anymore - warnings are clear and style themes merge with default styling.

Where the style cannot be parsed, we correctly warn and use a default.

Looks good to me! 🥳

Copy link

sonarcloud bot commented Jun 12, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@dbauszus-glx dbauszus-glx merged commit f0f4742 into GEOLYTIX:main Jun 12, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Issues related to the code structure and performance. Feature New feature requests or changes to the behaviour or look of existing application features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

style.style styles assignment & merge (styleParser)
5 participants