-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Additional SVG diagrams in updaters package.md #5936
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #5936 +/- ##
===========================================
Coverage 69.73% 69.73%
- Complexity 17292 17315 +23
===========================================
Files 1954 1960 +6
Lines 74163 74268 +105
Branches 7595 7603 +8
===========================================
+ Hits 51714 51794 +80
- Misses 19809 19831 +22
- Partials 2640 2643 +3 ☔ View full report in Codecov by Sentry. |
also added line break to improve page formatting
| * A 2D array of String containing zero or more Via messages displayed at each stop in the | ||
| * stop sequence. A Via is an additional intermediate destination that is displayed alongside the | ||
| * terminus headsign, but will usually change or only be displayed at certain stops along the way. | ||
| * While the concept of Headsigns exists in both GTFS (Headsign) and Netex (DestinationDisplay), | ||
| * the Via concept is only present in Transmodel. This reference be null if no stop in the entire | ||
| * sequence of stops has any via strings. Any subarray may also be null or empty if no Via strings | ||
| * are displayed at that particular stop. These nulls are allowed to conserve memory in the common | ||
| * case where there are few or no via messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All domain specific info should be moved to and merged with the TripTimes#getHeadsignVias(int stop). Only implementation details should be here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved that information to the getter ScheduledTripTimes#getHeadsignVias. The question then arises how far up the chain of accessors the information should be placed, and whether it is acceptable for it to be replicated.
It turns out similar information was already available on RealTimeTripTimes#getHeadsignVias and in the interface declaration TripTimes#getHeadsignVias, but I somehow didn't spot or understand these when working on ScheduledTripTimes. No such explanation is present on TripTimeOnDate#getHeadsignVias which reads through to the same data, but the Javadoc is readily found by following the method call.
In the end I consolidated information from the various Javadoc on the interface (overridden) method declaration.
...ain/java/org/opentripplanner/transit/model/timetable/StopTimeToScheduledTripTimesMapper.java
Show resolved
Hide resolved
src/main/java/org/opentripplanner/updater/images/snapshot-manager.svg
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the diagrams there are a few references(arrows) from Timetable X#List<TripTimes> to Timetable. Should they point to TripTimes instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was intentional, but I can see why the meaning might not be clear. In the original white snapshot, the TripTimes are only shown for the frontmost timetable A in the pile. Timetables B and C are also implied to have TripTimes B1, B2, B3... and C1, C2, C3... which are not shown to avoid clutter. For this reason the protective copy of timetable B is shown pointing to a pile of objects that includes the Timetables and their associated (implied) TripTimes. I think if I try to show all these the diagram will become super complicated (and it will take some hours to re-organize). So I will at least add an explanation that the B and C trip times are implied to be located within that sub-tree of objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diagrams have been updated as discussed in the meetings.
…TimeToScheduledTripTimesMapper.java Co-authored-by: Thomas Gran <[email protected]>
also updated descriptions to reflect recent changes to realtime system
|
I have updated the text in response to review comments, and to reflect changes introduced in #5974. This is ready for another review. |
| @Nullable | ||
| private final String[][] headsignVias; | ||
|
|
||
| /** TODO RT_AB: document, what is this for? */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for matching stops with external services, like in the realtime-updaters.
arrows now point at correct hierarchical level, but stop short of the specific individual objects when they are referring to instances that are elided from the diagrams
|
I've made the requested changes to the Javadoc, and changed the arrows as discussed in a recent meeting. Arrows now point at the correct hierarchical level in the correct snapshot column, but stop short of the specific individual objects when they are referring to instances that are elided from the diagrams. Excalidraw seems to have started embedding fonts in the SVG styles some time after I started this PR. So the newly exported ones (diagrams 6 through 8) all display with the scribbly Excalidraw font now, while the display of the first five seems to depend on which browser is displaying them (I see a serif font or scribble font depending on where I preview it). I then touched up and re-generated diagram 5 trying a sans-serif font to see how the fonts look inline in the docs. The content and layout of the diagrams is finalized for review now, but I'll want to make all the fonts uniform. Any preferences between scribble / sans-serif / serif when you're looking at the package.md? Though the scribble font is harder to read, the diagrams were laid out with that font. So re-exporting them will probably go smoother (less repetitive pixel pushing tweaks) if I set everything to scribble font. |
src/main/java/org/opentripplanner/transit/model/timetable/ScheduledTripTimes.java
Outdated
Show resolved
Hide resolved
| @Nullable | ||
| private final String[][] headsignVias; | ||
|
|
||
| /** This is for matching stops with external services, in the realtime-updaters for example. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /** This is for matching stops with external services, in the realtime-updaters for example. */ |
This field should probably be renamed to match the getter method. This is pass-through-information and no implementation specific information should be necessary. The existing JavaDoc for this field, found on the TripTimes#gtfsSequenceOfStopIndex(int stop) method, is:
Returns the GTFS sequence number of the given 0-based stop position.
These are the GTFS stop sequence numbers, which show the order in which the vehicle visits the
stops. Despite the fact that the StopPattern or TripPattern enclosing this class provides an
ordered list of Stops, the original stop sequence numbers may still be needed for matching
with GTFS-RT update messages. Unfortunately, each individual trip can have totally different
sequence numbers for the same stops, so we need to store them at the individual trip level. An
effort is made to re-use the sequence number arrays when they are the same across different
trips in the same pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have moved Javadoc up to interfaces and renamed the field to match interface accessor methods as requested. Even where information is pure pass-through I am still finding it a little awkward to remove all Javadoc, because although the IDE helps a bit, you still need to check all uses of the field, browse through the read uses to figure out which one is an accessor, go to that accessor, and then use something like cmd-U to find the right superclass (interface) method with the Javadoc. What do you think about using @see to show which interface method is reading through to the field, and allow the reader to find the Javadoc in one click (example in my latest commit)?
also moved javadoc to interfaces instead of impl in response to PR review
The PR was not tagged.
This PR adds more diagrams and explanation to the
src/main/java/org/opentripplanner/updater/package.mddeveloper documentation. The diagrams are stored as Excalidraw files, which have been rendered to SVG in theimagessubdirectory. The new diagrams are:The second, larger diagram also shows how snapshots interact with multiple routing and API requests, and the effects of garbage collection. This information is complementary to the other SVG sequence diagram already present in this directory.
I also included a few edits to Javadoc on a topic that I ran into while working on this (Transmodel "via" strings).