diff --git a/application/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNamesTest.java b/application/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNamesTest.java index 7f389bc41ec..bc7e7c1cd4a 100644 --- a/application/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNamesTest.java +++ b/application/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNamesTest.java @@ -57,7 +57,9 @@ void changeNames() { var updatedLeg = itinerary.getLegs().getFirst(); assertEquals(STOP_C.getName(), updatedLeg.getFrom().name); + assertEquals(STOP_C.getId(), updatedLeg.getFrom().stop.getId()); assertEquals(STOP_D.getName(), updatedLeg.getTo().name); + assertEquals(STOP_C.getId(), updatedLeg.getTo().stop.getId()); // Check that the fares were carried over assertEquals(fpu, updatedLeg.fareProducts()); diff --git a/application/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationModuleTest.java b/application/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationModuleTest.java index 0df495d63d0..01fa0de29eb 100644 --- a/application/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationModuleTest.java +++ b/application/src/ext-test/java/org/opentripplanner/ext/stopconsolidation/StopConsolidationModuleTest.java @@ -2,6 +2,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.opentripplanner.ext.stopconsolidation.TestStopConsolidationModel.PATTERN; import static org.opentripplanner.ext.stopconsolidation.TestStopConsolidationModel.STOP_A; import static org.opentripplanner.ext.stopconsolidation.TestStopConsolidationModel.STOP_B; @@ -29,6 +30,7 @@ void replacePatterns() { var modifiedPattern = transitModel.getTripPatternForId(PATTERN.getId()); assertFalse(modifiedPattern.getRoutingTripPattern().getPattern().sameAs(PATTERN)); assertFalse(modifiedPattern.sameAs(PATTERN)); + assertTrue(modifiedPattern.getScheduledTimetable().getPattern().sameAs(modifiedPattern)); var modifiedStop = modifiedPattern .getRoutingTripPattern() diff --git a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java index 1806b3e9e32..5cb6b07ae06 100644 --- a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java +++ b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/DecorateConsolidatedStopNames.java @@ -31,8 +31,9 @@ public void decorate(Itinerary itinerary) { * operating the route, so that the name in the result matches the physical signage on the stop. *

* If the leg has a "to" stop that is a primary stop, then we don't want to show the stop that's on - * the signage but what is shown _inside_ the vehicle. That's why we use the agency-specific (aka - * secondary) stop. + * the signage but what is shown _inside_ the vehicle. However, we still want to use the primary ID + * for stop information and alternatives legs. So we copy the agency-specific (aka secondary) stop + * name to the primary stop object. *

* This follows the somewhat idiosyncratic logic of the consolidated stops feature. */ diff --git a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java index 53a0f8ed827..4756113c5d5 100644 --- a/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java +++ b/application/src/ext/java/org/opentripplanner/ext/stopconsolidation/internal/DefaultStopConsolidationService.java @@ -8,8 +8,13 @@ import org.opentripplanner.ext.stopconsolidation.StopConsolidationService; import org.opentripplanner.ext.stopconsolidation.model.ConsolidatedStopGroup; import org.opentripplanner.ext.stopconsolidation.model.StopReplacement; +import org.opentripplanner.framework.i18n.I18NString; import org.opentripplanner.transit.model.framework.FeedScopedId; import org.opentripplanner.transit.model.organization.Agency; +import org.opentripplanner.transit.model.site.AreaStop; +import org.opentripplanner.transit.model.site.GroupStop; +import org.opentripplanner.transit.model.site.RegularStop; +import org.opentripplanner.transit.model.site.RegularStopBuilder; import org.opentripplanner.transit.model.site.StopLocation; import org.opentripplanner.transit.service.TransitModel; import org.slf4j.Logger; @@ -93,7 +98,24 @@ private Optional findAgencySpecificStop(StopLocation stop, Agency .flatMap(g -> g.secondaries().stream()) .filter(secondary -> secondary.getFeedId().equals(agency.getId().getFeedId())) .findAny() - .map(id -> transitModel.getStopModel().getRegularStop(id)); + .map(id -> copyStopWithName(stop, transitModel.getStopModel().getRegularStop(id).getName())); + } + + private StopLocation copyStopWithName(StopLocation primaryStop, I18NString agencySpecificName) { + if (primaryStop instanceof RegularStop) { + return ((RegularStop) primaryStop).copy().withName(agencySpecificName).build(); + } + + if (primaryStop instanceof AreaStop) { + return ((AreaStop) primaryStop).copy().withName(agencySpecificName).build(); + } + + if (primaryStop instanceof GroupStop) { + return ((GroupStop) primaryStop).copy().withName(agencySpecificName).build(); + } + + // should not happen unless new stop types are added + throw new IllegalArgumentException("Unknown stop location type: " + primaryStop); } @Override diff --git a/application/src/main/java/org/opentripplanner/model/Timetable.java b/application/src/main/java/org/opentripplanner/model/Timetable.java index 6740e2cc99d..1cf819966f7 100644 --- a/application/src/main/java/org/opentripplanner/model/Timetable.java +++ b/application/src/main/java/org/opentripplanner/model/Timetable.java @@ -20,6 +20,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import javax.annotation.Nullable; import org.opentripplanner.framework.time.ServiceDateUtils; @@ -483,4 +484,13 @@ private static TripTimes getRepresentativeTripTimes( return null; } } + + public boolean sameAs(Timetable other) { + return ( + Objects.equals(pattern, other.pattern) && + Objects.equals(tripTimes, other.tripTimes) && + Objects.equals(frequencyEntries, other.frequencyEntries) && + Objects.equals(serviceDate, other.serviceDate) + ); + } } diff --git a/application/src/main/java/org/opentripplanner/transit/model/network/TripPattern.java b/application/src/main/java/org/opentripplanner/transit/model/network/TripPattern.java index becf1686733..f542a0361d1 100644 --- a/application/src/main/java/org/opentripplanner/transit/model/network/TripPattern.java +++ b/application/src/main/java/org/opentripplanner/transit/model/network/TripPattern.java @@ -128,17 +128,7 @@ public final class TripPattern this.netexSubMode = requireNonNull(builder.getNetexSubmode()); this.containsMultipleModes = builder.getContainsMultipleModes(); - if (builder.getScheduledTimetable() != null) { - if (builder.getScheduledTimetableBuilder() != null) { - throw new IllegalArgumentException( - "Cannot provide both scheduled timetable and scheduled timetable builder" - ); - } - this.scheduledTimetable = builder.getScheduledTimetable(); - } else { - this.scheduledTimetable = - builder.getScheduledTimetableBuilder().withTripPattern(this).build(); - } + this.scheduledTimetable = builder.getScheduledTimetableBuilder().withTripPattern(this).build(); this.originalTripPattern = builder.getOriginalTripPattern(); @@ -469,7 +459,7 @@ public boolean sameAs(TripPattern other) { Objects.equals(this.containsMultipleModes, other.containsMultipleModes) && Objects.equals(this.name, other.name) && Objects.equals(this.stopPattern, other.stopPattern) && - Objects.equals(this.scheduledTimetable, other.scheduledTimetable) + this.scheduledTimetable.sameAs(other.scheduledTimetable) ); } diff --git a/application/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java b/application/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java index ec2451ea66d..8175597e974 100644 --- a/application/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java +++ b/application/src/main/java/org/opentripplanner/transit/model/network/TripPatternBuilder.java @@ -28,7 +28,6 @@ public final class TripPatternBuilder private SubMode netexSubMode; private boolean containsMultipleModes; private StopPattern stopPattern; - private Timetable scheduledTimetable; private TimetableBuilder scheduledTimetableBuilder; private String name; @@ -50,7 +49,7 @@ public final class TripPatternBuilder this.netexSubMode = original.getNetexSubmode(); this.containsMultipleModes = original.getContainsMultipleModes(); this.stopPattern = original.getStopPattern(); - this.scheduledTimetable = original.getScheduledTimetable(); + this.scheduledTimetableBuilder = original.getScheduledTimetable().copyOf(); this.createdByRealtimeUpdate = original.isCreatedByRealtimeUpdater(); this.originalTripPattern = original.getOriginalTripPattern(); this.hopGeometries = @@ -93,24 +92,13 @@ public TripPatternBuilder withStopPattern(StopPattern stopPattern) { } public TripPatternBuilder withScheduledTimeTable(Timetable scheduledTimetable) { - if (scheduledTimetableBuilder != null) { - throw new IllegalStateException( - "Cannot set scheduled Timetable after scheduled Timetable builder is created" - ); - } - this.scheduledTimetable = scheduledTimetable; + this.scheduledTimetableBuilder = scheduledTimetable.copyOf(); return this; } public TripPatternBuilder withScheduledTimeTableBuilder( UnaryOperator producer ) { - // create a builder for the scheduled timetable only if it needs to be modified. - // otherwise reuse the existing timetable - if (scheduledTimetableBuilder == null) { - scheduledTimetableBuilder = scheduledTimetable.copyOf(); - scheduledTimetable = null; - } producer.apply(scheduledTimetableBuilder); return this; } @@ -141,9 +129,6 @@ public int transitReluctanceFactorIndex() { } public Direction getDirection() { - if (scheduledTimetable != null) { - return scheduledTimetable.getDirection(); - } return scheduledTimetableBuilder.getDirection(); } @@ -172,10 +157,6 @@ public StopPattern getStopPattern() { return stopPattern; } - public Timetable getScheduledTimetable() { - return scheduledTimetable; - } - public TimetableBuilder getScheduledTimetableBuilder() { return scheduledTimetableBuilder; } diff --git a/application/src/test/java/org/opentripplanner/transit/model/network/TripPatternTest.java b/application/src/test/java/org/opentripplanner/transit/model/network/TripPatternTest.java index b2adc7544a3..622b2e3e4a8 100644 --- a/application/src/test/java/org/opentripplanner/transit/model/network/TripPatternTest.java +++ b/application/src/test/java/org/opentripplanner/transit/model/network/TripPatternTest.java @@ -68,6 +68,9 @@ void copy() { assertEquals(subject.getHopGeometry(0), copy.getHopGeometry(0)); assertEquals(subject.getHopGeometry(1), copy.getHopGeometry(1)); assertEquals(HOP_GEOMETRIES.get(1), copy.getHopGeometry(1)); + + // The linked timetable should link back to the new pattern + assertSame(copy, copy.getScheduledTimetable().getPattern()); } @Test