Modify

Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#5135 closed enhancement (fixed)

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

Reported by: Upliner Owned by: team
Priority: normal Milestone:
Component: Core Version:
Keywords: Cc:

Description

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.

Attachments (3)

undelete.diff (6.4 KB) - added by Upliner 10 years ago.
patch for support of undeleted objects
undelete.2.diff (9.5 KB) - added by Upliner 10 years ago.
patch for support of undeleted objects
undelete.3.diff (18.0 KB) - added by Upliner 10 years ago.
more robust solution

Download all attachments as: .zip

Change History (16)

Changed 10 years ago by Upliner

Attachment: undelete.diff added

patch for support of undeleted objects

comment:1 Changed 10 years ago by stoecker

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

comment:2 in reply to:  1 Changed 10 years ago by 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?

comment:3 Changed 10 years ago by 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 ;-)

Changed 10 years ago by Upliner

Attachment: undelete.2.diff added

patch for support of undeleted objects

comment:4 Changed 10 years ago by 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

Changed 10 years ago by Upliner

Attachment: undelete.3.diff added

more robust solution

comment:5 Changed 10 years ago by 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

comment:6 Changed 10 years ago by 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?

comment:7 in reply to:  6 Changed 10 years ago by 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...

comment:8 Changed 10 years ago by 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... :)

comment:9 Changed 10 years ago by 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.

comment:10 Changed 9 years ago by stoecker

Resolution: fixed
Status: newclosed

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

comment:11 Changed 9 years ago by bastiK

see #5191. Is it related?

comment:12 in reply to:  11 Changed 9 years ago by 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...

comment:13 Changed 9 years ago by bastiK

Ok.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.