[WIP patch] Smarter DistributeAction action for single node

[gaben]

Problem

Imagine the following scenario:

1. There is a way with a few nodes (o is a node):

   o---------o---o---o
   1         2   3   4
   

2. Let's say the second node should be aligned relative to the first and third nodes:

   o------o------o---o
   1      2      3   4
   

Currently the only way for doing by selecting nodes 1, 2 and 3, then using the Distribute Nodes Action.

Proposal

With the new method (source code calls it algorithm), only the second node needs to be selected JOSM will figure out if possible, then act accordingly.

The patch is in testing, looks good for now. Probably I'll need help to make it unit testable.

Please provide any additional information below. Attach a screenshot if possible.

I may already created a ticket for it a few years back, couldn't find it now.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2023-03-05 17:42:37 +0100 (Sun, 05 Mar 2023)
Build-Date:2023-03-06 02:30:59
Revision:18682
Relative:URL: ^/trunk

[gaben]

unit test needed

[gaben]

Just noticed the notification description is not precise enough, should be edited. (Kind of a note for myself.)

[gaben]

add HTML notification formatting

[gaben]

Crated a PR on GH: ​pull/129.

Just realized that DistributeAction can cause overlapping way issues by design. That's the only reason I marked it as a draft, otherwise ready for merge.

To reproduce, create a zigzag way with 4 nodes, where the two inner nodes should be the furthest apart.

[taylor.smock]

Another way to create an overlapping way issue is via the same zigzag issue you pointed out in comment:2, except you select one of the inner nodes instead of all the nodes. I was going to ask why you weren't using cmds = distributeWay(nodes.iterator().next().getParentWays(), nodes);, but that looks like it just performs the action on the entire way.

[taylor.smock]

A minor suggestion:

We can split out a good chunk of the distributeNodes method out into its own method, e.g.

    private static Collection<Command> distributeNodes(INode nodea, INode nodeb, Collection<Node> nodes) {
        // Find out co-ords of A and B

and call that from the original distributeNodes with distributeNodes(nodea, nodeb, nodes) along with calling distributeNodes(neighbours.get(0), neighbours.get(1), new ArrayList<>(Collections.singleton(nodeToDistribute))); from distributeNode.

[gaben]

I have a new idea to work around the overlapping way issue.

What if I:

1. split the way by neighbour nodes (we have 2 or 3 ways at this point)
2. apply the distributeWay() function to the new way (which has three nodes)
3. merge the ways.

---

Replying to taylor.smock:

A minor suggestion:

We can split out a good chunk of the distributeNodes method out into its own method

Usually, I intentionally do not make changes to existing code or data flow because

1. I don't want to break things
2. I only have JOSM core checked out
3. it's usually rewritten after the review anyway

If you already brought up structural changes/simplifications, I think there should be some public utility methods for such primitive calculations. And yes, it looks like we have a duplicate in org.openstreetmap.josm.actions.AlignInLineAction#nodePairFurthestApart (you'll find it as "Appart", there is a typo in the name).

In summary, with these proposals we could simplify two classes and fix the ticket.

I might open a task ticket with the duplicates, or good utility method candidates I find along coding.