Opened 10 years ago
Closed 9 years ago
#9584 closed enhancement (needinfo)
[Patch in development] Support cardinal direction member roles in LinkedCellRenderer
Reported by: | mvexel | Owned by: | team |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | Core | Version: | |
Keywords: | Cc: |
Description
This covers cardinal direction support in the relation editor, particularly in the LinkedCellRenderer.
Right now, only the 'forward' and 'backward' roles are considered when rendering the relation visualization in the LinkedCellRenderer. This patch expands the support to 'north' / 'south' and 'east' / 'west', member roles that are frequently used in the United States and other countries where the motorways / freeways are signposted with cardinal directions, for example: Interstate 80 East.
Attachments (4)
Change History (27)
comment:1 by , 10 years ago
Summary: | Support cardinal direction member roles in → Support cardinal direction member roles in LinkedCellRenderer |
---|
comment:2 by , 10 years ago
Summary: | Support cardinal direction member roles in LinkedCellRenderer → [Patch] Support cardinal direction member roles in LinkedCellRenderer |
---|
by , 10 years ago
Attachment: | cardinaldirections.patch added |
---|
by , 10 years ago
Attachment: | 9584_v2.patch added |
---|
follow-up: 10 comment:3 by , 10 years ago
Please always try to reformat/reindent as little code as possible to ease reviewing the patch. See attachment:9584_v2.patch for a cleaned up version of your attachment:cardinaldirections.patch
by , 10 years ago
Attachment: | I80_west_unconnected.png added |
---|
follow-up: 6 comment:4 by , 10 years ago
follow-up: 7 comment:5 by , 10 years ago
Is'nt there a problem with this:
ROUNDABOUT_LEFT { @Override public Direction interpretation() { return FORWARD; } }, ROUNDABOUT_RIGHT { @Override public Direction interpretation() { return BACKWARD; } },
Does it work correctly in left-driving countries as in right-driving countries ?
follow-up: 8 comment:6 by , 10 years ago
Replying to simon04:
When opening relation/113177 in the editor,
west
roles show up to be unconnected – is this an expected behaviour?
I get the same behavior when I change the roles from east / west to forward / backward (also in the current release client) so this is not behavior that was introduced through this patch as far as I can see. Perhaps a problem with the relation sorter?
follow-up: 11 comment:7 by , 10 years ago
Replying to Don-vip:
Is'nt there a problem with this:
...
Does it work correctly in left-driving countries as in right-driving countries ?
Not that I can see, what do you think the problem might be?
follow-up: 9 comment:8 by , 10 years ago
Replying to mvexel:
Perhaps a problem with the relation sorter?
4 related tickets, maybe it's there: #6166, #6920, #8019, #8925
Replying to mvexel:
Not that I can see, what do you think the problem might be?
I don't know, just want to check it works correctly in both systems (haven't done any test yet with the patch)
comment:9 by , 10 years ago
Replying to Don-vip:
Replying to mvexel:
Perhaps a problem with the relation sorter?
4 related tickets, maybe it's there: #6166, #6920, #8019, #8925
#6166 seems relevant. #6920 to my mind describes a different situation. #8019 I think has been fixed (in the sense that reversing a way that has forward:*
is correctly detected and supported, as is north:*
and the other cardinal directions. #8925 I don't even understand :)
Replying to mvexel:
Not that I can see, what do you think the problem might be?
I don't know, just want to check it works correctly in both systems (haven't done any test yet with the patch)
comment:10 by , 10 years ago
Replying to simon04:
Please always try to reformat/reindent as little code as possible to ease reviewing the patch. See attachment:9584_v2.patch for a cleaned up version of your attachment:cardinaldirections.patch
Thanks and sorry about that, Eclipse messed it up and I neglected to check before I submitted.
follow-up: 15 comment:11 by , 10 years ago
Replying to mvexel:
Not that I can see, what do you think the problem might be?
Why is it necessary to change the interpretation of roundabouts to something different, i.e., why not interpret ROUNDABOUT_LEFT
as ROUNDABOUT_LEFT
?
follow-up: 13 comment:12 by , 10 years ago
A first note after a look at the (cleaned) patch:
The changes in src/org/openstreetmap/josm/corrector/ReverseWayTagCorrector.java are wrong. They duplicate an entry and break the changed line.
follow-up: 14 comment:13 by , 10 years ago
Replying to stoecker:
A first note after a look at the (cleaned) patch:
The changes in src/org/openstreetmap/josm/corrector/ReverseWayTagCorrector.java are wrong. They duplicate an entry and break the changed line.
The only thing changed in that file is fixing two (actually technically unrelated) typos 'forwards' --> ' forward' and 'backwards' --> 'backward' - unless there is something you see that I am not?
comment:14 by , 10 years ago
Replying to mvexel:
The only thing changed in that file is fixing two (actually technically unrelated) typos 'forwards' --> ' forward' and 'backwards' --> 'backward' - unless there is something you see that I am not?
See private static final StringSwitcher FORWARD_BACKWARD = new StringSwitcher("forward", "backward");
some lines above your change.
follow-up: 17 comment:15 by , 10 years ago
follow-up: 19 comment:16 by , 10 years ago
Since you basically consider north/west/…
as forward/backward
, in org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionTypeCalculator#determineDirection
and org.openstreetmap.josm.gui.dialogs.relation.MemberTableLinkedCellRenderer#paintComponent
, Direction is never NORTH/WEST/…
comment:17 by , 10 years ago
Replying to stoecker:
Replying to simon04:
Replying to mvexel:
Not that I can see, what do you think the problem might be?
Why is it necessary to change the interpretation of roundabouts to something different, i.e., why not interpret
ROUNDABOUT_LEFT
asROUNDABOUT_LEFT
?
Same question for "NONE".
Good point, those should probably go through unmapped. Let me see if I can / should submit a modified patch.
comment:18 by , 10 years ago
To only influence the rendering of the directional keys, you might want to elaborate from
-
src/org/openstreetmap/josm/gui/dialogs/relation/sort/WayConnectionTypeCalculator.java
diff --git a/src/org/openstreetmap/josm/gui/dialogs/relation/sort/WayConnectionTypeCalculator.java b/src/org/openstreetmap/josm/gui/dialogs/rel index b5cbd2e..fa48702 100644
a b import static org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionType 6 6 import static org.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionType.Direction.NONE; 7 7 8 8 import java.util.ArrayList; 9 import java.util.Arrays; 9 10 import java.util.List; 10 11 11 12 import org.openstreetmap.josm.data.osm.Node; … … public class WayConnectionTypeCalculator { 52 53 wct.linkPrev = i>0 && con.get(i-1) != null && con.get(i-1).isValid(); 53 54 wct.direction = NONE; 54 55 55 if (RelationSortUtils.isOneway(m)){ 56 final boolean isDirectionalForward = m.getRole().startsWith("south") || m.getRole().startsWith("west"); 57 final boolean isDirectionalBackward = m.getRole().startsWith("north") || m.getRole().startsWith("east"); 58 final boolean isDirectional = isDirectionalForward || isDirectionalBackward; 59 if (isDirectional) { 60 wct.direction = isDirectionalForward ? FORWARD : BACKWARD; 61 wct.linkPrev = lastWct != null && lastWct.direction == wct.direction; 62 } 63 64 if (!isDirectional && RelationSortUtils.isOneway(m)){ 56 65 if (lastWct != null && lastWct.isOnewayTail) { 57 66 wct.isOnewayHead = true; 58 67 } … … public class WayConnectionTypeCalculator { 64 73 } 65 74 } 66 75 67 if ( wct.linkPrev) {76 if (!isDirectional && wct.linkPrev) { 68 77 if (lastBackwardWay != UNCONNECTED && lastForwardWay != UNCONNECTED) { 69 78 wct = determineOnewayConnectionType(con, m, i, wct); 70 79 if (!wct.linkPrev) { … … public class WayConnectionTypeCalculator { 78 87 } 79 88 } 80 89 81 if (! wct.linkPrev) {90 if (!isDirectional && !wct.linkPrev) { 82 91 wct.direction = determineDirectionOfFirst(i, m); 83 92 if (RelationSortUtils.isOneway(m)){ 84 93 wct.isOnewayLoopForwardPart = true;
follow-up: 20 comment:19 by , 10 years ago
Replying to simon04:
Since you basically consider
north/west/…
asforward/backward
, inorg.openstreetmap.josm.gui.dialogs.relation.sort.WayConnectionTypeCalculator#determineDirection
andorg.openstreetmap.josm.gui.dialogs.relation.MemberTableLinkedCellRenderer#paintComponent
, Direction is neverNORTH/WEST/…
Hey Simon, don't quite understand what you're saying here, or in #18 - could you elaborate please? Happy to get in IRC as well if that works better for you. I am supplying a modified patch that should address the other concerns, thanks!
comment:20 by , 10 years ago
Summary: | [Patch] Support cardinal direction member roles in LinkedCellRenderer → [Patch in development] Support cardinal direction member roles in LinkedCellRenderer |
---|
Replying to mvexel:
Hey Simon, don't quite understand what you're saying here
You actually never assign Direction.NORTH
to a variable, e.g., the method determineDirection
in WayConnectionTypeCalculator
does never return NORTH
. Currently, this enum field is completely useless.
Instead, you might want to handle the north
role in WayConnectionTypeCalculator
as sketched in comment:18 and ignore your current patch for now.
comment:21 by , 10 years ago
I don't understand what you're saying. The code we wrote maps from North, South, East, West to Forward/Backward with JOSM already understands.
comment:23 by , 9 years ago
Resolution: | → needinfo |
---|---|
Status: | new → closed |
Can you please attach your patch with
.diff
or.patch
extension ? It allows Trac to display it nicely for review. Thanks :)