Modify

Opened 3 months ago

Closed 9 days ago

Last modified 8 days ago

#18596 closed defect (fixed)

[PATCH] Fix relation ordering after split-way

Reported by: JeroenHoek Owned by: team
Priority: normal Milestone: 20.03
Component: Core Version: latest
Keywords: Cc: Larry0ua, taylor.smock

Description (last modified by JeroenHoek)

In some cases the split-way command failed to detect the direction of ways part of a relation by breaking too soon from the loop that inspects relation members. This commit fixes this.

The types of relations affected are route, multipolygon, and boundary relations; e.g., relations containing ordered sets of ways.

Additionally, a warning is shown to the user if by splitting the way a relation may have been damaged. This warning is raised when all of these requirements are met:

  • the relation concerned is a route, multipolygon, or boundary relation,
  • has more than one member,
  • does not have all of its members downloaded, and
  • the direction could not be determined from the available members.

Background

This bug causes routes for public transport, hiking, and cycling to break when edits are performed on small parts of the map — e.g., when a mapper is working on details in a neighbourhood. It is common for such relations to have only a few members downloaded.

The split way command tries to determine the order of the new parts by looking at the available members, but due to a bug in the detection loop this can fail even when at least two connected members are available. This specific bug is hit when a way is split that is at one end of the downloaded members: in that case there is a 50% chance (depending on the position of the member in the relation) that the split will place the new parts in the wrong order. This commit solves this by not breaking from the detection loop too soon.

Attachments (14)

warning.png (110.8 KB) - added by JeroenHoek 3 months ago.
boundary-with-fix.png (99.5 KB) - added by JeroenHoek 2 months ago.
boundary-without-fix.png (63.4 KB) - added by JeroenHoek 2 months ago.
download-for-warning.png (152.1 KB) - added by JeroenHoek 2 months ago.
18596-sample.osm (163.2 KB) - added by GerdP 2 months ago.
18596-sample-larger.osm (213.9 KB) - added by GerdP 2 months ago.
18596-false-positive.osm.bz2 (5.0 KB) - added by GerdP 2 months ago.
0001-Fix-relation-ordering-after-split-way.2.patch (1 bytes) - added by JeroenHoek 7 weeks ago.
Mark attachment as empty
0002-Unit-tests-for-split-way.patch (1 bytes) - added by JeroenHoek 7 weeks ago.
Mark attachment as empty
testdata-bad.png (199.5 KB) - added by JeroenHoek 6 weeks ago.
testdata-good.png (265.9 KB) - added by JeroenHoek 6 weeks ago.
0001-Fix-relation-ordering-after-split-way.patch (274.1 KB) - added by JeroenHoek 6 weeks ago.
17400
translation-argument-missing.jpg (124.0 KB) - added by Hb--- 6 weeks ago.
josm_18596_false_positive.osm.bz2 (148.3 KB) - added by skyper 5 weeks ago.
example

Download all attachments as: .zip

Change History (113)

Changed 3 months ago by JeroenHoek

Attachment: warning.png added

comment:1 Changed 3 months ago by JeroenHoek

The warning is shown when only one member of a relation is downloaded (warning.png).

comment:2 in reply to:  1 Changed 3 months ago by skyper

Thanks.
Please, add type boundary relations.

You do not need to download all members only the neighbours, so, the message is wrong.
Did you test it with ways which are multiple times part of a relation ?
Did you have a look at #18018 ?
Did you test it with unordered relations ?

Replying to JeroenHoek:

The warning is shown when only one member of a relation is downloaded (warning.png).

I would prefer to have JOSM deal this case by downloading one (or two) more members to solve it automatically. At least offer to download with the warning or do it silently.

comment:3 Changed 3 months ago by anonymous

Please, add type boundary relations.

I can do that.

You do not need to download all members only the neighbours, so, the message is wrong.

JOSM doesn't have an option to download just the neighbours (although users can do this manually). The warning intends to point the user to the 'download all members' option in the context menu of a relation.

Did you test it with ways which are multiple times part of a relation ?

Yes. Looks fine.

Did you test it with unordered relations ?

What kind of behaviour do you expect? This patch does not appear to break them. The warning is only shown for ordered relations (routes, multipolygons).

Did you have a look at #18018 ?

I have tested it, and it looks to be a different kind of bug. I may look into it after this patch has landed, but it is not related to the issue I am fixing here.

I would prefer to have JOSM deal this case by downloading one (or two) more members to solve it automatically. At least offer to download with the warning or do it silently.

That is a good suggestion, but I would prefer to tackle that in a future patch in order to keep the scope of this patch limited.

comment:4 in reply to:  3 Changed 3 months ago by skyper

Replying to anonymous:

You do not need to download all members only the neighbours, so, the message is wrong.

JOSM doesn't have an option to download just the neighbours (although users can do this manually). The warning intends to point the user to the 'download all members' option in the context menu of a relation.

I you want the user to download hundreds of objects instead of two nodes with their parent ways, fine. Still the question between "incomplete" members or "all" members needs to be find.

Did you test it with unordered relations ?

What kind of behaviour do you expect? This patch does not appear to break them. The warning is only shown for ordered relations (routes, multipolygons).

Thought about relations which are broken already.

comment:5 Changed 3 months ago by JeroenHoek

Updated patch now also applies same strategy to boundary relations.

comment:6 Changed 2 months ago by JeroenHoek

Description: modified (diff)

comment:7 Changed 2 months ago by GerdP

Can you add an *.osm example that shows the problem, please?
I don't understand why boundary relations or multipolygons need special treatment. Order of members doesn't really matter.

comment:8 Changed 2 months ago by JeroenHoek

JOSM doesn't like it when the order is wrong. Compare these two screenshots for the same split done under the same conditions, with and without the fix (boundary-with-fix.png, boundary-without-fix.png).

The unit tests I added highlight this specific bug.

Changed 2 months ago by JeroenHoek

Attachment: boundary-with-fix.png added

Changed 2 months ago by JeroenHoek

Attachment: boundary-without-fix.png added

comment:9 Changed 2 months ago by JeroenHoek

There seems to be a strong agreement that the boundary sections are listed in their natural order in the relation. The same goes for multipolygons. With routes it is of course essential.

comment:10 Changed 2 months ago by GerdP

Sure, it makes no sense to change the order without reason. Makes it much more difficult to find out what was changed when you look at the history of a relation.
I tested the example for #18018 and the patch improves the situation.
What I don't have is a sample which prints the new warning message.

Changed 2 months ago by JeroenHoek

Attachment: download-for-warning.png added

comment:11 Changed 2 months ago by JeroenHoek

You can get the new warning by downloading any way that is part of a relation, but not any other members. The warning is shown when the direction of the way cannot be determined because its neighbours aren't loaded.

For example, download this bit (download-for-warning.png​), located here: https://www.openstreetmap.org/#map=19/53.20041/5.80800

Be careful to download only that one bit of way. The dowload view is centred around one if its (unconnected) nodes.

You will get the warning if you split that three node way in the middle.

comment:12 Changed 2 months ago by JeroenHoek

One of the new unit tests (oneMemberOrderedRelationShowsWarningTest) also triggers the warning.

Changed 2 months ago by GerdP

Attachment: 18596-sample.osm added

Changed 2 months ago by GerdP

Attachment: 18596-sample-larger.osm added

comment:13 Changed 2 months ago by GerdP

OK, I see the message with the atached 18596-sample.osm. I wondered why I have to download all the members of all the relations.
My approach to solve the problem would be to select the way, press 3 to "zoom to selection" and download the area, see 18596-sample-larger.osm. Of course this might fail if the way is very long.

comment:14 Changed 2 months ago by JeroenHoek

That would work, but it's probably easier to direct users to the download members context action of the relation(s).

A future patch might ask the user for permission to download the neighbouring members of the affected relations and fix it that way, but I want to limit the scope of this patch.

comment:15 Changed 2 months ago by GerdP

Well, the problem is that one might not know how to find the relations.
Another option would be to download the parents of one of the endpoints of the way. That seems to be all that is needed.

comment:16 Changed 2 months ago by JeroenHoek

Agreed, but I would like to tackle that in a follow-up patch after this one lands to keep the complexity of the patch down. I don't know if the split-way command should download things on its own or if the user must be consulted via a dialogue screen, and testing that functionality is more complex too.

This patch targets the bug I identified (with incomplete relations where one neighbouring relation is available, covered by unit test doIncompleteMembersOrderedRelationCorrectOrderTest), and additionally adds the warning we discussed.

comment:17 Changed 2 months ago by GerdP

OK, I understand. We have the "Update Multipolygon" action which automatically downloads incomplete members. On the other hand it seems that some JOSM maintainers don't like this automatical download. I am not one of them.
I'll look again at the patches tomorrow, it produces checkstyle warnings for the unit test.

comment:18 Changed 2 months ago by JeroenHoek

Sorry about the checkstyle warnings.

comment:19 in reply to:  17 Changed 2 months ago by stoecker

Replying to GerdP:

OK, I understand. We have the "Update Multipolygon" action which automatically downloads incomplete members. On the other hand it seems that some JOSM maintainers don't like this automatical download. I am not one of them.
I'll look again at the patches tomorrow, it produces checkstyle warnings for the unit test.

"Update Multipolygon" does not automatically download anything. It's a user triggered action. What I'm against is automatic software triggered remote actions, not anything which is caused by the user.

comment:20 Changed 2 months ago by GerdP

OK, so it would be OK to download the parents here when user pressed P to split and information is missing?

comment:21 Changed 2 months ago by GerdP

If not I would prefer to refuse the splitting instead of executing it and asking the user to undo next.
Instead I would show a notification with something like "Cannot split because information is missing. Download the parents of the end-nodes of the way".

comment:22 Changed 2 months ago by stoecker

Such a notice (probably together with a button to do so) is fine.

Something like "Cannot split without downloading missing information". And buttons "Continue and download now" and "abort."

I want to user to be in control, not the software. If the user chooses a "download and don't bother me next time" checkbox that's perfectly ok for me (we have that e.g. for the auto-plugin updates).

Last edited 2 months ago by stoecker (previous) (diff)

comment:23 Changed 2 months ago by GerdP

Sounds good to me. If user choses "Cancel and remember my choice" I suggest to show a notification the next time this situation occurs. In fact it would be good probably always good to have such a notification in the dialogs which have the "remember my choice" checkbox.

@JeroenHoek: Please let us know if you have time to implement this, else I'd use your patches as a base.

comment:24 Changed 2 months ago by JeroenHoek

I can have a look.

Could you review these two patches (fix and tests) in the mean time?

comment:25 Changed 2 months ago by GerdP

Thanks, yes, I'll review them.

Changed 2 months ago by GerdP

comment:26 Changed 2 months ago by GerdP

Load attached 18596-false-positive.osm.bz2 and search for id:344502450 OR id:344502451.Press P to split the inner ring of the multipolygon at nodes 344502450 and 344502451.

Click OK one the following dialog (Which way segment should reuse the history...).
Expected: Way is split without any notification.
Instead the notification for the MP pops up

I also don't like that this and other notifications stay so long, but that is probably no longer important.

comment:27 Changed 2 months ago by GerdP

Some code issues:

  • "Strings literals should be placed on the left side when checking for equality", so use "route".equals(type) instead of type.equals("route")
  • the unit tests have no javadoc, see e.g. OsmValidatorTest.testCleanupIgnoredErrorsTicket17837(). In HighwaysTest.testTicket14891() you can find an example that uses an *.osm file as input. This makes it easier to understand what the test is about as you can also load the test data into JOSM.
  • please use ant checkstyle pmd and fix the reported problems before providing a patch

comment:28 Changed 2 months ago by JeroenHoek

the unit tests have no javadoc

I just followed what the existing test did.

comment:29 Changed 2 months ago by skyper

Found one more relation type: turn-restiction
Especially with way(s) as via members, see #10808.

Last edited 2 months ago by skyper (previous) (diff)

comment:30 Changed 2 months ago by skyper

Ticket #8082 has been marked as a duplicate of this ticket.

comment:31 Changed 2 months ago by skyper

Cc: Larry0ua added

comment:32 Changed 2 months ago by skyper

See #17898 for thoughts in the same direction.

comment:33 Changed 7 weeks ago by JeroenHoek

I think I have the download-part working now. Next step is to add a dialogue to ask the user permission before doing that. I'll update the patches when I have that part working too.

I've read the discussion about getting the JOSM codebase into git. For what it's worth: SVN makes it hard for me to keep these patches up-to-date with the current release of JOSM. I'll probably run into problems with that later. I can only (warmly!) recommend switching to git.

Last edited 7 weeks ago by JeroenHoek (previous) (diff)

comment:34 Changed 7 weeks ago by JeroenHoek

GerdP: I've modified the detection loop to ignore self-closing ways (i.e., no downloading of missing members needed). That seems to handle 18596-false-positive.osm.bz2 properly.

comment:35 Changed 7 weeks ago by GerdP

Sounds good. Where is the patch?

comment:36 Changed 7 weeks ago by JeroenHoek

I've updated 0001. The 0002 patch is included in that (I can't seem to delete attachments).

This is a preview lacking better documentation and better unit tests. If this approach is workable I can improve on those points.

The dialog is also missing a better description.

The patch should now download members if the order of the new ways cannot be determined. To do this, the split way method is split up into three methods: analysis, downloading of missing members (if necessary, and if approved by the user), and the splitting itself.

I've tried to integrate the points mentioned above, please have a look if this is what we want.

comment:37 Changed 7 weeks ago by GerdP

The patch for JoinAreasAction doesn't apply cleanly.

comment:38 Changed 7 weeks ago by JeroenHoek

I'm not sure what should happen if the user cancels the proposed download action. I guess a warning? The patch now aborts the split action at that point.

comment:39 Changed 7 weeks ago by JeroenHoek

This should resolve the merge conflict.

comment:40 Changed 7 weeks ago by GerdP

Your changes to the interface are probably not compatible with some plugins. Several of them use SplitWayCommand.

comment:41 Changed 7 weeks ago by JeroenHoek

How's this?

comment:42 Changed 7 weeks ago by GerdP

Did not try it. Is the new return value Optional<SplitWayCommand> binary compatible with SplitWayCommand ?

Last edited 7 weeks ago by GerdP (previous) (diff)

comment:43 Changed 7 weeks ago by JeroenHoek

It's a Java Optional, but I have limited its use to doSplitWay now. That method was already documented with:

This method is only public for {@code SplitWayAction}.

(Not by me.)

So plugins should not use that method at all.

Last edited 7 weeks ago by JeroenHoek (previous) (diff)

comment:44 Changed 7 weeks ago by GerdP

I see e.g.

        SplitWayCommand result = SplitWayCommand.splitWay(way, wayChunks, Collections.emptyList(), strategy); 

in pt_assistant plugin.

comment:45 Changed 7 weeks ago by JeroenHoek

Yeah, I've updated the patch to leave that part of the interface as-is.

comment:46 Changed 7 weeks ago by GerdP

I see now. And I found no plugin which uses doSplitWay() in the JOSM repo.

ant checkstyle 

complains a lot.
I've just tried this with the latest version of the patch:
Download way 29355341 without parents and split it. I would have expected that the referring multipolygon relation is downloaded, but the way is split without any dialog or further download.

comment:47 Changed 7 weeks ago by JeroenHoek

Download way 29355341 without parents and split it. I would have expected that the referring multipolygon relation is downloaded, but the way is split without any dialog or further download.

I don't think that is related to the issue I am solving. Is this a regression from vanilla JOSM without this patch?

ant checkstyle complains a lot.

I will attempt to fix those if this approach works.

comment:48 in reply to:  47 Changed 7 weeks ago by GerdP

Replying to JeroenHoek:

Download way 29355341 without parents and split it. I would have expected that the referring multipolygon relation is downloaded, but the way is split without any dialog or further download.

I don't think that is related to the issue I am solving. Is this a regression from vanilla JOSM without this patch?

No, not a regression. But I saw new code to download members of a relation and wondered what your code does when the relation is not yet known.
I'll play with the fix to find out in what case it is an improvement when data is incomplete.

comment:49 Changed 7 weeks ago by JeroenHoek

This patch fixes a very common problem where mappers split a way containing route relations, but don't have enough relation members to deduce the order of the new parts in the relation. A quick glance at the map history tells me this happens weekly with bus and bicycle routes in the Netherlands (worldwide probably, but I map in the Netherlands), where some expert mappers use warning software to detect breakage caused by Id and Josm. Josm users tend to be more experienced mappers, and they (myself included) tend to be surprised that the relations break even though Josm didn't warn them.

This patch aims to prevent such breakage from Josm.

In Josm these edits usually start from the slippy map download screen, and thus all ways downloaded will contain all of their relations (but of course not all relation members).

I'm not a Josm developer, so I limit my patch to this specific (impactful) bug to prevent making the patch more complex than needed. This is already a tricky patch because relations are used in so many places in OSM.

comment:50 Changed 7 weeks ago by GerdP

OK, I just expected that the parents of the way are also downloaded when they are not known. I think this is also a common reason for the mentioned problems.
I think I can easily add the code for this based on your work.

comment:51 Changed 7 weeks ago by GerdP

Milestone: 20.03

comment:52 Changed 7 weeks ago by JeroenHoek

There is one thing that still need addressing with this patch.

  • We don't want to download relation members without consent of the user, so we ask the user (good).
  • Most users will want to download these members automatically (always, or only during this session), so we ask if the user wants to do this automatically from now on (good).
  • Users can also cancel the operation (good).

Now one other option the user should have is to proceed with the operation, but without downloading missing members. This may break things, but the user is warned, and there may be exceptions where the user knows what they are doing, and will make sure nothing breaks.

That option can be added.

I think it is desirable to allow the user to indicate that they want to download missing members automatically the next time, but I think it would be a mistake to allow them to automatically perform the split without doing this. This is because users will have no way of knowing whether a split succeeded because it contained no relations where the order of the new way parts could not be determined, or because they indicated that missing members should never be downloaded.

Also automatically cancelling the split would be very confusing.

What I think would be best is to offer these choices instead:


For x relations the order of the new way parts could not be determined automatically due to missing relation members. JOSM can download these for you before performing the split way operation.

  • Download missing relation members — do not ask again (remembers choice)
  • Download missing relation members — do not ask again (this session)
  • Download missing relation members — ask again the next time
  • Proceed with the split without downloading anything — caution: this may break relations!
  • Cancel

It would be an uncommon type of dialog in JOSM though.

Another option is to present this question in two dialogs:


For x relations the order of the new way parts could not be determined automatically due to missing relation members. JOSM can download these for you before performing the split way operation.

  • Download missing relation members
  • Proceed with the split without downloading anything — caution: this may break relations!
  • Cancel

Choosing the download option will then show this dailog:


Missing relation members will be downloaded:

o Ask again the next time
o Do not ask again (this session)
o Do not ask again (remembers choice)

[ OK ]


Version 0, edited 7 weeks ago by JeroenHoek (next)

comment:53 Changed 7 weeks ago by GerdP

When we add the possible download of the parents of the split node and split way it is probably better to say something like
"Relations might be corrupted by splitting the way. Download missing information from Server?"

comment:54 Changed 7 weeks ago by JeroenHoek

But I saw new code to download members of a relation and wondered what your code does when the relation is not yet known. I'll play with the fix to find out in what case it is an improvement when data is incomplete.

I've had a look at what happens when you download a way without its parents, but it looks like there is no way of knowing if it has parents without a download action. Wouldn't that mean that every split action command suddenly needs to offer to download information?

That would change this dialog from something you only see when you edit something containing routes or parts of a multipolygon, to something you see every time.

comment:55 Changed 7 weeks ago by GerdP

No, not always. The normal way is to check Way.isOutsideDownloadArea() or maybe only Node.isOutsideDownloadArea()
In fact it would be better to have a flag like "areParentsKnown()" but we don't have this. As you already wrote the normal use is to download an area and work with objects inside the area.

comment:56 Changed 7 weeks ago by JeroenHoek

I've improved the code documentation and the user consent dialog. I went for the two-part dialog, because that allows me to use JOptionPane and ConditionalOptionPaneUtil as-is.

Could someone remove attachments 0002-Unit-tests-for-split-way.patch​ and 0001-Fix-relation-ordering-after-split-way.2.patch​? They are not needed.

Changed 7 weeks ago by JeroenHoek

Mark attachment as empty

Changed 7 weeks ago by JeroenHoek

Mark attachment as empty

Changed 6 weeks ago by JeroenHoek

Attachment: testdata-bad.png added

Changed 6 weeks ago by JeroenHoek

Attachment: testdata-good.png added

comment:57 Changed 6 weeks ago by JeroenHoek

I've added a regression test that fails without this patch. You can reproduce it by loading data.osm, and splitting the way on node '100002'. You can see that the order of the relation is broken in the 'edit relation' dialog (the three ways are not all connected, see testdata-bad.png).

With this patch, the result is as expected (testdata-good.png).

Careful: this is mock data, so don't try to download/upload anything.

I don't think I can easily write a similar test for the edge case where members should be downloaded first.

comment:58 in reply to:  57 Changed 6 weeks ago by skyper

Replying to JeroenHoek:

...
Careful: this is mock data, so don't try to download/upload anything.
...

See wiki:/Help/Menu/OSMLayer#Layerstate, how to disable up- and/or download.

comment:59 in reply to:  56 Changed 6 weeks ago by GerdP

@JeroenHoek: Thanks for all the work!
1) I found a special case with a complex multipolygon relation where the order is still wrong after splitting a way.
Not sure if your patch is meant to fix this?
2) I would add a <p> before "To fix this, some missing relation members should be downloaded first." to reduce the width of the popup.
3) I was surprised that the "remember my choice" dialog doesn't pop up when I select one of the "No,..." options.

@Dirk: I am uncertain about the two new popups. Are they okay reg. I18N?

comment:60 Changed 6 weeks ago by GerdP

Forgot the link to the complex relation:
Load attachment rel1956189.osm.bz2 in #13289 (my preferred example for performance tests). Check that order of members is correct. Search for id:1580328498 and press P to split the way that is part of a complex inner as well as two outer members.
After splitting order is no longer correct.

comment:61 Changed 6 weeks ago by stoecker

The scripts will probably complain about the missing {0} in the number 1 case. Not sure.

comment:62 Changed 6 weeks ago by GerdP

Some small issues:

  • field DOWNLOAD_MISSING_PREF_KEY is not static. Intended?
  • javadoc for parameter whenRelationOrderUncertain is missing
  • sonarlint doesn't like type.equals("route") and wants "route"equals(type) even though type cannot be null here.


comment:63 Changed 6 weeks ago by anonymous

3) I was surprised that the "remember my choice" dialog doesn't pop up when I select one of the "No,..." options.

See the comments in code and https://josm.openstreetmap.de/ticket/18596#comment:52

The no-options really shouldn't be chosen automatically, because the user can't tell if this happens because they stored that preference, or because the split doesn't contain any relations that would otherwise trigger it.

The choice to store the preference is between always downloading members and always informing the user relations might be broken.

comment:64 Changed 6 weeks ago by JeroenHoek

Also, because this is really an edge-case (most splits don't need missing relation members to be downloaded, most issues solved in this patch are fixed by the improvements of the analysis phase of the split command), this is probably not much of a hindrance.

comment:65 Changed 6 weeks ago by JeroenHoek

The option that allows the user to proceed with a split without downloading missing relation members when the split analysis couldn't determine the order of some relations is potentially destructive, it really should only be chosen on a case-by-case basis, not automatically.

This is why only the option to download missing members automatically from now on is a storable preference.

comment:66 Changed 6 weeks ago by GerdP

OK, I also don't like the idea that the "No.." buttons are remembered silently. We do this in other situations as well (e.g. combine way needs revert) and it produces tickets from time to time. My preferred solution would be to have a notification whenever a dialog doesn't show up with a non-default option.

Last edited 6 weeks ago by GerdP (previous) (diff)

comment:67 Changed 6 weeks ago by JeroenHoek

field DOWNLOAD_MISSING_PREF_KEY is not static. Intended?

It is static though.

    private static String DOWNLOAD_MISSING_PREF_KEY = "split_way_download_missing_members";
Last edited 6 weeks ago by GerdP (previous) (diff)

comment:68 in reply to:  60 Changed 6 weeks ago by JeroenHoek

Replying to GerdP:

Forgot the link to the complex relation:
Load attachment rel1956189.osm.bz2 in #13289 (my preferred example for performance tests). Check that order of members is correct. Search for id:1580328498 and press P to split the way that is part of a complex inner as well as two outer members.
After splitting order is no longer correct.

I had a look, but that is a different problem (although related of course). I think it is too complex to solve within the scope of this patch.

comment:69 in reply to:  67 Changed 6 weeks ago by GerdP

Replying to JeroenHoek:

field DOWNLOAD_MISSING_PREF_KEY is not static. Intended?

It is static though.

    private static String DOWNLOAD_MISSING_PREF_KEY = "split_way_download_missing_members";

Arg, sorry, I meant final. Because name is in upper case.

comment:70 Changed 6 weeks ago by alphensebezorger

https://josm.openstreetmap.de/ticket/17400 seems related. (Ticket appears to address the same problem: splitting a way which extends outside the download-area, frequently breaks the sorting of parent relation(s), suggests warning the user before proceeding with splitting.)

comment:71 Changed 6 weeks ago by GerdP

I am not sure what to do with unit test oneMemberOrderedRelationShowsWarningTest(). It seems to work when I start the test on the command line

ant -Ddefault-junit-includes="**/**/SplitWayActionTest.class" test

but it fails in Eclipse no matter what button I press in the popup dialog.

Last edited 6 weeks ago by GerdP (previous) (diff)

comment:72 Changed 6 weeks ago by GerdP

Cc: taylor.smock added

Ah, sorry, I confused SplitWayActionTest with SplitWayCommandTest. With

ant -Ddefault-junit-includes="**/**/SplitWayCommandTest.class" test

I see

[junit] Tests run: 5, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 3,941 sec
    [junit] Test org.openstreetmap.josm.command.SplitWayCommandTest FAILED

I don't know how to mock the user input in unit tests.

comment:73 Changed 6 weeks ago by JeroenHoek

17400 is related, and features exactly the bug I have fixed. I have added the reproduction case from 17400 as a regression test.

I don't know how to mock the user input in unit tests.

I have amended the test. I forgot to do so after changing the patch from warning to downloading missing members. The test now verifies that the split is aborted when a split is attempted where the order of a relation cannot be determined without downloading missing members (this is one possible use of the WhenRelationOrderUncertain enum).

Changed 6 weeks ago by JeroenHoek

17400

comment:74 in reply to:  70 Changed 6 weeks ago by JeroenHoek

Replying to alphensebezorger:

https://josm.openstreetmap.de/ticket/17400 seems related.

Goed gevonden trouwens.

comment:75 Changed 6 weeks ago by GerdP

In 15943/josm:

see #18596 Fix relation ordering after split-way
Patch by JeroenHoek, slightly modified

  • download needed relation members if wanted
  • improve member ordering of route relations

TODO:

  • download parents if not known
  • fix also ordering of members in multipolygon relations

comment:76 Changed 6 weeks ago by GerdP

@Dirk: Where can I see whether the I18N strings are okay?

comment:77 in reply to:  72 ; Changed 6 weeks ago by taylor.smock

Replying to GerdP:

Ah, sorry, I confused SplitWayActionTest with SplitWayCommandTest. With

ant -Ddefault-junit-includes="**/**/SplitWayCommandTest.class" test

I see

[junit] Tests run: 5, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 3,941 sec
    [junit] Test org.openstreetmap.josm.command.SplitWayCommandTest FAILED

I don't know how to mock the user input in unit tests.

attachment:0001-Fix-relation-ordering-after-split-way.patch isn't applying cleanly for me.

That being said, I think I'd look at the JOptionPaneSimpleMocker first.

Alternatively, you can manually set the preferences yourself (I've done this, don't really recommend it, and I should probably go back and fix it now that I know about JOptionPaneSimpleMocker).

comment:78 in reply to:  76 ; Changed 6 weeks ago by stoecker

Replying to GerdP:

@Dirk: Where can I see whether the I18N strings are okay?

Call ant updatecore in i18n directory.

comment:79 in reply to:  77 Changed 6 weeks ago by GerdP

Replying to taylor.smock:

I don't know how to mock the user input in unit tests.

attachment:0001-Fix-relation-ordering-after-split-way.patch isn't applying cleanly for me.

Yes, sorry, the patch was changed and the unit tests worked, so I committed it.

That being said, I think I'd look at the JOptionPaneSimpleMocker first.

Alternatively, you can manually set the preferences yourself (I've done this, don't really recommend it, and I should probably go back and fix it now that I know about JOptionPaneSimpleMocker).

OK, thanks, I'll have a look.

comment:80 in reply to:  78 Changed 6 weeks ago by GerdP

Replying to stoecker:

Replying to GerdP:

@Dirk: Where can I see whether the I18N strings are okay?

Call ant updatecore in i18n directory.

Looking at the prerequisites I think this only works on unix based machines. I looked at jenkins job Projekt JOSM-i18n but found no hint.

comment:81 Changed 6 weeks ago by skyper

Ticket #17400 has been marked as a duplicate of this ticket.

comment:82 Changed 6 weeks ago by Hb---

The commit produced some new strings. Some use the phrase "part of a relation" for relation members. When meaning not yet downloaded members of relations JOSM uses "incomplete" für unvollständig. JOSM favors the "members" wording for elements of relations.

Parts of ways are called "segments" in JOSM. I guess here are the new ways meant. (See line 735)

The i18n process for some languages wants even for singular cases a placeholders for the number value 1. (See line 743 and #9110)

The patch does some Word Processing with the message of the dialog. In European languages this might seem possible but elsewhere it may create more problems than benefit. For translators the number of words in a long fragment is not an issue. Problems are very short fragments whose meanings do not depend on themselves. So these strings might want to be rechecked:

Download missing relation members?

Download incomplete relation members?

This way is part of a relation.
This way is part of {0} relations.
this relation
these relations
one relation
{0} Relations
For {0} the correct order of the new way parts could not be determined. To fix this, some missing relation members should be downloaded first.

The way to split is member of {0} relation. The correct order of the new ways can not be determined because of incomplete members.

The way to split is member of {0} relations. The correct order of the new ways can not be determined because of incomplete members.

Whats is bad with the classic OK, No, Cancel wording?

Yes, download the missing members

OK, download members

No, abort the split operation

Cancel the split

No, perform the split without downloading (The decision to be avoided has the longest caption?)

No

Thanks for the good work. Those nasty relation errors are a pain. Could type=waterways be included in this?

Last edited 6 weeks ago by Hb--- (previous) (diff)

comment:83 in reply to:  82 ; Changed 6 weeks ago by stoecker

Replying to Hb---:

The i18n process for some languages wants even for singular cases a placeholders for the number value 1. (See line 743 and #9110)

That's wrong. For the single case the {0} can be left out as long as the parameter is passed. Translation is independent from this and can readd it.

Changed 6 weeks ago by Hb---

comment:84 in reply to:  83 ; Changed 6 weeks ago by Hb---

Replying to stoecker:

... For the single case the {0} can be left out as long as the parameter is passed. Translation is independent from this and can readd it.

I don't understand this sentence. All placeholders given to the tr and trn functions are seen in launchpad as mandatory arguments for the translations. See pic. Placeholders given to trnand trnc functions are optional for the translator.

Also /DevelopersGuide/StyleGuide#Internationalization is very short on this.

Addendum: The pure getText allows to leave out all placeholders for plural entries https://www.gnu.org/software/gettext/manual/html_node/Translating-plural-forms.html but Launchpad notand Launchpad too.

Addenum2:
Seems that the trn function does not need a {0} at all. Only an integer to let the plural forms logic decide if the singular or which of the many possible plurals is returned.

String msgRelationsMissingData = trn(
        "The way to split is a member of a relation. <br>" +
                "The correct order of the new ways in this relation can <br>" +
                "not be determined because it has incomplete members.",
        "The way to split is a member of {0} relations. <br>" +
                "The correct order of the new ways in these relations can <br>" +
                "not be determined because they have incomplete members.",
        msgReferToRelations,
        msgReferToRelations,
);
Last edited 6 weeks ago by Hb--- (previous) (diff)

comment:85 in reply to:  84 Changed 6 weeks ago by stoecker

Replying to Hb---:

Seems that the trn function does not need a {0} at all. Only an integer to let the plural forms logic decide if the singular or which of the many possible plurals is returned.

That's what I said. For the single case there must be no placeholder. So the English text can also use one or a. Other languages where this is not so easy (e.g. Japanese) simply can add the {0} again when translating.

comment:86 Changed 6 weeks ago by JeroenHoek

Other languages where this is not so easy (e.g. Japanese) simply can add the {0} again when translating.

Even in Japanese this often leads to a more user-parseable translation; e.g.:

  "This way is part of {0} relation.",
  "This way is part of {0} relations.",

このウェイは1個のリレーションに含まれています。
このウェイは5個のリレーションに含まれています。

Versus:

  "This way is part of a relation.",
  "This way is part of {0} relations.",

このウェイは一つのリレーションに含まれています。 (or) このウェイはリレーションに含まれています。
このウェイは5個のリレーションに含まれています。

comment:87 in reply to:  82 Changed 6 weeks ago by JeroenHoek

Replying to Hb---:

Whats is bad with the classic OK, No, Cancel wording?

The meaning of OK, No, Cancel, has to be explained somewhere. This can be done in the dialog message, but in this case the button options seemed like a better place. The question posed to the user here is not as straight-forward as most dialogs.

Fortunately, this dialog is only shown for the relatively rare case of having only one member of a relation downloaded, and I expect that most users will consent to automatically downloading missing members after encountering it for the first time.

Replying to Hb---:

Could type=waterways be included in this?

As this patch has landed, I would recommend opening a new issue, preferably with an .osm file that illustrates current broken behaviour with waterways for use in a regression test (like #17400 has for bus-routes).

I think there may me more relation types that are applicable to this approach, so I hope that user feedback on this patch will point those out.

comment:88 in reply to:  82 ; Changed 6 weeks ago by JeroenHoek

Replying to Hb---:

The commit produced some new strings. Some use the phrase "part of a relation" for relation members. When meaning not yet downloaded members of relations JOSM uses "incomplete" für unvollständig. JOSM favors the "members" wording for elements of relations.

That was my mistake.

Parts of ways are called "segments" in JOSM. I guess here are the new ways meant. (See line 735)

That line was already present before my patch.

The patch does some Word Processing with the message of the dialog. In European languages this might seem possible but elsewhere it may create more problems than benefit. For translators the number of words in a long fragment is not an issue.

What do you suggest?

I wanted to prevent strings like these:

This way is part of 1 relation. For 1 relation the correct order of the new way parts could not be determined. To fix this, some missing relation members should be downloaded first.

This way is part of 5 relations. For 1 relations the correct order of the new way parts could not be determined. To fix this, some missing relation members should be downloaded first.

This way is part of 2 relations. For 2 relations the correct order of the new way parts could not be determined. To fix this, some missing relation members should be downloaded first.

This way is part of 4 relations. For 3 relations the correct order of the new way parts could not be determined. To fix this, some missing relation members should be downloaded first.

Preferring instead:

This way is part of a relation. For this relation the correct order of the new way parts could not be determined. To fix this, some missing relation members should be downloaded first.

This way is part of 5 relations. For one relations the correct order of the new way parts could not be determined. To fix this, some missing relation members should be downloaded first.

This way is part of 2 relations. For these relations the correct order of the new way parts could not be determined. To fix this, some missing relation members should be downloaded first.

This way is part of 4 relations. For 3 relations the correct order of the new way parts could not be determined. To fix this, some missing relation members should be downloaded first.

The last one is the same of course. Is there another way to do provide translators with the means to provide such translations? Just repeat the various long strings without the variables?

comment:89 Changed 6 weeks ago by GerdP

My suggestion would be to omit the details:
trn("Information about incomplete relation member is needed.<br>Download needed member?",
"Information about incomplete {0} relation members is needed.<br>Download needed members?",...);
Buttons as suggested above: OK, Cancel the split, No

comment:90 in reply to:  88 Changed 6 weeks ago by Hb---

Replying to stoecker:

... For the single case there must be no placeholder.

I doubt that for the plural case aAlso for the plural case no placeholder is needed when the numeric value should not be presented to the user, but JOSM guidelines may vary. Technically needed is only the long integer for the trn() function. A separate question is if hiding the value is good for the user.

Other languages where this is not so easy (e.g. Japanese) simply can add the {0} again when translating.

I doubt that tThe occurrence and the number of parameters cannot be varied in simple translations handled by Launchpad, see screenshot in comment:84. In plural translations all parameters are optional for the translator.


Replying to JeroenHoek:

The meaning of OK, No, Cancel, has to be explained somewhere. ...
... and I expect that most users will consent to automatically downloading missing members after encountering it for the first time.

That's why I suggested the extended OK No Cancel strings. JOSM has now several Yes, do this strings but only one OK, do this. So Yes is a good option, too.

  • Yes, download incomplete because Yes, download incomplete members seems very long.
  • Cancel or Cancel the split
  • No because with No, perform the split without downloading the disliked decision has the longest caption.

Replying to JeroenHoek:

The patch does some Word Processing

What do you suggest?

...
This way is part of 4 relations. For 3 relations the correct order of the new way parts could not be determined. To fix this, some missing relation members should be downloaded first.

The last one is the same of course.

To provide correct plural forms for two numeric values the trn() function must be called twice.

Is there another way to do provide translators with the means to provide such translations?

JOSM uses a "context" to technically separate equal source strings with different meanings from each other. So different translations can be given to the user, see trc() and trnc() functions. For example most menu items have menu as context.

Additionally the java comments above the trn() calls are scraped from the source and delivered to the translator as a comment. But they decide not about the translation given to the user. Example is line 742 in SplitWayCommand.java with for correct i18n of plural forms - see #9110.

Just repeat the various long strings without the variables?

As GerdP wrote, the number of not order-able relations can be omitted in the scope of this dialog box.

Last edited 6 weeks ago by Hb--- (previous) (diff)

comment:91 Changed 6 weeks ago by stoecker

@Hb--:

As said in the other tickets. Stop commenting on issues where you don't understand the underlying principles. You only make contributors feel unsure.

@stoecker:
Sorry, the different placeholder requirements between simple and plural msgstr trapped me. So I changed and added my comments 82 , 84 and 90. The Internationalization section will be updated. --Hb 2020-03-01 12:45

Last edited 6 weeks ago by Hb--- (previous) (diff)

comment:92 Changed 5 weeks ago by skyper

Found a false positive: see attached example and split at node id:4518025255. There are at least the privious and next members present if ordered. Could it be that there is a problem when splitting the first way of a relation when above members are only stops and platforms ?

Last edited 5 weeks ago by skyper (previous) (diff)

comment:93 in reply to:  92 Changed 5 weeks ago by JeroenHoek

Replying to skyper:

Found a false positive: see attached example and split at node id:4518025255. There are at least the privious and next members present if ordered. Could it be that there is a problem when splitting the first way of a relation when above members are only stops and platforms ?

I've had a look, and it looks like relation CB 2 has its ways in the wrong order, causing this bug to pop up. It is correct that the split way operation cannot be sure of the direction, but it shouldn't offer to download in this case, because the neighbouring members are already known.

So two bugs really:

  • Bus route CB 2 here is broken
  • The split-way command should not offer to download anything when the neighbouring members are known, but not connected

At least I think that is what's wrong here.

Could you open a new issue with this reproduction case and bug? That will give it its own number (useful for the regression test) and a place to submit a patch.

comment:94 Changed 5 weeks ago by skyper

Thought, as the ticket is still open, to collect all info here but sure I can open separate tickets for each issue in this ticket.

In the example, there are almost all relations unordered and many broken. That is real life. You analysis is right, so, nothing JOSM can do and no warning needed.

EDIT: On more note: If forward/backward is present as role there is no need to download but looking at the direction of the way to split should be enough to find the proper position. not always and it is not possible without at least one more neighbor.

Last edited 8 days ago by skyper (previous) (diff)

comment:95 in reply to:  94 Changed 5 weeks ago by JeroenHoek

Replying to skyper:

Thought, as the ticket is still open, to collect all info here but sure I can open separate tickets for each issue in this ticket.

Thanks, I think I can solve some of them, so feel free to CC me (I think that's how it works on Trac?) on these. As this patch has already landed, it is probably easier to have separate issues for any bugs or new use cases found to allow them to be fixed and reviewed separately.

Changed 5 weeks ago by skyper

example

comment:96 Changed 4 weeks ago by Don-vip

@Gerd: what's the status of this ticket? The number of comments is astonishing.

comment:97 Changed 4 weeks ago by GerdP

My understanding is that the problem that is described in the first description block is fixed, but minor new problems were introduced with the patch and some related problems are not fixed, see comment:75
I kept it open because the ticket summary implies that all problems are fixed.
If we change it to something like "Fix route relation ordering after split-way" we can probably call it fixed.

comment:98 Changed 3 weeks ago by skyper

See #18863 for an additional patch.

comment:99 Changed 9 days ago by Don-vip

Resolution: fixed
Status: newclosed

Too many comments, it's impossible for me to understand the status of this ticket. Please create new tickets for remaining points, if any.

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.