Add road-following routing to the maps API - #5480
Open
shai-almog wants to merge 11 commits into
Open
Conversation
MapContainer.addPath and MapSurface.addPolyline join the coordinates they are given with straight segments; they know nothing about roads. Getting a line that follows the road network needs a routing service to work out the geometry first, and the maps API had none. Add com.codename1.maps.routing: * Routing - the entry point. showRoute(map, origin, destination) fetches the route, draws it and frames the camera; findRoute(...) hands back the result for styling and UI. * RouteRequest / TravelMode - waypoints, driving/walking/cycling, alternatives and a steps toggle. * Route / RouteLeg / RouteStep - drawable geometry (toPolyline()), bounds, total distance and duration, and the turn-by-turn breakdown. * RouteService / RouteCallback - the SPI. Routing.setService(...) swaps in Google Directions, Mapbox, GraphHopper or an in-house server without app code changing. * OsrmRouteService - the keyless default routing over OpenStreetMap data, mirroring how OpenFreeMap is the keyless tile default. Its default endpoint is OSRM's public demo server, so the docs are explicit that production apps should point it at their own instance. Also add PolylineCodec and Polyline.fromEncoded(...) for the encoded-polyline format (precision 5 and polyline6), which every directions API returns, so a geometry fetched by hand is one call from being drawable. Polyline's class doc now says outright that it draws straight segments and points at routing. The developer guide gains a "Routes that follow the road" section with include-backed snippets covering the zero-config call, the callback form and pointing the service at a self-hosted endpoint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 434533f1b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds first-class road-following routing support to the modern com.codename1.maps API by introducing a new com.codename1.maps.routing package with a default, keyless OSRM-backed implementation, plus an encoded-polyline codec to turn typical directions geometries into drawable Polylines. It also updates the developer guide with usage examples and adds unit tests for the routing facade, OSRM URL/response handling, and polyline encoding/decoding.
Changes:
- Introduces
Routingfacade + routing model/SPI (Route*,RouteRequest,TravelMode,RouteService,RouteCallback) with a defaultOsrmRouteService. - Adds
PolylineCodecandPolyline.fromEncoded(...)to support encoded polyline (precision 5 and 6) geometries. - Adds developer guide documentation and generated snippets, plus unit tests and SpotBugs exclusions.
Reviewed changes
Copilot reviewed 16 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| maven/core-unittests/src/test/java/com/codename1/maps/routing/MapsRoutingTest.java | Unit tests for routing model, OSRM URL/response parsing, and Routing facade behavior. |
| maven/core-unittests/src/test/java/com/codename1/maps/PolylineCodecTest.java | Unit tests for encoded polyline encode/decode (precision 5/6) and Polyline.fromEncoded. |
| maven/core-unittests/spotbugs-exclude.xml | Adds SpotBugs exclusion for ConnectionRequest subclasses used by routing. |
| docs/developer-guide/Maps.asciidoc | Documents “Routes that follow the road” and shows how to use/configure routing. |
| docs/demos/common/src/main/java/com/codenameone/developerguide/snippets/generated/MapsJava005Snippet.java | Generated snippet: simplest Routing.showRoute(...) usage. |
| docs/demos/common/src/main/java/com/codenameone/developerguide/snippets/generated/MapsJava006Snippet.java | Generated snippet: Routing.findRoute(...) with callback and custom styling/UI updates. |
| docs/demos/common/src/main/java/com/codenameone/developerguide/snippets/generated/MapsJava007Snippet.java | Generated snippet: configuring OsrmRouteService endpoint. |
| CodenameOne/src/com/codename1/maps/routing/TravelMode.java | Adds travel modes and wire IDs for routing backends. |
| CodenameOne/src/com/codename1/maps/routing/Routing.java | Adds routing facade methods (findRoute, showRoute) and default service management. |
| CodenameOne/src/com/codename1/maps/routing/RouteStep.java | Adds immutable step model (instruction, road name, distance/duration, geometry). |
| CodenameOne/src/com/codename1/maps/routing/RouteService.java | Adds routing service SPI contract. |
| CodenameOne/src/com/codename1/maps/routing/RouteRequest.java | Adds request model (origin/destination, waypoints, travel mode, steps, alternatives). |
| CodenameOne/src/com/codename1/maps/routing/RouteLeg.java | Adds leg model containing steps and aggregate metrics. |
| CodenameOne/src/com/codename1/maps/routing/RouteCallback.java | Adds callback interface for async route results/failures. |
| CodenameOne/src/com/codename1/maps/routing/Route.java | Adds route model (geometry, bounds, distance/duration, legs) + toPolyline(). |
| CodenameOne/src/com/codename1/maps/routing/package-info.java | Package-level documentation for routing API and default OSRM behavior. |
| CodenameOne/src/com/codename1/maps/routing/OsrmRouteService.java | Implements OSRM backend: URL construction, response parsing, and async networking. |
| CodenameOne/src/com/codename1/maps/PolylineCodec.java | Implements encoded polyline codec (precision 5/6) with truncation tolerance. |
| CodenameOne/src/com/codename1/maps/Polyline.java | Adds Polyline.fromEncoded(...) factory methods and clarifies straight-line semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Cloudflare Preview
|
Codex and Copilot both caught a real hang, plus three smaller issues: * Deliver the callback on transport failures. The request was marked failSilently, and NetworkManager swallows IOException/RuntimeException outright for silent requests -- handleException() never ran, so a client that lost DNS or TLS waited forever for a callback that never came, breaking the exactly-once contract RouteService advertises. The flag is gone (every failure hook here is overridden, so nothing reaches the default retry dialog anyway), and the request now also watches its own completion event as a backstop, covering a killed request or an app-wide error listener that consumes the event first. * Reject an encoded value cut in half. Exhausting the string mid-value returned the partial accumulator as if it were a complete delta, so dropping the final byte of the Google sample still emitted three points with the last longitude at -121.21 instead of -126.453 -- a spurious segment and skewed route bounds. readValue now reports whether it saw a terminating chunk and the point is dropped when it did not. * Return an unmodifiable view from RouteRequest.getWaypoints(), so a caller cannot slip a null or non-LatLng entry past addWaypoint() and blow up later inside buildUrl(). * Fix the showRoute() javadoc, which claimed the callback could restyle the drawn route. It cannot: that polyline is not exposed and toPolyline() returns a fresh instance each call. The doc now says so and points at findRoute() for styling. Also apply the complete Codename One license header to the six new files that carried a short or missing one, and rework the guide prose the Vale and LanguageTool gates flagged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad93d433fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The previous commit paired the failSilently fix with a per-request PROGRESS_TYPE_COMPLETED listener, meant to cover the one case the fix leaves open: an app-wide error listener that consumes the event, so NetworkManager never reaches req.handleIOException(). That backstop is racy and has to go. NetworkManager fires COMPLETED from its finally block for every attempt, including a redirect hop that is about to be retried (ConnectionRequest sets redirecting = true, calls retry() and returns false). Guarding on isRedirecting() does not help: progress events reach the listener through callSerially, so the check runs on the EDT after the network thread may already have started the retried attempt and reset the flag at the top of performOperationComplete. A redirect would then deliver a spurious failure and latch `delivered`, swallowing the real route that followed. Removing failSilently is the actual fix and stands on its own -- it restores delivery on every path NetworkManager exposes to the request. The remaining gap is ordinary ConnectionRequest behavior shared by the whole framework, so it is documented on RouteConnection rather than worked around with a racy listener. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7b4d0966f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… one
parseResponse() is public and documents IOException for a malformed body,
but a non-object entry in routes/legs/steps escaped through the unchecked
casts, so a caller feeding it a response from its own transport crashed
past the documented catch:
{"code":"Ok","routes":[null]} -> NullPointerException
{"code":"Ok","routes":[7]} -> ClassCastException
Every object cast now goes through requireObject(value, what), which
raises IOException("Malformed routing response: <what> is not an object")
for route, leg and step entries; parseRoute and parseLeg propagate it. The
nested optional fields were already guarded -- maneuver behind
instanceof Map, location behind instanceof List, numerics through
number(), which falls back rather than throwing.
A malformed entry fails the parse rather than being skipped. Silently
dropping a leg or a step from a public parser hides the problem, and a
hard failure is what the documented contract promises. The javadoc now
states that malformed input never escapes as an unchecked exception, so
catching IOException is enough.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Six review findings, all valid:
* The OSRM travel-mode documentation overclaimed. A stock osrm-routed
process serves the single dataset it was prepared with and ignores the
profile path component, so "a self-hosted instance with the matching
profiles honors them properly" was wrong -- one endpoint cannot serve
all three modes. The mode stays in the URL, since OSRM-compatible hosted
services do select on it, but the docs now say one service speaks to one
endpoint and show the per-mode instance pattern. TravelMode no longer
implies the backend honors whatever is asked.
* Narrow the SpotBugs exclusion to the literal
OsrmRouteService$RouteConnection. The ($.*)? form was copied from the
HttpTileSource entry, where the wildcard covers anonymous subclasses;
here it only risked hiding future findings on the service itself.
* Catch Exception rather than Throwable around the parse. An
OutOfMemoryError is a VM problem, not a routing failure, and disguising
it as one buries it.
* Build the failure message from getMessage() with a readable fallback
instead of concatenating the exception, which leaked class names into
text a caller may show a user. The throwable still reaches the callback.
* Always pass the throwable from the parse path. Nulling it for every
IOException conflated "OSRM declined to route" with "the body was
malformed" and discarded the detail worth logging for the second.
RouteCallback documents when error is null rather than that special case.
* Reject a precision outside 1 to 10 in PolylineCodec. Zero or negative
left the scale at 1 and inflated coordinates; a huge one overflowed the
scale and collapsed them to zero, both silently. Validated before the
empty-input short-circuits so the argument is checked regardless of
payload. A range rather than {5, 6} because precision 7 is in real use.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
The quality-report gate rejects UnnecessaryImport, and Routing's reference to Polyline became documentation-only once showRoute stopped naming the type. PMD does not read markdown doc comments, so the import counted as unused. Link the type by its fully qualified name in the doc instead. Worth noting for future changes: `mvn pmd:check` passes on this, which is why it slipped through locally. The gate that fails is .github/scripts/generate-quality-report.py, which parses target/pmd.xml against its own forbidden-rule list. Running it locally over the generated reports reproduces CI exactly, and now reports no findings at all in the new maps sources. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 19 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
CodenameOne/src/com/codename1/maps/routing/OsrmRouteService.java:420
- The
RouteServicecontract requires exactly one callback invocation per request, butRouteConnection.handleException()can be skipped if an app-wideNetworkManagererror listener consumes the exception event. In that caseNetworkManagerdoesn’t call the request’shandleIOException()/handleRuntimeException(), so this request never delivers a callback, which can leave callers waiting indefinitely.
A robust fix is to register a per-request NetworkManager error/progress listener (filtered to this request) that calls failLater(...) on error and unregisters itself on completion, so the routing callback is delivered even when the global error listener consumes the event.
/// One framework-level caveat remains, and it is the same for every
/// `ConnectionRequest`: an app-wide error listener registered through
/// [com.codename1.io.NetworkManager#addErrorListener] that *consumes* the
/// event has taken over failure reporting, and this request never hears
/// about it.
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Seven more review findings (two of them the same point twice): * Reject a null callback instead of returning quietly. RouteService documents exactly one callback per request, and a silent no-op both breaks that and leaves the caller waiting on nothing. There is no useful fire-and-forget here -- the result is the whole point of the call -- so Routing.findRoute and OsrmRouteService.findRoutes both throw IllegalArgumentException, and the SPI states the requirement. * Honor RouteService.isAvailable() in the facade. Its own javadoc promised callers a fast, readable failure instead of a network error, but nothing consulted it. Routing.findRoute now fails the request through the usual callSerially path so an unavailable service behaves like any other failure. * Stop decoding at characters outside the encoded-polyline alphabet. A byte below ASCII_OFFSET makes the chunk negative, and a negative chunk reads as terminating, so junk mid-geometry closed the value early and emitted a confident, wrong coordinate rather than stopping. * Convert RuntimeException from JSONParser into IOException. parseJSON does throw unchecked on some malformed input, so the promise that IOException is all a caller must catch needed the parse wrapped. The cause is kept; ParparVM's IOException carries the (String, Throwable) constructor. * Let PolylineCodec own the default precision: both Polyline.fromEncoded overloads now go through one private helper instead of restating 5. * Fix the RouteRequest javadoc, which credited addWaypoint with blocking non-LatLng entries. The signature does that; addWaypoint filters null, and the unmodifiable view is what stops injection through the getter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f39d045ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Two more review findings: * NativeMap.fitBounds kept the current zoom and only recentered, so a region larger than the viewport stayed clipped and MapSurface#fitBounds meant something different depending on which surface you held -- showRoute framed the route on a vector map and not on a native one. It now solves for the zoom that contains the bounds and clamps it to the provider's range, using the same Web Mercator arithmetic VectorMapEngine.fitBounds already runs, with device pixels converted through the shared MapView.devicePixelRatio(). Native SDKs use the same slippy convention, so the two surfaces finally frame a region identically -- which markers and polygons benefit from as much as routes. A real provider cannot be driven from a unit test, so the arithmetic lives in a pure, package-visible NativeMap.zoomToFit(...) that is tested directly: whole world at 256px is zoom 0, each doubling adds a level, padding and a 2x density ratio each cost what they should, the tighter axis wins, a smaller region out-zooms a larger one, and the degenerate cases return NaN so fitBounds falls back to recentering. * Restore the completion backstop, this time soundly. Dropping setFailSilently fixed delivery for transport failures, but a global error listener that consumes the event still makes NetworkManager skip this request's hooks entirely. The version removed in d7b4d09 tested isRedirecting() from the listener, which races: the check runs on the EDT after the network thread may already have started the next attempt and cleared the flag, so every redirect became a spurious failure. This one counts attempts instead -- outstandingAttempts starts at 1 and retry() raises it, because retry() runs inside performOperationComplete ahead of the completion event for the attempt that scheduled it, so the count is always raised before the event it must outlive. The counter is guarded by the request monitor, since increments come from the network thread and decrements from the EDT. A real result still wins: postResponse is queued to the EDT first and the backstop hops once more before delivering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c28e4d266
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…p gap * NetworkManager.addToQueue validates synchronously (validateImpl at NetworkManager.java:611), and ConnectionRequest.validate throws for a URL that is not HTTP. An ftp:// base URL therefore escaped findRoutes as an unchecked exception instead of reaching routeFailed, and the progress listener registered a line earlier stayed attached for good, pinning the request and its callback to the NetworkManager. start() now catches a synchronous rejection, detaches the listener and delivers the failure the same way as any other. (The neighbouring silent-drop path -- addToQueue returning early for a duplicate -- would hang identically but does not apply: duplicateSupported defaults to true.) * Document the antimeridian limitation on Route.getBounds() rather than patch it in routing. MapBounds normalizes its corners so longitudes always run west to east, so it cannot represent a wrapped span at all and there is nowhere for a shortest-interval result to go: handing back new MapBounds(sw=179.5, ne=-179.5) is normalized straight back to -179.5..179.5. A route crossing the antimeridian consequently reports a box the long way round and frames most of the globe. The fix belongs in MapBounds -- treating sw.lon > ne.lon as wrapped, with matching changes to contains, getCenter, getLongitudeSpan, fromCoordinates and both camera-fit paths -- which predates this PR and affects every marker and polygon, so doing it for routes alone would leave the framework inconsistent. Also give MapsModelTest the complete license header, which editing it now requires. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83811d96a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…metry
* Frame bounds at the projected midpoint rather than the mean of the two
latitudes. Mercator stretches away from the equator, so bounds spanning 0
to 80 degrees are visually centered near 57, not 40; centering on 40 needs
68.18 world-pixels of half-height where the fit budgets 49.63, leaving the
northern edge outside the viewport the zoom just sized for it. This was
not only in the new native path -- VectorMapEngine.fitBounds has always
used bounds.getCenter() -- so fixing NativeMap alone would have re-broken
the surface parity established a commit ago. Both now go through a shared
WebMercator.centerLatitude(south, north). No screenshot golden exercises
fitBounds, so the vector change has no baseline to invalidate.
* Reject a route whose geometry decodes to no coordinates. The request
always sends overview=full, so {"code":"Ok","routes":[{}]} is malformed by
construction, yet it produced a cheerful empty Route -- showRoute would
add an empty polyline and report success, which tells the app nothing went
wrong. One coordinate is enough to accept, since a degenerate route whose
endpoints coincide can legitimately be a single point.
* Guard Routing.findRoute against a service that throws, using a one-shot
wrapper rather than a bare try/catch. Catching alone would introduce a
different bug: a service that reports and then throws would hand the app
two callbacks, trading a missing answer for a duplicate. The wrapper
delivers the first outcome and swallows the rest, and the catch path hops
through callSerially so every touch of the latch is on the EDT.
* Return early from the completion backstop once a result has been
delivered, instead of allocating a Runnable and burning an EDT hop on
every successful request. postResponse is queued ahead of the completion
event, so the flag is reliably set by the time the listener runs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89a5ec08c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
* Clamp before projecting. Mercator runs to infinity at the poles, and centerLatitude averaged the projections directly. The observed failure is not NaN, as one might expect, but something quieter and worse: only one side goes infinite, because tan(PI/2) is merely enormous in doubles rather than infinite, so latToWorldY(90) is -1421.3 while latToWorldY(-90) is Infinity. A pole-to-pole span therefore centered on -90 -- the south pole, not the equator -- as a perfectly plausible-looking latitude that no check would catch. WebMercator now exposes MAX_LATITUDE and clampLatitude, used by centerLatitude and by both span calculations (VectorMapEngine. worldSpanY, NativeMap.zoomToFit), where a pole would otherwise project to infinity and collapse the solved zoom. The inline 85.05112878 literals in panPixels now reference the shared constant. * Detach the progress listener from every terminal path, not just the backstop. EventDispatcher.fireActionSync skips remaining listeners once an event is consumed (EventDispatcher.java:416), so an app-level progress listener registered ahead of this one starves the backstop and the only removeProgressListener call never runs -- leaving one listener per route, each retaining the request and the application callback, accumulating for the life of the app. deliverRoutes, deliverFailure and the queue-rejection path now share a detach() with the backstop, so the listener comes off however the request ends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Review status: all 30 conversations addressed and resolved28 findings were raised by Codex and Copilot across 7 review rounds. Every one has a reply in its thread and is marked resolved. Summary here so the state is readable without scrolling the thread list. Real defects a user would have hit
Contract and robustness hardening
One thing I did not fix, deliberatelyAntimeridian bounds. A route crossing 180° reports a box the long way round, so framing it zooms out to most of the globe. Two other places I kept the finding but not the suggested remedy, both argued in-thread: OSRM per-mode endpoints (fixed as documentation plus a per-instance pattern rather than a mode-to-URL map inside one backend), and the Verification on
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f9bac6151
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+435
to
+439
| /// That still leaves one path where nothing would be delivered: an | ||
| /// app-wide error listener registered through | ||
| /// [com.codename1.io.NetworkManager#addErrorListener] that *consumes* the | ||
| /// event makes `NetworkManager` skip this request's hooks entirely. A | ||
| /// completion listener closes that hole -- see [#actionPerformed]. |
There was a problem hiding this comment.
When an application has both a global error listener that consumes a transport exception and an earlier progress listener that consumes PROGRESS_TYPE_COMPLETED, NetworkManager skips the request's failure hooks and EventDispatcher skips this completion listener. Neither path then calls the callback, contradicting the exactly-once contract and leaving the listener attached; use a completion signal that cannot be suppressed by another global progress listener.
Useful? React with 👍 / 👎.
| } | ||
| RouteService routeService = getService(); | ||
| final OnceOnly guarded = new OnceOnly(callback); | ||
| if (!routeService.isAvailable()) { |
There was a problem hiding this comment.
When an installed third-party service throws from isAvailable()—for example while checking configuration or initializing a provider—the exception escapes synchronously here because only findRoutes() is inside the guarded try. Routing.findRoute() therefore provides no callback despite promising exactly one asynchronous outcome; include readiness and identifier probing in the same guarded failure path.
Useful? React with 👍 / 👎.
Comment on lines
+132
to
+136
| if (request == null || request.getOrigin() == null || request.getDestination() == null) { | ||
| fail(callback, "A route request needs both an origin and a destination", null); | ||
| return; | ||
| } | ||
| RouteConnection req = new RouteConnection(callback); |
Comment on lines
+163
to
+165
| /// Exposed so an app that fetches from an OSRM-compatible endpoint through | ||
| /// its own transport (a proxy, a cached response, a bundled fixture) can | ||
| /// reuse the same parsing. |
This file contains hidden or 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
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.
Fixes #5478.
The report
MapContainer.addPathdraws a straight line between the coordinates instead of the route a driver would take.That is what a polyline does —
addPath/MapSurface.addPolylinejoins the vertices it is given with straight segments and has no knowledge of roads. Turning two endpoints into road geometry is a separate job, done by a routing service, and the maps API had none. (MapContaineris the deprecatedcodenameone-google-mapscn1lib; the guide already points at the modernMapView/NativeMap, so the fix lands there.)What this adds
A new
com.codename1.maps.routingpackage:Routing— the entry point.Routing.showRoute(map, origin, destination)fetches the route, draws it and frames the camera.findRoute(...)hands back the result when you want to style the line or show distance/ETA.RouteRequest/TravelMode— waypoints,DRIVING/WALKING/CYCLING, alternatives, and a steps toggle.Route/RouteLeg/RouteStep— drawable geometry (toPolyline()),getBounds()forfitBounds, total distance and duration, and the turn-by-turn breakdown with generated instruction text.RouteService/RouteCallback— the SPI.Routing.setService(...)swaps in Google Directions, Mapbox, GraphHopper or an in-house server without app code changing.OsrmRouteService— the keyless default, routing over OpenStreetMap data. This mirrors how OpenFreeMap is the keyless tile default: a road route works in the simulator and on device with no API key and no signup.Plus
PolylineCodecandPolyline.fromEncoded(...)for the encoded-polyline format (precision 5 andpolyline6) that every directions API returns, so a geometry you fetched yourself is one call from being drawable.Polyline's class doc now states outright that it draws straight segments, and points at routing.The reporter's case becomes:
On the default endpoint
OsrmRouteServicedefaults to OSRM's public demo server: no SLA, rate limited, and it hosts the car profile so walking/cycling requests route over driving data there. The class docs, the package docs and the developer guide all say this explicitly and show the one-line switch to a self-hosted (or any OSRM-compatible) endpoint:Docs
New "Routes that follow the road" section in
docs/developer-guide/Maps.asciidoc, with three include-backed snippets: the zero-config call, the callback form, and pointing the service at your own endpoint.Verification
PolylineCodecTest,MapsRoutingTest) covering the codec against the Google specification vector, truncated-input tolerance, OSRM URL construction (lon/lat ordering and fixed-decimal formatting that keeps scientific notation out of the URL), response parsing, service-level error codes, and the facade.total_bugs=0) and PMD clean. OneEQ_DOESNT_OVERRIDE_EQUALSexclusion added for the one-shotConnectionRequestsubclass, mirroring the existingHttpTileSourceentry.🤖 Generated with Claude Code