[Patch] Automatically refresh relation editor on external changes

[Woazboat]

Relation data in currently open relation editor windows is automatically reloaded when the relation is modified outside of the relation editor and there are no pending changes in the editor. If the relation has already been modified in the editor by the user and there are pending changes, display a warning notification instead that a conflict resolution will be required.

​https://github.com/JOSM/josm/pull/88
​https://patch-diff.githubusercontent.com/raw/JOSM/josm/pull/88.patch

[Woazboat]

Related to #12410 #12411 #15371 #16112

[taylor.smock]

A couple of comments from my perspective (feel free to disagree):

• I think that isDirtyRelation() should have a default implementation of isDirtyRelation(false). You could go the other way (isDirtyRelation(boolean) defaults to calling isDirtyRelation(), which is probably better from an backward-looking standpoint, but it doesn't look like any plugins in SVN/JOSM GitHub use it).
• You need to update GenericRelationEditorTest.java​ with the new method, if you aren't going to add a default implementation for isDirtyRelation(boolean).
• Tests verifying the correctness of the logic would be nice. Unless we want someone to accidentally break it in the future. For this, you'd probably want to use JOptionPaneSimpleMocker​.

[Woazboat]

I added the default implementation for isDirtyRelation() and updated the test. Adding a unit test for the new refresh logic would be a bit more involved and also require some refactoring of the existing code as far as I can tell.

[stoecker]

In 19398/josm:

Auto relation refresh, fix #21840 - patch by Woazboat

[stoecker]

In 19402/josm:

fix boolean logic broken in r19398, see #21840

[GerdP]

When I revert r19402 I don't see this bug. What was the reason to change the logic?

[GerdP]

Sorry, I changed the wrong ticket. See #24423 for a major regression.

[GerdP]

With the change in r19402 the method isDirtyRelation() returns false in far too many cases.
So far I don't see any need for this complex check. Why is field dataset relevant?
Possible patch:

src/org/openstreetmap/josm/gui/dialogs/relation/RelationEditor.java

@@ -152 +152 @@
 
     @Override
     public final boolean isDirtyRelation(boolean ignoreUninterestingTags) {
-        if (relation == null || relation.getDataSet() == null ||
-            relationSnapshot == null || relationSnapshot.getDataSet() == null) {
-            return false;
-        }
-
-        return !relation.hasEqualSemanticAttributes(relationSnapshot, ignoreUninterestingTags);
+        return relation != null && !relation.hasEqualSemanticAttributes(relationSnapshot, ignoreUninterestingTags);
     }
 
     /* ----------------------------------------------------------------------- */

[GerdP]

My patch makes problems when the relation editor is really dirty. This needs more thinking.

[GerdP]

Seems I didn't recognize the new Notification Relation modified outside of relation editor with pending changes. Conflict resolution required.
Up to now I could only find problem with the code in the older version r19398: if relation is null the code in isDirtyRelation() would crash. I assume that was the reason to change the logic.
I'll post the patch again in ticket #24423.

[GerdP]

See #24444 for a for regression caused by my patch :(

Would it be an option to revert all changes? This automatical refresh is somehow against the basic concept of the relation editor.

[anonymous]

Sure, that's an option. But it also means no progress. Rather try fixing it?

[GerdP]

I didn't find a simple fix. My approach would include a new status flag in the editor window which is set to true when the user clicks on Save or OK and later (when exactly?) restored to false. This would allow to distinguish wether the last change command was added inside or outside the editor when this notification is produced.

[stoecker]

Sorry. Forgot login. Last was me.

[stoecker]

Go ahead. This isn't simple, so probably a solution isn't simple as well...

[GerdP]

see #24444. Rather a hack than a solution.

[GerdP]

I don't know what's better. My workaround in 24444.patch or a complete revert of the changes for #21840.
We have to do something before the next release because current version is always warning and tested version is corrupting data.