Feature/additional vector layer styling (#878) #953

perosoderstrom · 2021-12-13T11:29:51Z

Restores previous vector layer styling.
See issue #878

perosoderstrom · 2021-12-15T09:22:34Z

@jacobwod , @Hallbergs. This is blocking updating Kungsbackas public map. If possible, Kungsbacka would like to have this fix in 3.8 version.

Hallbergs · 2021-12-16T05:43:21Z

@jacobwod , @Hallbergs. This is blocking updating Kungsbackas public map. If possible, Kungsbacka would like to have this fix in 3.8 version.

Me, Jacob, and Per discussed this in a meeting yesterday. Some kind of solution will probably be presented in 3.8.

perosoderstrom · 2021-12-17T08:05:55Z

Let me know if there is anything I can fix.

jesade-vbg · 2022-01-14T08:48:57Z

new-admin/src/views/layerforms/vectorlayerform.jsx

@@ -133,6 +158,10 @@ class VectorLayerForm extends React.Component {
      return new Date().getTime().toString();
    }

+    function rgba_to_string(c) {


Please use camelCase for consistency. rgbaToString

jesade-vbg · 2022-01-14T08:49:06Z

new-admin/src/views/layerforms/vectorlayerform.jsx

@@ -144,6 +173,8 @@ class VectorLayerForm extends React.Component {
    if (fieldName === "date") value = create_date();


Please use camelCase for consistency. createDate

Hi @jesade-vbg,
Thanks for reviewing!
"create_date" is used in other *layerform.jsx and in search and edit.jsx so i suggest we leave it as is, for consinstency ;-).
I changed "rgba_to_string" to "rgbaToString" as you suggested in previous comment. Pushed it to github.

Great! Sorry I was'nt paying attention to where create_date was placed. Admin is a diffrent story.

jacobwod · 2022-01-20T07:47:39Z

I'll check it out with our current SLD styling locally, to ensure everything works out fine with current solutions too. (Hopefully later today.)

If so, it'll be ready to merge.

jacobwod · 2022-01-20T08:19:57Z

The solution crashes when no values are specified for the newly added keys. In my case, e.g. the icon is undefined:

Which leads to this:

Please note that this does not only apply to icon - all keys should be treaded the same.

You should ensure that all needed properties are set in config, before you try to create the custom style this.style = createStyle.apply(this);.

In addition, I don't see the point of having createStyle outside and doing this cumbersome .apply() call. Please place it as a usual class method.

jacobwod · 2022-01-23T18:33:48Z

Looks like a good commit @perosoderstrom! I'll take a closer look tomorrow.

jacobwod · 2022-01-25T09:23:34Z

Hi @perosoderstrom.

So the styling is correct if I supply SLD or point out an SLD file in Admin UI.

If I however remove them, I don't get the default OL-styling (the blue polygons), but rather something like this:

That's the case with empty settings (or as empty as they can be - I can't remove some of these…).

- Since createStyle is now a class method, there's no need to use .call() or .apply(). Changed how it's invoked. - Removed getStyle() as it was only called once, in the constructor. We can instead just use the value that resists in this.style. - Even better: there's no need to save the value in this.style, as it is only used in once place (again, the constructor). Hence, line 115 now is simplified. - As createStyle() is invoked only once and _without_ parameters, there is no need to have the parameters in method definition (see line 306). Removing them would lead to further simplification inside createStyle. - What's even more interesting is that we have a lot of unused code in createStyle. E.g. everything that has to do with 'feature' is never invoked, as we never provide any 'feature' to the method. - The entire comment with generateLegend() should be removed.

jacobwod

Apart from the behavior above, I've commited some changes (that don't fix the problem in my previous comment).

You can see the changes in VectorLayer.js:

Since createStyle is now a class method, there's no need to use .call() or .apply(). Changed how it's invoked.
Removed getStyle() as it was only called once, in the constructor. We can instead just use the value that resists in this.style.
Even better: there's no need to save the value in this.style, as it is only used in once place (again, the constructor). Hence, line 115 now is simplified.
As createStyle() is invoked only once and without parameters, there is no need to have the parameters in method definition (see line 306). Removing them would lead to further simplification inside createStyle.
What's even more interesting is that we have a lot of unused code in createStyle. E.g. everything that has to do with 'feature' is never invoked, as we never provide any 'feature' to the method.
The entire comment with generateLegend() should be removed.

perosoderstrom · 2022-01-28T07:32:27Z

Hi @jacobwod,
A question about vector colors.
lineColor: "rgba(0, 0, 0, 0.5)" was used as default for vector layers prior to the SLD styling was implemented. It was defined in new-admin/src/views/layermanager.jsx and new-admin/src/views/layerforms/vectorlayerform.jsx.
Shall we change default Hajk vector style to default OpenLayers style?

jacobwod · 2022-01-28T09:06:10Z

Shall we change default Hajk vector style to default OpenLayers style?

Hi @perosoderstrom. I would prefer the following chain of events:

const style = custom config from Admin UI || custom SLD from Admin UI  || no style at all (i.e. fallback to OpenLayers default)

perosoderstrom · 2022-01-28T09:24:02Z

I'm not quite sure what you mean with "custom config from Admin UI".

In current PR it is implemented as
const style = custom SLD from Admin UI || custom OpenLayers style from UI || no style, fall back to Hajk default

But instead of falling back to previous Hajk default it will use OpenLayers default. Will that be OK?

jacobwod · 2022-01-28T09:35:42Z

But instead of falling back to previous Hajk default it will use OpenLayers default. Will that be OK?

I think we talk about different "previous" versions here. To me, "previous" is the state it was in prior this PR. (While I think that you refer to the previous implementation of custom styling, that has been removed when I implemented styling with SLD, right?)

If so: yes, it's fine to just fall back to OpenLayers' defaults, if no custom nor SLD styling is set.

- Removed unused generateLegend code - Use OpenLayers default vector style if neither SLD nor OpenLayers style is specified in admin - Fixed icon scale (that has never worked)

jacobwod · 2022-03-01T09:28:14Z

@jesade-vbg What's your opinion on this:

Hajk/new-client/src/models/layers/VectorLayer.js

Lines 18 to 20 in 0ab97c5

    
           const fetchConfig = { 
        
             credentials: "same-origin", 
        
           };

Is it really needed when hfetch is used?

Hajk/new-client/src/models/layers/VectorLayer.js

Line 430 in 0ab97c5

hfetch(url, fetchConfig).then((response) => {

jesade-vbg · 2022-03-01T09:50:53Z

same-origin is the default behavior of hfetch, so the three lines can be removed.

jacobwod · 2022-03-01T10:13:40Z

I took a look at the code and corrected one minor thing (see previous comment).

Unfortunately, there's something going on in this branch that leads to total freeze upon app load. Please the video below where I compare develop (which has no lag) with this branch. The app just freezes for about 10 seconds:

Skarminspelning.2022-03-01.kl.11.05.53.mov

I'd be great if someone could confirm, ping @Hallbergs @jesade-vbg. I have two WFS layers in this map config, both styled by SLD. When I do the same test using a map config without WFS layers, there is no freeze. So it's clearly related to VectorLayer.

Update: happens on both Safari and Firefox.

jesade-vbg · 2022-03-01T11:13:11Z

It is probably these settings that are missing in map config. It does not fallback to correct values if not specified in config.

"altShiftDragRotate": true,
"onFocusOnly": false,
"doubleClickZoom": true,
"keyboard": true,
"mouseWheelZoom": true,
"shiftDragZoom": true,
"dragPan": true,
"pinchRotate": true,
"pinchZoom": true,
"zoomDelta": false,
"zoomDuration": false

jacobwod · 2022-03-01T12:02:22Z

It is probably these settings that are missing in map config. It does not fallback to correct values if not specified in config.

Thanks for pointing it out - it reminded my to merge latest develop into this branch.

Anyway, the problem persists. And I'm on the same map config as you can see in the video, only switching branches. The settings you refer are in AppModel.js, and that file is unchanged between this branch and develop.

perosoderstrom · 2022-03-01T13:57:29Z

Just tested with a fresh clone.
I added one vector layer and I can't see any freeze, without styling, with SLD styling or with OpenLayers styling.
The layer I tested was a point layer.

Hallbergs · 2022-03-01T14:03:33Z

We've located the issue. If the vector layer added contains alot of features, we block the thread when we try to parse them. See this line.

We could get around this by doing this in a worker, but we're not sure thats a valid solution since we're still fetching all the data just to create a legend graphic (we're talking several MBs of unneccesary data). Hmm...

Hallbergs · 2022-03-01T14:10:57Z

A possible soultion could be to do a DescribeFeatureType request instead maybe.

jacobwod · 2022-03-01T14:31:29Z

The problem is the legend generation. Unfortunately, it results in two separate problems:
A. A lot of data is loaded on app load (depending on the size of dataset of course, but potentially hundreds of thousands of objects)
B. The legend is generated even if separate SLD styling is supplied, which leads to discrepancy between what the features look like in map vs what the legend says they look like.

A. Everything is loaded

To generate the legend, the code does basically this:

gets all features
parses them
checks what we've got (points? lines? polygons?)
generates a symbol for each of the types found in step 3

This algorithm's weakness lies in the first point: the entire dataset is retrived from server. In your case, @perosoderstrom, where the dataset is small, you don't see the delay. In our case, where we get all buildings, step 1 leads to 13 Mb being downloaded, while step 2 has to parse 103605 features. That's what causes the freeze.

B. Generated legend differs from SLD styling

Since the algorithm for legend generation is invoked even when SLD styling is used, we can get "funny" results, like those in the screenshot below:

Hallbergs · 2022-03-01T14:40:29Z

At a second thought, DescribeFeatureType probably won't work since the type returned is Geometry in many cases (which tells us nothing).

perosoderstrom · 2022-03-02T08:59:16Z

A thought.
The vector features are first fetched in loadData, and then they are fetched again and parsed in generateLegend, where problem A occurs. It should be possible to use feature(s) from loadData in generateLegend.
(Note: In generateLegend only the first feature is used to get featureType = features[0].getGeometry().getType();)

jacobwod · 2022-03-02T14:06:30Z

(Note: In generateLegend only the first feature is used to get featureType = features[0].getGeometry().getType();)

Well, in that case the algoritm isn't particularly clever. A simple limit=1 to the WFS request would solve the risk of huge data loads. Still though, the generated legend can be inaccurate as it only cares about the first element in the dataset. I feel this confuses the user and leads to a worse product than without this function.

I propose that everything related to the generateLegend functionality is removed before the PR is merged. In order to ensure a proper legend, admins should generated a static image and point it (it's the icon property if I recall correctly).

- See discussion in #953. - Main take aways: a) it loads all features which can lead to enormous data transfers, just to get the first element; b) it parses all features, which can lead to hang-ups, but only cares for the first feature; c) it only generetes legend for the first feature which will lead to misleading legends for layers with mulitple geometry types.

jacobwod

Just pushed a change where legend generation is removed. This should be a good compromise for everyone.

persoderstrom added 3 commits December 10, 2021 14:57

Added ol styling of vector layers

254eed7

Merge branch 'develop' into feature/additional-vector-layer-styling

4e588a4

Fixed missing maxZoom

def40a7

jesade-vbg reviewed Jan 14, 2022

View reviewed changes

persoderstrom added 3 commits January 17, 2022 09:01

Changed from snake_case to camelCase: rgbaToString

c9aba66

Removed console.log

6e60117

Merge branch 'develop' into feature/additional-vector-layer-styling

b693f30

persoderstrom added 3 commits January 20, 2022 11:51

Set ol style properties to "" if not defined in config

f31d470

If ol styles are configured, create and set the styles

e75150f

Made createStyle a class method

f0da2ca

jacobwod requested changes Jan 25, 2022

View reviewed changes

persoderstrom added 4 commits January 30, 2022 15:44

Removed Hajks default vector style

ff8a90c

- Cleaned up createStyle

395d12e

- Removed unused generateLegend code - Use OpenLayers default vector style if neither SLD nor OpenLayers style is specified in admin - Fixed icon scale (that has never worked)

Restored legend

0ab97c5

Merge branch 'develop' into feature/additional-vector-layer-styling

c7e740f

perosoderstrom requested a review from jacobwod February 16, 2022 17:26

No need for supplying 'same-origin'

be48b88

Merge branch 'develop' into feature/additional-vector-layer-styling

5b9ad40

jacobwod approved these changes Mar 7, 2022

View reviewed changes

jacobwod merged commit 396d992 into develop Mar 7, 2022

jacobwod deleted the feature/additional-vector-layer-styling branch March 7, 2022 08:58

jacobwod linked an issue Mar 7, 2022 that may be closed by this pull request

Symbols for vector layers not working #878

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/additional vector layer styling (#878) #953

Feature/additional vector layer styling (#878) #953

perosoderstrom commented Dec 13, 2021

perosoderstrom commented Dec 15, 2021

Hallbergs commented Dec 16, 2021

perosoderstrom commented Dec 17, 2021

jesade-vbg Jan 14, 2022

jesade-vbg Jan 14, 2022

perosoderstrom Jan 17, 2022

jesade-vbg Jan 17, 2022

jacobwod commented Jan 20, 2022

jacobwod commented Jan 20, 2022 •

edited

Loading

jacobwod commented Jan 23, 2022

jacobwod commented Jan 25, 2022

jacobwod left a comment

perosoderstrom commented Jan 28, 2022

jacobwod commented Jan 28, 2022

perosoderstrom commented Jan 28, 2022

jacobwod commented Jan 28, 2022 •

edited

Loading

jacobwod commented Mar 1, 2022

jesade-vbg commented Mar 1, 2022

jacobwod commented Mar 1, 2022 •

edited

Loading

jesade-vbg commented Mar 1, 2022

jacobwod commented Mar 1, 2022 •

edited

Loading

perosoderstrom commented Mar 1, 2022 •

edited

Loading

Hallbergs commented Mar 1, 2022

Hallbergs commented Mar 1, 2022

jacobwod commented Mar 1, 2022 •

edited

Loading

Hallbergs commented Mar 1, 2022

perosoderstrom commented Mar 2, 2022

jacobwod commented Mar 2, 2022

jacobwod left a comment

		@@ -144,6 +173,8 @@ class VectorLayerForm extends React.Component {
		if (fieldName === "date") value = create_date();

Feature/additional vector layer styling (#878) #953

Feature/additional vector layer styling (#878) #953

Conversation

perosoderstrom commented Dec 13, 2021

perosoderstrom commented Dec 15, 2021

Hallbergs commented Dec 16, 2021

perosoderstrom commented Dec 17, 2021

jesade-vbg Jan 14, 2022

Choose a reason for hiding this comment

jesade-vbg Jan 14, 2022

Choose a reason for hiding this comment

perosoderstrom Jan 17, 2022

Choose a reason for hiding this comment

jesade-vbg Jan 17, 2022

Choose a reason for hiding this comment

jacobwod commented Jan 20, 2022

jacobwod commented Jan 20, 2022 • edited Loading

jacobwod commented Jan 23, 2022

jacobwod commented Jan 25, 2022

jacobwod left a comment

Choose a reason for hiding this comment

perosoderstrom commented Jan 28, 2022

jacobwod commented Jan 28, 2022

perosoderstrom commented Jan 28, 2022

jacobwod commented Jan 28, 2022 • edited Loading

jacobwod commented Mar 1, 2022

jesade-vbg commented Mar 1, 2022

jacobwod commented Mar 1, 2022 • edited Loading

jesade-vbg commented Mar 1, 2022

jacobwod commented Mar 1, 2022 • edited Loading

perosoderstrom commented Mar 1, 2022 • edited Loading

Hallbergs commented Mar 1, 2022

Hallbergs commented Mar 1, 2022

jacobwod commented Mar 1, 2022 • edited Loading

A. Everything is loaded

B. Generated legend differs from SLD styling

Hallbergs commented Mar 1, 2022

perosoderstrom commented Mar 2, 2022

jacobwod commented Mar 2, 2022

jacobwod left a comment

Choose a reason for hiding this comment

jacobwod commented Jan 20, 2022 •

edited

Loading

jacobwod commented Jan 28, 2022 •

edited

Loading

jacobwod commented Mar 1, 2022 •

edited

Loading

jacobwod commented Mar 1, 2022 •

edited

Loading

perosoderstrom commented Mar 1, 2022 •

edited

Loading

jacobwod commented Mar 1, 2022 •

edited

Loading