fix duplicate objects after restoring data from localStorage

[tyrasd]

Attempt to fix #2134 (and possibly also #2091 and #1249).

This is not yet 100% finished, but can already be reviewed. Todos:

• fix failing restores from v2 JSON (modification) test
• add tests for restores from v3 JSON
• undo v -> version refactoring

[jfirebaugh]

Thanks for digging into this Martin. Can you explain what the problem is and the strategy this PR takes for fixing it?

[tyrasd]

OK. We have the following situation:

One edits in region A modifying object X (version v -> v+1) then closes the browser tab and later continues to edit in region B, using the restore tool.

After the fromJSON() the history stack looks like the following: stack[0] contains the data loaded from the API for region B, stack[1-…] contain the restored edits, including object X in version v+1. When iD later calculates the difference() between the head and the base (which is stack[0]), object X gets interpreted as a newly created one. The produced osmChange looks like the following:

<osmChange version="0.3" generator="iD">
  <create>
    <way id="X" version="v" changeset="…">…</way>
  </create>
</osmChange>

Note the non-negative id X of the "created" way and that v > 1. The response from the OSM API is something like:

<way old_id="X" new_id="…some new id…" new_version="1"/>

To solve this, I extended the toJSON() method to save also the original versions of all modified objects in the localStorage JSON (field baseEntities), which get rebased into the initially loaded data during the restore action.

[tyrasd]

This line contained a typo (first parenthesis should close right after 'saved_history').
But then this would be quite contra-productive if the browser crashes right after restoring the data (before uploading or another change).
So, I simply dropped it.

[tyrasd]

I've also refactored the Entity class to use the version field instead of the v, because the version is already present from the API calls and it doesn't really make sense to maintain two different sets of entity versions. This should lead to osmChange files that have more meaningful version parameters (although the API apparently doesn't care about them anyway):

<osmChange version="0.3" generator="iD">
  <modify>
    <way id="X" version="v+1" changeset="…">…</way>
  </modify>
</osmChange>

(note the v+1 instead of simply v as above)

[tyrasd]

OK, the PR should be complete now.

[jfirebaugh]

version and v serve two different purposes. version is the OSM version, v is an internal identifier that is incremented on when the entity undergoes a modification; it's used to avoid updating D3 selection elements whose corresponding datum has not changed. Please revert that portion of the commit.

[tyrasd]

Oh, I see. (should have looked more at the inline documentation ^^).
It's reverted now.

[jfirebaugh]

Ok, it took me a while to digest everything that was going on, but I think I have a good understanding now, and this approach looks sound. I see a couple adjustments to make:

• A similar fix is needed for deletions -- the base version should be recorded and restored. Right now if you restore in a different area the deletion disappears. If you remove the history.merge([iD.Node({id: 'n1'})]); line from the deletion test (which should be done), you'll see it fail.
• When rebasing, the tree needs to be rebased as well.

[tyrasd]

A similar fix is needed for deletions

True: efd3223