[PATCH] support undeletion without re-creating primitive with new id

[Upliner]

While developing the reverter plugin I've encountered some problems with undeletion. First, undeleted objects treated as "modified" but they should be handled as added: they should be uploaded in specific order(first nodes, then ways, then relations). It's not a big problem, I've just added UploadHook that sorts modified objects before uploading. Second problem is more serious. When user undeletes some objects, then deletes undeleted objects, then uploads the dataset, JOSM sends delete command for primitives that are already deleted on the server. Currently I use a dirty hack to workaround this problem which may work wrongly in some cases.

I wrote a patch for JOSM core which will allow reverter to work without any hooks and hacks.

PS: I don't want to hear about re-creation of undeleted primitives with new ID. This will break objects history and may mislead mappers. If OSM API supports undeletion without changing the ID, we should use it.

[Upliner]

patch for support of undeleted objects

[stoecker]

Looks clean to me. Did you test for side effects? I don't know that part of code in detail.

[Upliner]

Replying to stoecker:

Looks clean to me. Did you test for side effects?

I've done some tests of reverter functionality. Anyway this patch only affects primitives that have "visible=false" attribute. They can appear only on undeletion or on deletion conflicts. I tried to test this kind of conflicts and hadn't revealed any side-effects too because conflict system either completely purges the conflicted primitive or re-creates it with new id.

I don't know that part of code in detail.

It's sad. I want to know more about reasons that affect handling of "invisible" primitives in JOSM. Maybe it should be discussed in josm-dev list?

[stoecker]

You can try, but this code was made by Gubaer and he seems to be no longer active. Probably you're the expert now :-) Congratulations.

I will checkin code later this week. I still have the chance to make 3333 a tested ;-)

[Upliner]

patch for support of undeleted objects

[Upliner]

You flatter me :-)

I had done some more testing and revealed another one issue: If one undeletes some nodes, draws a way through these nodes and tries to upload it, "precondition failed" error will appear. Sorting modified objects doesn't help here, more complicated solution is needed.

However, this patch is better than nothing. I've updated the patch to support UploadSelected action. For more details you can see my git branch: ​http://github.com/Upliner/josm/commits/undelete

[Upliner]

more robust solution

[Upliner]

Unfortunately I was too quick with previous patches. They're most likely harmless but doesn't solve all problems with undeletion. I hope that new solution will be final. I've created a gist with comments for all changes: ​http://gist.github.com/440399

[bastiK]

One thing: If you have a visible deleted object and set deleted to false, it is marked as not modified. So either we have to exclude this case by raising an exception or remember the modified state before deletion, right?

[Upliner]

Replying to bastiK:

One thing: If you have a visible deleted object and set deleted to false, it is marked as not modified. So either we have to exclude this case by raising an exception or remember the modified state before deletion, right?

This behavor is present in current JOSM and I don't know any bugs related to it...

[bastiK]

Sure, your patch doesn't make it worse. Just when you read xor in code it looks like someone has given it some thought...

Then you think yourself and it is strange... :)

[Upliner]

OK, we can replace it to something like that

if (!isVisible()) setModified(!deleted)
if (isVisible() && deleted) setModified(true);

I don't think that solutions with exceptions or state remembering will be better. State remembering is work for undo/redo mechanism; those who call setDeleted(false) should know what they're doing.

[stoecker]

(In [3336]) #close #5135 - allow undeleting without recreating object - patch by Upliner

[bastiK]

see #5191. Is it related?

[Upliner]

Replying to bastiK:

see #5191. Is it related?

I don't think so because referred deleted primitive is new, so it cannot be invisible or undeleted...

[bastiK]

Ok.