Revert voice hint reindexing #594
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a9e8731 made voice hints available from `formatAsGeoJson()`, which is used both in the GeoJSON HTTP API and in the JSON Java API. To indicate a specific type of voice hint, it was chosen to include its numeric id in the output JSON array among other data. The full list of available ids was defined in `class VoiceHint`, e.g. `static final int C = 1;`. Consumers of the API now depended on the mapping from id to intended voice hint not changing, since otherwise incorrect voice hints could be displayed. Unfortunately that API contract was broken in c9ae7c8, where instead of assigning unused ids to new commands, the meaning of existing ids was changed. This broke compatibility: Clients adapted to the change did not work with the old indexing anymore, and clients not yet adapted would break with newer BRouter releases, e.g. they would suddenly display "Off route" for a "right u-turn". To restore compatibility, the indexing is reverted to its old state. This will unbreak GeoJSON/JSON API users no yet adapted to BRouter 1.7.0 or 1.7.1, e.g. BRouter-Web as well as unmaintained clients. While API users which already patched ids would need to undo or special-case their changes, the impact is believed to be low, as no such users are currently known and the breakage was released only recently. The changed meaning of `TU` in output formats (before: `u-turn-left`, now: `u-turn-180`) has not been reverted for now, since either that command is mapped to fallback solutions anyway (e.g. Orux, old Locus, Gpsies), the change has already been implemented in clients (new Locus, Cruiser) or was only planned to be implemented in the future (OsmAnd). Fixes #584 Test Plan: - `./gradlew test` - Run BRouter with an unpatched BRouter-Web and confirm voice hint ids have been restored to the same ones as emitted by BRouter 1.6.3.
As c9ae7c8 showed, changing internal ids without being aware of the possible impact might easily lead to break the external API. While ids could be fixated by adding respective tests, an even more elegant solution is to make the mapping from internal ids to external ids explicit, similar how it is already done for other voice hint formats. To underline the purpose of the mapping even more, the respective method is renamed appropriately. Test Plan: - `./gradlew test` - Export a complex route in BRouter-Web and check voice hints have not been changed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Split out from #591 as requested.
See comments there and commit messages for why we need this.