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

ol.parser.GeoJSON: identifier is dropped #724

Closed
wants to merge 1 commit into from

Conversation

fredj
Copy link
Member

@fredj fredj commented May 22, 2013

@bartvde
Copy link
Member

bartvde commented May 22, 2013

LGTM @fredj I need to do the same for other parsers
Please merge if Travis says it's okay.

@fredj
Copy link
Member Author

fredj commented May 22, 2013

Forgot to mention that if the properties already contains an id it will be overwritten.

@bartvde
Copy link
Member

bartvde commented May 22, 2013

This makes me wonder if we should have a fid again like in OL2, which was separate from the attributes.
@fredj what do you think?

@ahocevar
Copy link
Member

We do not need a distinction between internal id and fid, because we use goog.getUid for the internal id.

@bartvde
Copy link
Member

bartvde commented May 23, 2013

hey @ahocevar I was talking about a distinction between an attribute named 'id' and the fid (I am not 100% sure if e.g. GML allows this but I think it does).

If I understand @fredj correctly with this approach the fid would override any property named 'id' on the feature already.

Or should we map fid to the internal id instead?

@ahocevar
Copy link
Member

Mapping the feature id to the internal id would at least be awkward, if not
impossible.

Storing the fid (or id, not sure what the better name would be) as a member
(without a getter) would be fine I think.

Either way, we will need the server's feature id if we use a tile based bbox strategy - for keeping track of edits and for fetching and rendering data from non tile aware feature servers, so we can keep track of features that we already have in the client side store.

@ahocevar
Copy link
Member

Looking into this now. Maybe we can do something similar to what we did with the default geometry.

@ahocevar
Copy link
Member

@bartvde, @fredj: Would #733 be an acceptable alternative approach?

@bartvde
Copy link
Member

bartvde commented May 23, 2013

I like this approach @ahocevar

Sent from my iPhone

On May 23, 2013, at 6:17 PM, ahocevar notifications@github.com wrote:

@bartvde, @fredj: Would #733 be an acceptable alternative approach?


Reply to this email directly or view it on GitHub.

@fredj
Copy link
Member Author

fredj commented May 24, 2013

agree that #733 is a better approach

@fredj fredj closed this May 24, 2013
@fredj fredj deleted the geojson-id branch May 24, 2013 06:58
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.

3 participants