-
Notifications
You must be signed in to change notification settings - Fork 394
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
#9263 geoprocessing tools #9316
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I wasn't able to perform intersection
- If I select a raster as source (maybe I shouldn't for certain processes) and i try to select a feature, it gives me successful notification
select_feature_raster_gpt.webm
- If I select a vector layer (e.g. on of the ones generated by 'buffer') I've got this error in console.
geoProcessingTools.js:140 TypeError: Cannot read properties of undefined (reading 'protocol')
at isURLSameOrigin (isURLSameOrigin.js:57:24)
at dispatchXhrRequest (xhr.js:145:50)
at new Promise (<anonymous>)
at xhrAdapter (xhr.js:15:10)
at dispatchRequest (dispatchRequest.js:58:10)
- Sometimes under certain selections, run do not re-enable after errors.
gpt_error_run.webm
- I wasn't able to perform one intersection that works. How to do it?
@MV88 Please do a quick check of the files before to submit the review. Some of the wrong commit content can be identified quiclky by you, but it may be difficoult for the reviewer, and some may not be so clear that are not needed.
@@ -55,6 +55,20 @@ export const updateOptionsByOwner = (owner, options) => { | |||
options | |||
}; | |||
}; | |||
/** | |||
* merge options of additional layers selected by owner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mergeOptionsByOwner
is a little obscure as usage and name. The action looks to update every additional layer with the same owner. like updateOptionsByOwner
.
I think this should be implementaed as an option of updateOptionsByOwner
.
Add a 3rd optional argument to updateOptionsByOwner
, named mode
. By default it is 'override'. (similar to the actionType
of other actions here). Other option is merge
. If merge
, it will merge the existing options. if override
it will work as it works now.
Correct me if I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should check the implementation of updateOptionsByOwner,
mergeOptionsByOwner goes in override and not in replace
I can refactor it if you want, but i feel it is more clear to use a different action
too many parameters and multi purpose function complicates things as well
"warningTitle": "Warning", | ||
"warningBody": "You have not selected a feature and this may slow down performances in geoserver. Do you want to continue?", | ||
"warningConfirmText": "Yes", | ||
"warningCancel": "Cancel", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not tooltips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes in ConfirmModal of Main comp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean they are not tooltips, they are texts for buttons, title and body of a modal, but they are under "tooltips": section of the json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i missed this. going to fix it
wps_intersection_log_error.log This is caused by an issue in fetching the feature collection of the highroad which is a big MultiLineString here is a request to test it over gs-stable
@tdipisa will this need a JIRA? all the rest is addressed or there is response in the comment |
@offtherailz to test intersection you can add this data attached to a postgis JNDI layer to the geoserver. CREATE TABLE
global_polygons
(
id SERIAL NOT NULL,
NAME CHARACTER VARYING(64),
the_geom geography,
PRIMARY KEY (id)
);
INSERT INTO global_polygons (id, name, the_geom) VALUES (1, 'test polygon', '0103000020E61000000100000005000000805FC9B96FAFF93F6427F84321024740805FC9B96FAFF93F5419470E8FCF4540D04562E3829B23405419470E8FCF4540D04562E3829B23406427F84321024740805FC9B96FAFF93F6427F84321024740');
INSERT INTO global_polygons (id, name, the_geom) VALUES (2, 'small polygon', '0103000020E6100000010000000500000080D36B9B5AFA2440948BFE78CE46464060A577E8C7AD1140948BFE78CE46464060A577E8C7AD1140802EC1510123464080D36B9B5AFA2440802EC1510123464080D36B9B5AFA2440948BFE78CE464640');
|
@MV88 gs-stable follows regularly the GS release workflow, that means it will be updated to a new GS stable version as soon as it is available. In the meantime we can test with our gs-main. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that using this layer
clevelandmetro:osm_extract_cleveland_optimized
from gs-stable, that contains > 60000 features, the tool becomes unusable. Maybe we should add a maxItems from the feature selector's request, or paginate it.
The rest looks ok.
I agree @MV88, that's usually important to manage. The pagination is better. |
@tdipisa @offtherailz consider that when i pick the feature clicking on map it must be part of the list, additionally we repopulate the list if the user has cleared selection demo.using.pagination.and.select.features.mp4 |
@ElenaGallo please test in DEV |
@MV88 please backport this to the stable branch. Thanks |
Description
this PR implements the buffer and intersection tool. for more details see the issue
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x", remove the others)
Issue
What is the current behavior?
What is the new behavior?
see issue description for more details
Breaking change
Does this PR introduce a breaking change? (check one with "x", remove the other)
Other useful information
Testing information
It have to be tested against gs-main because of this #9316 (comment)