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

Preserve annotation ids when reloading geojson. #747

Merged
merged 3 commits into from
Nov 16, 2017

Conversation

manthey
Copy link
Contributor

@manthey manthey commented Nov 15, 2017

Fix some of the documentation.

Fix some of the documentation.
@manthey
Copy link
Contributor Author

manthey commented Nov 15, 2017

Prior to this, no effort was made to keep annotation IDs the same between generating geojson and setting them via geojson. This now tries to use the same IDs. As an example, go to this example. The point annotation's ID will be 1. On the current PR it will be 20.

@manthey manthey requested a review from jbeezley November 15, 2017 19:29
m_id = m_options.annotationId;
delete m_options.annotationId;
if (m_id === undefined || (m_options.layer && m_options.layer.annotationById(m_id))) {
annotationId += 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should emit a warning here if the "requested" id is ignored. Even better, perhaps if we are wanting to preserve id's across contexts, maybe we should just make them uuid's. I think the value is fairly opaque with the exception of the default title.

Also, the id attribute isn't documented. Maybe it's better to just keep it as a private interface due to the id conflict issues. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default title was my main motivation of using low integers. We could use uuids for the ids, change the accessor function from id to _id to indicate that it is private, and use a different counter for generating mostly distinct names.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay with leaving the switch to uuid until later. I think it would be good to issue a warning here to avoid confusion given how Emory is using these id's.

@manthey manthey merged commit db64d20 into master Nov 16, 2017
@manthey manthey deleted the preserve-annoation-id branch November 16, 2017 14:44
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.

2 participants