-
Notifications
You must be signed in to change notification settings - Fork 100
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
feat: Initial support for validation app GUI #1146
feat: Initial support for validation app GUI #1146
Conversation
|
cc @barbeau in case you have thoughts |
@bdferris thanks for working on this initial GUI! |
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 haven't reviewed everything yet, but here are my first comments.
app/gui/src/main/java/org/mobilitydata/gtfsvalidator/app/gui/GtfsValidatorApp.java
Outdated
Show resolved
Hide resolved
app/gui/src/main/java/org/mobilitydata/gtfsvalidator/app/gui/GtfsValidatorApp.java
Outdated
Show resolved
Hide resolved
app/gui/src/main/java/org/mobilitydata/gtfsvalidator/app/gui/GtfsValidatorApp.java
Outdated
Show resolved
Hide resolved
app/gui/src/main/java/org/mobilitydata/gtfsvalidator/app/gui/GtfsValidatorApp.java
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.
When I run this on Windows 10 Home with JDK 11, I see the UI and can click on "Choose local file" to select a GTFS file, but when I click on "Validate" I get this error:
May 17, 2022 10:35:01 AM org.mobilitydata.gtfsvalidator.app.gui.Main main
INFO: gtfs-validator: start
May 17, 2022 10:35:01 AM org.mobilitydata.gtfsvalidator.app.gui.Main main
INFO: gtfs-validator: exit
May 17, 2022 10:36:19 AM org.mobilitydata.gtfsvalidator.runner.ValidationRunner run
INFO: validation config:
ValidationRunnerConfig{gtfsSource=file:///C:/git-projects/gtfs-validator/tma-tn-us.zip, outputDirectory=C:\Users\sjbar\GtfsValidator, storageDirectory=Optional.empty, validationReportFileName=report.json, systemErrorsReportFileName=system_errors.json, numThreads=1, countryCode=ZZ, prettyJson=true}
May 17, 2022 10:36:19 AM org.mobilitydata.gtfsvalidator.runner.ValidationRunner run
INFO: validators:
Single-entity validators
GtfsFrequency: GtfsFrequencyEndRangeValidator
GtfsCalendar: GtfsCalendarEndRangeValidator
GtfsShape: GtfsShapeLatLonValidator
GtfsStopTime: GtfsStopTimeEndRangeValidator
GtfsAttribution: AttributionWithoutRoleValidator
GtfsFeedInfo: FeedExpirationDateValidator FeedServiceDateValidator GtfsFeedInfoEndRangeValidator
GtfsStop: GtfsStopLatLonValidator LocationTypeSingleEntityValidator StopNameValidator
GtfsPathway: PathwayLoopValidator
GtfsRoute: RouteColorContrastValidator RouteNameValidator
Single-file validators
GtfsStopTableContainer: ParentLocationTypeValidator
GtfsShapeTableContainer: ShapeIncreasingDistanceValidator
GtfsFareRuleTableContainer: DuplicateFareRuleZoneIdFieldsValidator
GtfsFrequencyTableContainer: OverlappingFrequencyValidator
GtfsRouteTableContainer: DuplicateRouteNameValidator
GtfsAgencyTableContainer: AgencyConsistencyValidator
GtfsStopTimeTableContainer: MissingTripEdgeValidator StopTimeArrivalAndDepartureTimeValidator StopTimeIncreasingDistanceValidator TimepointTimeValidator
Multi-file validators
BlockTripsWithOverlappingStopTimesValidator GtfsAttributionAgencyIdForeignKeyValidator GtfsAttributionRouteIdForeignKeyValidator GtfsAttributionTripIdForeignKeyValidator GtfsFareAttributeAgencyIdForeignKeyValidator GtfsFareRuleContainsIdForeignKeyValidator GtfsFareRuleDestinationIdForeignKeyValidator GtfsFareRuleFareIdForeignKeyValidator GtfsFareRuleOriginIdForeignKeyValidator GtfsFareRuleRouteIdForeignKeyValidator GtfsFrequencyTripIdForeignKeyValidator GtfsPathwayFromStopIdForeignKeyValidator GtfsPathwayToStopIdForeignKeyValidator GtfsRouteAgencyIdForeignKeyValidator GtfsStopLevelIdForeignKeyValidator GtfsStopParentStationForeignKeyValidator GtfsStopTimeStopIdForeignKeyValidator GtfsStopTimeTripIdForeignKeyValidator GtfsTransferFromStopIdForeignKeyValidator GtfsTransferToStopIdForeignKeyValidator GtfsTripRouteIdForeignKeyValidator GtfsTripServiceIdForeignKeyValidator GtfsTripShapeIdForeignKeyValidator LocationHasStopTimesValidator MatchingFeedAndAgencyLangValidator MissingCalendarAndCalendarDateValidator MissingLevelIdValidator PathwayDanglingGenericNodeValidator PathwayEndpointTypeValidator PathwayReachableLocationValidator ShapeToStopMatchingValidator ShapeUsageValidator StopTimeTravelSpeedValidator StopZoneIdValidator TranslationFieldAndReferenceValidator TripAgencyIdValidator TripUsabilityValidator TripUsageValidator UrlConsistencyValidator
May 17, 2022 10:36:19 AM org.mobilitydata.gtfsvalidator.app.gui.ValidationDisplay handleError
SEVERE: Error running validation
java.nio.file.InvalidPathException: Illegal char <:> at index 2: /C:/git-projects/gtfs-validator/tma-tn-us.zip
at java.base/sun.nio.fs.WindowsPathParser.normalize(WindowsPathParser.java:182)
at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:153)
at java.base/sun.nio.fs.WindowsPathParser.parse(WindowsPathParser.java:77)
at java.base/sun.nio.fs.WindowsPath.parse(WindowsPath.java:92)
at java.base/sun.nio.fs.WindowsFileSystem.getPath(WindowsFileSystem.java:229)
at java.base/java.nio.file.Path.of(Path.java:147)
at java.base/java.nio.file.Paths.get(Paths.java:69)
at org.mobilitydata.gtfsvalidator.runner.ValidationRunner.createGtfsInput(ValidationRunner.java:227)
at org.mobilitydata.gtfsvalidator.runner.ValidationRunner.run(ValidationRunner.java:80)
at org.mobilitydata.gtfsvalidator.app.gui.MonitoredValidationRunner.lambda$run$0(MonitoredValidationRunner.java:39)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
at java.base/java.lang.Thread.run(Thread.java:834)
app/gui/src/main/java/org/mobilitydata/gtfsvalidator/app/gui/GtfsValidatorApp.java
Outdated
Show resolved
Hide resolved
This same error actually happens with CLI validation as well, with parameters
We should probably add additional unit tests to check path parsing on Windows, or set up one of the end-to-end CI tests to run on Windows as well (or maybe both). |
Here's what the raw text looks like in the GTFS input text box after selecting the file:
|
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've reviewed everything now. This is a great PR, thank you @bdferris-v2! A few more comments before approval.
Also, I tested the app on Mac OS and it worked perfectly for both the local ZIP file and the URL.
app/gui/src/main/java/org/mobilitydata/gtfsvalidator/app/gui/Main.java
Outdated
Show resolved
Hide resolved
package org.mobilitydata.gtfsvalidator.app.gui; | ||
|
||
import com.google.common.flogger.FluentLogger; | ||
import java.awt.*; |
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.
import java.awt.*; | |
import java.awt.Desktop; |
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.
@bdferris-v2 There is a IntelliJ Google Java Format style file linked in this section which should keep you from fighting the IDE:
https://github.com/google/google-java-format#intellij-android-studio-and-other-jetbrains-ides
It's strange that the CI task to check code format doesn't catch import wildcards. 🤷
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'm actually already using that plugin, but it doesn't seem to affect imports. I went and tweaked my Intellij imports settings and it seems to be working better now.
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.
For imports I believe you need to set the style using https://raw.githubusercontent.com/google/styleguide/gh-pages/intellij-java-google-style.xml - the plugin alone won't do it.
app/gui/src/main/java/org/mobilitydata/gtfsvalidator/app/gui/ValidationDisplay.java
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 HTML output, the parameter definition is slightly more descriptive, so most users wouldn't need to go to the USAGE.md file to understand them.
In the "Advanced section", could we have matching descriptions? I added them in line.
Also: are we going to add the other parameters as discussed in the design document? (validation report name, systems errors report name, HTML report name, and export notice schema?)
app/gui/src/main/java/org/mobilitydata/gtfsvalidator/app/gui/GtfsValidatorApp.java
Outdated
Show resolved
Hide resolved
app/gui/src/main/java/org/mobilitydata/gtfsvalidator/app/gui/GtfsValidatorApp.java
Outdated
Show resolved
Hide resolved
Sean, I believe the validation exception you are seeing on Windows is fixed with ed9c7a3. I was able to repro the issue myself. |
@isabelle-dr I made the text changes to the existing fields. For the other proposed advanced options, I have the following thoughts:
|
Thanks @bdferris-v2! Confirmed, it's fixed now on my machine. I tested with a directory with spaces as well and that works too. Not sure it's related to this PR, but I'm noticing that in the HTML report the input and output paths aren't formatted the same way: |
I opened PR #1155 to add a CI end-to-end check on Windows as well. |
Whoops, sorry, I didn't mean to merge master branch into this PR (hit the button on the wrong tab). But I guess it needed to happen anyway... |
@bdferris-v2 Re: the advanced parameters. Additionally, the users that would export the notice schema for parsing would probably use the validator with the command line where they have access to this parameter. Thanks for updating the descriptions. |
Oh, right. This is because the GTFS Input is a |
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.
Other than this, I think we are ready to merge. Thank you @bdferris-v2 !
Cool. I can commit to a fast-follow to address the path display issue on Windows. Thanks! |
Apologies @maximearmstrong I missed that import issue. Let me fix that as well. |
Provides initial implementation for the simple GUI to be used with the packaged validation app. Details of the UI product requirements and proposed implementation are documented in https://bit.ly/gtfs-validator-packaged-exe
.