search

r-spatial / mapedit

Interactive editing of spatial data in R
https://www.r-spatial.org/r/2019/03/31/mapedit_leafpm.html
Other
218 stars 33 forks source link

add event listener for all_features and add it to the featurelist #98

Closed Gutschlhofer closed 4 years ago

Gutschlhofer commented 5 years ago

There is currently no way to get all polygons of the group used for editing (incl. the ones not touched by the user). My use-case was a shiny app that displayed the user a set of geometries that could be edited but didn't have to be.

Say I give the user 5 geometries, she/he edits 2, deletes 1, and adds 1. So far I could only get those that he touched (so the 2 edited, 1 deleted and 1 added) but not the others (remaining 2 geometries).

After the user is happy with his edit, I wanted to get all geometries currently displayed to the user in the edit-group. With the new addition, this is now possible with crud()$all.

timelyportfolio commented 5 years ago

@Gutschlhofer, I certainly see some sense in the pull request and I really appreciate the effort.

I would like to think all the way through this to make sure we don't cause any confusion. One small problem is that leafpm does not provide (see lines) an all event, since leafpm multiple layer support makes this much more difficult. I guess for now we could just leave blank for leafpm.

For the downstream mapedit functions, I don't think this addition causes any problems

Gutschlhofer commented 5 years ago

@timelyportfolio, after your reply I played around with if-clauses to exclude it for leafpm, but I think it makes more sense to just keep it as is and return NULL to leafpm users, that way leaflet.extras users will have the nice benefit of this and others won't be disturbed in any way.

timelyportfolio commented 5 years ago

great @Gutschlhofer I plan to review this and other pull requests next week. I really appreciate your efforts.

Gutschlhofer commented 5 years ago

That's great to hear! I just stumbled across editor == "leafpm" and changed it to editor[1] == "leafpm" to avoid warnings.

tim-salabim commented 5 years ago

Shouldn't this usually be matched right at the beginning of the function, as in editor = match.arg(editor) ?

Gutschlhofer commented 5 years ago

@tim-salabim thanks for pointing that out! I only now saw that https://github.com/r-spatial/mapedit/pull/100 reworks the editor-logic and also introduces the match.arg(editor) so I reverted back to my original commit.

timelyportfolio commented 5 years ago

So sorry this has taken so long. Going to merge into a new develop branch with #97 and #100 and after a short review period merge to master and CRAN over the next couple of days. Thanks so much for contributing. If ok, I will add you to DESCRIPTION as contributor.

@tim-salabim any other comments?

timelyportfolio commented 4 years ago

@Gutschlhofer merged in #107. I really appreciate your contribution. Thanks. Please let us know if you have any other ideas for improvement.