The Android location button, and the one thing that still will not carry it (#5667) - #5684
Open
shai-almog wants to merge 82 commits into
Open
The Android location button, and the one thing that still will not carry it (#5667)#5684shai-almog wants to merge 82 commits into
shai-almog wants to merge 82 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 143ad6fa22
ℹ️ 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".
shai-almog
force-pushed
the
location-button-android17
branch
from
September 3, 2026 11:28
143ad6f to
6a3f4e6
Compare
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a3f4e6aee
ℹ️ 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".
shai-almog
force-pushed
the
location-button-android17
branch
from
September 3, 2026 11:34
6a3f4e6 to
12606dd
Compare
Contributor
Cloudflare Preview
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12606ddacb
ℹ️ 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: |
shai-almog
force-pushed
the
location-button-android17
branch
from
September 3, 2026 11:52
12606dd to
f0547d1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0547d10bf
ℹ️ 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 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. |
shai-almog
force-pushed
the
location-button-android17
branch
from
September 3, 2026 14:11
f0547d1 to
88985db
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88985dbbb1
ℹ️ 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".
shai-almog
force-pushed
the
location-button-android17
branch
from
September 3, 2026 15:45
88985db to
b8344db
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8344dbdc8
ℹ️ 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".
shai-almog
force-pushed
the
location-button-android17
branch
from
September 3, 2026 16:27
b8344db to
bb507bf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb507bfb2b
ℹ️ 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".
shai-almog
force-pushed
the
location-button-android17
branch
from
September 3, 2026 17:37
bb507bf to
22570ff
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22570ff425
ℹ️ 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 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
shai-almog
force-pushed
the
location-button-android17
branch
from
September 4, 2026 12:46
22570ff to
8348671
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8348671f6a
ℹ️ 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".
shai-almog
force-pushed
the
location-button-android17
branch
from
September 4, 2026 13:03
8348671 to
fe09952
Compare
The whole of this gesture is that the tap earns precise location for the session. It was reporting the fix taken BEFORE it. getCurrentLocationSync hands back the CACHED location whenever a listener is installed, and `listener` is static -- shared by every LocationManager in the process -- so an application that installed one anywhere sent even the bridge's freshly granted manager down that path. On an application that until then held only the approximate grant, the cache holds an approximate fix. The control appeared to work and answered with exactly the accuracy the user had just agreed to improve. The obvious fix is unsafe, which is why the delegation is there in the first place: LL.bind calls setLocationListener, so starting a fresh acquisition would replace the application's own listener and silently end its tracking. freshLocationSync waits for the next fix that listener delivers instead -- the first one taken under the new grant -- and recognises it by timestamp. It registers nothing and clears nothing, so the application's listener is untouched. When nothing newer arrives before the deadline it returns the cached fix rather than null, because a listener registered at a slow interval may have nothing for minutes and a slightly imprecise answer beats an empty one: this is never worse than the call it replaces. Package private, and LocationButton is in this package, so no public API grows for it. A public "give me a fix ignoring the cache" would promise more than this keeps. The test fails with expected <2000> but was <1000> when the call site goes back to getCurrentLocationSync -- the pre-grant fix, reported for the tap that was meant to improve on it. 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: 48b4127f14
ℹ️ 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".
…ve up on an empty one Both halves of a P1 on the method the previous commit added, and both are in that method rather than around it. IT FROZE THE EDT. getCurrentLocation is not the cheap accessor its name suggests: on Android with Play Services it reaches getLastKnownLocation, which spins `while (!client.isConnected()) sleep(300)` with no bound at all. The baseline read ran that on the EDT, before invokeAndBlock -- so the whole application stopped for as long as that client took to connect, and the caller's timeout could not bound a wait that had not started. Every read is inside the block now, the baseline included. IT REPORTED NOTHING FOR AN EMPTY CACHE. That same manager THROWS when it holds no fix, and the outer catch turned the throw into an immediate null -- to a caller whose next update was moments away, which is the one thing this method exists to wait for. An empty cache is not a failure: it means the application has no fix at all, so the first one to arrive was taken after the grant and is the answer rather than a baseline to improve on. Both are covered and both were checked by breaking them. Restoring the abort fails the empty-cache test with "expected: not <null>"; moving the baseline read back to the EDT fails the thread assertion in the freshness test with "expected: <false> but was: <true>" -- the test asks the manager which thread its first read arrived on, because that is the property, not a timing. What this does NOT claim to fix is a single read that blocks past the deadline inside the block. Nothing in core can interrupt a port that sleeps, and the timeout bounds the polling rather than any one call into the platform. The EDT is free either way, which is what the freeze was about. 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: e667f30716
ℹ️ 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".
…cation caller The ceiling the previous commit documented, removed at its source rather than worked around in core. AndroidLocationPlayServiceManager.getLastKnownLocation spins `while (!client.isConnected()) Util.sleep(300)` with no bound, and the client does not always connect -- Play Services can be absent, disabled or out of date on the device. getCurrentLocation calls it, so on such a device EVERY caller of getCurrentLocation waits for ever, the location button's tap among them: its listener is never called and the timeout the application set can never fire, because there is no deadline on a wait that has not ended. Bounding it in core was the alternative and it is the wrong half of the problem. Core cannot interrupt a port that sleeps without running the read on a thread of its own and abandoning it, which is cross-thread state in a framework that is single threaded by design -- and it would leave every other caller of getCurrentLocation hanging exactly as before. There are seven of these loops in the class and they are not equivalent. Six sit inside threads of their own that nothing joins, so a client that never connects costs that thread and nothing else; this one runs on the CALLER's thread and hands back a value. Only it is bounded here, and the javadoc says why, so the next person does not read the other six as an oversight. Ten seconds, because a real connect is quick and this is only reached when one is not happening at all. Expiry returns null, which is the answer this method already documents for having no fix and which getCurrentLocation turns into the same "cannot retrieve location try later" it raises for an empty cache -- freshLocationSync then treats it as no cache and goes on waiting for the fix the listener will deliver. The file is CRLF and stays CRLF: the change is 37 added lines rather than a whole-file rewrite. 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: 3796ae56df
ℹ️ 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".
An import the compiler emits nothing for was enough to turn the whole feature on. The two scans in this file want opposite things from an ambiguous file and this one had the wrong direction. Naming a location PROVIDER without evidently calling it is evidence toward refusing an exclusivity request, and over-reporting there costs the build a sentence naming the conflict. Naming the BUTTON costs the application the feature's entire manifest contribution -- and since the toolchain fallback landed, that contribution is the only thing that goes in: fine and coarse location, injected into an application that asks for location nowhere, with a Play data-safety declaration to answer for them. So an application carrying a leftover `import com.codename1.location.LocationButton` shipped asking for precise location over a line that compiles to nothing. The button's question now has its own reader. An import on its own is not use; everything else still is -- a qualified reference in code, an import the simple name goes on to use, a wildcard import of the package, the file that declares it. sourceNames is untouched, so the exclusivity scans keep their direction, and a test asserts they still over-report on the same shape of file. Two imports are use by themselves, because nothing else has to spell the name for them: a Kotlin alias import renames it, and a static import takes a member off it. Without that carve-out the tightening loses the button from a file that plainly builds one -- which the alias test catches, and did. Both halves were checked by breaking them. The old import-blind check fails the leftover-import test; dropping the alias escape fails the Kotlin one. 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: 9025b2ae0f
ℹ️ 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".
…ng uses
Three scanner findings, and two answers written into the code for findings
whose fix costs more than the defect.
WHITESPACE. The wildcard-import and package-declaration checks were
`indexOf("import " + pkg + ".*")` -- one literal space. Java separates a
keyword from its operand with any whitespace it likes, so a tab or a newline
put the package out of scope, and both consequences are the silent kind: the
simple name is then unresolved, so the button is deleted from an application
that builds one, and a provider call goes unseen by the exclusivity scan, which
accepts the hint over a request the application really makes.
META-INF/versions. A multi-release jar keeps per-version copies there and
Android loads none of them -- packaging drops the tree -- so a class that
exists only under it runs nowhere on the device. Counting it charged an
application fine and coarse location for a reference its device can never
reach. That is not the narrowing this file argues against; it is the documented
exception, where the code provably does not run. module-info.class goes with
it, and a test asserts the same class in the base tree still counts.
UNUSED ALIAS AND STATIC IMPORTS. The carve-out added with the unused-import fix
was unconditional, which put the permissions back for exactly the import that
fix exists to ignore. The name an import BINDS has to be used: the alias for
`import ... as Btn`, the member for a static import, the simple name otherwise.
A static wildcard binds every member and names none, so it stays evidence.
Two of these tests failed on the first attempt because the method's
package-in-scope fallback then found the import line itself and reported use
again -- recording those lines as import-only is what fixes it.
Two findings are answered rather than changed, in the code, because the review
thread is not where the next person will look.
The port's connect cap is its own and knows nothing of the caller's deadline,
so a read already in progress can overrun it: a one-second timeout can wait
about ten. Closing that means either handing the port a budget -- API on
LocationManager for a single caller -- or running the read on a thread core can
abandon, which is cross-thread state in a framework that is single threaded by
design. The residual is now stated exactly where the loop is.
And markTimedOut's note claimed setLocationListener clears and binds "within
that ONE port call, which is the ordering the port controls". That is wrong:
clearListener and bindListener each spawn a thread that waits for the API
client, so they are not ordered, which is the hazard the port documents at
AndroidLocationPlayServiceManager.clearListener along with the two patches for
it that were withdrawn. The note now says so, and states the trade this makes
rather than implying there is none: forgetting the field orphans the platform
subscription certainly and for good, while keeping it risks one retry timing
out, transiently, and only when the client is down as the clear starts.
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: 8e0c9e15bc
ℹ️ 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".
Two findings on the import reader added a commit ago, and they are the same
bug twice, so this replaces the reader rather than patching the two spellings.
It worked a line at a time. Both ways that is wrong are MISSES, and a miss here
deletes the bridge from an application that builds a button and omits the
permissions its fallback needs:
import com.codename1.location.LocationButton; class D { ... }
is one legal line, and skipping the whole line as an import made every
reference after the semicolon invisible. And
import com.codename1.location.LocationButton as<TAB>Btn
is legal Kotlin, where a literal " as " search found no alias -- while the code
spells only the alias, so the file read as not using the class at all.
An import statement now runs from its keyword to the first ';' or the end of
its line, whichever comes first, which is Java and Kotlin both; evidence is
looked for outside those ranges; and the bound name is TOKENISED out of the
statement rather than pattern-matched, so whitespace inside it cannot hide the
alias. A Java import split across lines before its semicolon reads as ending at
the newline and so over-reports rather than missing, which is the safer of the
two, and no formatter writes one -- said in the javadoc rather than left to be
discovered.
A static wildcard now counts as use. It binds every member of the class and
names none of them, so there is nothing to look for, and the previous answer --
fall back to the simple name, find nothing, report unused -- deleted the bridge
from an application that builds a button.
Three helpers the line reader needed are gone with it rather than left for
SpotBugs to report as uncalled.
Both halves were checked by breaking them: ending the range at the line fails
the same-line test, and restoring the literal " as " fails the tab one.
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: a39b331b8f
ℹ️ 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".
…d, a throttle never woke
Four findings. Three are fixed; the fourth is stated in the code, where the
next person will look.
THE OVERSIZED-FILE PATH READ PROSE. A source past the scan budget cannot be
held in memory, so it was searched with a raw indexOf -- justified in a comment
by the claim that over-reporting "refuses a build with a reason". That stopped
being true when an unsupported toolchain started falling back instead of
refusing: over-reporting now puts fine and coarse location into an application
that asks for location nowhere. So the streaming search became a real one --
comments and literals tracked a character at a time, token boundaries, and a
rolling tail of code so a literal that IS a loader's argument still counts,
which is the rule the buffered path applies. A comment, a diagnostic string and
LocationButtonHelper all stop counting; Class.forName still counts.
A KOTLIN RAW STRING HID A LOAD.
Class.forName("""com.codename1.location.LocationButton""") reads as an empty
literal to a parser that takes the second quote for the end of the string, so
the only reference in a native source went unseen and the bridge was deleted
from an application that builds a button. The test was written first and failed
against the old parser, which is how the premise was checked rather than
assumed.
THE STALE-REBUILD THROTTLE NEVER WOKE. It returned quietly, and that layout can
be the last one: a second recreation lands inside the first's window, nothing
afterwards has to lay the component out again, and the control stays tied to a
retired session for good -- the exact state this check exists to end. It now
schedules its own retry for the rest of the window, through UITimer rather than
java.util.Timer, whose non-daemon thread keeps a desktop JVM alive after the
app is done.
And the one that is not fixed. freshLocationSync takes its baseline AFTER the
grant, so a listener that delivers the first post-grant fix in that instant
makes it the baseline and the loop waits for a later one -- the caller then
waits out its deadline before getting back the very fix it wanted. Slow rather
than wrong. Curing it wants the cache as it stood BEFORE the grant, which
nothing on this side knows: the component hears about the grant only when the
platform reports it, and snapshotting when the control is built runs the same
potentially long read for every button on every form, tapped or not. That costs
more than the wait it saves, and the reasoning now sits beside the baseline.
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: 9e4fe06b67
ℹ️ 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".
…mport is not use Two findings, one of them arguing for LESS evidence in the exclusivity scan -- normally the direction that fails silently -- so its factual claim was checked before anything moved. It was right. The bytecode side asks constructs(pool, MAP_COMPONENT_CLASS), deliberately: what makes MapComponent persistent location use is the lookup its constructor makes to centre itself, so a field of that type, a parameter or an import does none of it. The source side asked sourceNames, and the comment above it claimed the two mirrored each other -- which was true of the other wrappers, counted by referencesClass, and false of this one. A native source holding a MapComponent field therefore refused an exclusive build over a declaration that asks the platform for nothing, and the developer could do nothing about it. The source side now tests for construction: the name followed by an open parenthesis, which is new MapComponent(...) in Java and MapComponent(...) in Kotlin and is neither a declaration nor a type reference. The other wrappers keep the naming test, and a test pins each side of that difference so the next person does not "fix" the asymmetry back out. And the streamed scan learned the import rule the buffered pass already had. An oversized file whose only mention is an import -- or an unused Kotlin alias -- no longer counts, which on the fallback toolchain is the difference between an app that ships asking for precise location and one that does not. One pass is enough because both languages put imports above the code that uses them, so a name bound in one is seen before the code that spells it. Both were checked by breaking them: counting MapComponent by name again fails the field test, and dropping the import state fails the two streamed ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A wildcard import of the package, and a file that declares it, both let the code write the bare simple name -- and the streamed matcher was looking only for the full dotted name or a name some import had bound. So import com.codename1.location.*; ... new LocationButton() read as no button at all in a file too large to hold in memory, and the builder then deletes the bridge package and omits the permissions its fallback needs from an application that plainly builds one. A file declaring package com.codename1.location missed the same way. Both put the simple name in scope now, which is the rule the buffered pass has always applied; the streamed pass simply had half of it. The wildcard is recognised from the statement rather than from the target, because the statement never contains the target -- that is exactly why the existing check could not see it. A negative test goes with them: another package's wildcard is not this package's scope. Without it the rule would charge every application that imports anything with a star. Each branch was checked by breaking it on its own, since they are independent paths through the same state: disabling the package branch fails only the package test, disabling the wildcard branch fails only the wildcard one. 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: f297f07338
ℹ️ 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".
…ream lost Two review findings and one this branch found by auditing itself. A SHADOWED SIMPLE NAME IS NOT OURS. A wildcard import of the package puts the simple name in scope, and an explicit "import com.acme.widgets.LocationButton" beside it takes that name back -- Java resolves the bare token to the explicit import, so the file never touches this component. Reading every such token as the button charged an application that uses none of it with fine and coarse location. A type of that name declared in the file does the same, and both are now checked before the wildcard or package scope is accepted. The negative goes with them: an unrelated import shadows nothing, or every wildcard beside any other import would stop counting. THE SDK-23 ELEMENT NEEDED ITS OWN OVERRIDE. uses-permission and uses-permission-sdk-23 are distinct keys to the merger -- this file relies on that in several places already -- and the merger unions attributes WITHIN a key. A Gradle-resolved aar contributing the sdk-23 shape merges after this runs and is never scanned here, so with nothing of ours under that key its declaration arrived unflagged and an exclusive build shipped ordinary precise-location access. Exclusivity now declares that shape too, flagged, so the union has something to meet. Only under the hint, so no other build changes shape. That changes what the block looks like, and four tests counted the permission NAME across the whole of it -- which stopped being a count of one element type. Each was checked as a counting artifact rather than real duplication before being scoped to the ordinary element. AND A JOIN THE STREAMED PASS LOST, found by enumerating the buffered pass's rules against it rather than by waiting for another report. The buffered one folds a loader argument across '+', so "com.codename1.location." + "LocationButton" is the button; the streamed one saw two literals, neither of them the name, and an oversized file lost a load the small one keeps. It folds now. The first attempt did not: it ended the join on the opening quote of the second piece, so a two-piece join never survived itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…stream behind the buffer Three findings. One is review's, one is review's and corrects a claim written here, and one this branch found by auditing itself against its own last change. A QUALIFIED NAME MAY BE BROKEN AT A NEWLINE, in an import and in code alike, and every search here looks for the name as one run of characters -- so the break hid it completely: neither half is the class, the simple name below then belonged to no import, and the file read as not using the button at all. The bridge went from an application that builds one. The note here called that an over-report and "the safer direction of the two". That was backwards, which is why it sat unnoticed: it is a miss. An import statement now continues across a newline while it is plainly unfinished -- a dot on either side of the break says so -- and qualified names are joined before any of the analysis, so the same break in ordinary code is covered by the same fix. Only whitespace that touches a dot is removed, which changes no expression and cannot join two names that were separate. A TYPE PARAMETER OWNS THE NAME INSIDE ITS OWN TYPE. class Box<LocationButton> binds it for the whole body, so the field is the parameter and not this component -- and charging for it puts fine and coarse location into an application that never touches the button. Only a parameter LIST counts, the one that follows a type's own name: List<LocationButton> elsewhere is a real use, and a test pins that, because reading it as a shadow would lose every file that holds the button in a collection. AND THE STREAMED PASS WAS BEHIND THE BUFFERED ONE AGAIN -- on shadowing, which the last commit added to the buffered side only. Found by writing the test against this branch's own just-pushed code and watching it fail. The fix needed a correction of its own: the shadow check fired on OUR import too, since that binds the simple name as well, so the ours-case has to be asked first. 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: 26356ca7ff
ℹ️ 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".
…overreached Four findings -- two from review, two this branch found by auditing itself -- and a test of mine that turned out to prove nothing. THE APPLICATION'S OWN PRECISE-LOCATION CALL WAS COMPUTED AND DROPPED. Only the library scan's copy of that flag reached the exclusivity check, so an application whose own classes call a provider directly -- a native jar merged into them, which is the shape that reaches that directory -- passed the test, had onlyForLocationButton applied, and got approximate results with nothing in the build to say so. That silent downgrade is the whole thing the check exists to prevent. The field's javadoc is corrected with it: its name described only the older of what are now two sources. A SUBCLASS CARRIES ITS PARENT'S CONSTRUCTOR. class MyMap extends MapComponent plus new MyMap() runs MapComponent's own constructor and the last-known-location lookup in it, and staged sources are compiled later by Gradle so no bytecode scan covers the call either. The DECLARATION is taken as the evidence rather than an instantiation in some other file: that over-reports for a subclass nobody builds, which is this scan's documented direction -- a refusal names its reason, a miss downgrades in silence. SHADOWS REACHED TOO FAR. A class declared inside a method was silencing the simple name across the whole file, so a file that builds the button in one method and declares a local class of that name in another lost the bridge. Brace depth separates them without parsing the language: a top-level type or a member of one reaches the file, anything deeper reaches its own block. Both passes, and the earlier member-level case still shadows. AND THE STREAMED PASS WAS BEHIND AGAIN, on the type-parameter shadow and on names broken at a newline -- both found by testing it against the buffered pass rather than waiting for a report. The streamed shadow test was VACUOUS when written: with the use before the declaration the stream matches and returns early, so it passed whatever the guard did. The probe is what caught it; the source now puts the shadow first. SpotBugs caught one of mine too. Leaving a match alone was written as "matched = matched", which is a self-assignment it reports; the condition is inverted instead. 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: 9f118aafa1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f118aafa1
ℹ️ 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".
…er happened Five findings, and one of them is answered by taking a rule BACK rather than pushing it further. A BUDGET-EXHAUSTED LIBRARY SCAN WAS FAILING THE BUILD. A large enough set of submitted libraries can exhaust the shared entry and byte budget on its own, and those builds worked before this scan existed -- so a feature nobody asked for was breaking projects that never mention the button. It degrades now: what the application's own classes reported still stands, the libraries' contribution is dropped, and the log says which. Under android.locationButton.exclusive it still fails, because there the answer decides whether the application's other location requests are about to be restricted and there is no safe guess. THE J2ME PORT HAD THE SAME UNBOUNDED WAIT the Play Services one did. provider.getLocation(-1) hands the wait to the implementation's default, so a provider that cannot get a fix meant the tap's listener was never called and LocationButton.setTimeout could never fire. Bounded; on expiry the provider throws and the caller already reads that as having no fix. FALLBACK TAPS WERE PAYING FOR A GRANT THEY NEVER GOT. freshLocationSync waits for a fix taken after the system button's grant, which is worth waiting for because the tap earned precise location and the cache predates it. A fallback tap earns nothing -- the permission is whatever it already was -- so every one of them waited out its full timeout to be handed the cache it could have had at once. Those go through the ordinary lookup now. THE STREAMED PASS lost a multiline import, found by testing it against the buffered one rather than waiting for a report. AND THE SHADOW RULE IS NARROWED, WHICH REVERSES TWO EARLIER FINDINGS. Review asked for local-type shadows, then type-parameter shadows, then lexical scoping for both. Each round made the shadow smarter, and a wrong shadow is a MISS: it deletes the bridge from an application that builds a button, silently. Deciding it properly means knowing which enclosing type every reference sits in, which is scope analysis, which is a parser. So the shadow is claimed only where it can be decided without one: an explicit single-type import, which is compilation-unit-wide by the language's own rules, and a TOP-LEVEL type declaration, which takes the name for the whole file. Member types, method-local types and type parameters are all scoped, and the scan over-reports there on purpose. That is the safe direction for this question -- a wrong shadow breaks the feature in silence, a missing one adds a permission the manifest shows and the developer can see -- and it settles the whole class of scoping reports rather than answering them one spelling at a time. The reasoning is on shadowsWholeFile, not only here. 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: cf3733a6a7
ℹ️ 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".
Capping provider.getLocation at ten seconds was wrong, and wrong in the way this branch has been objecting to elsewhere: it changed behaviour for every J2ME caller of getCurrentLocation to serve one feature. A cold GPS fix takes far longer than ten seconds on that hardware -- thirty to sixty is ordinary -- so the cap turned acquisitions that used to succeed into IOExceptions for callers that had asked for no deadline at all. -1 is the provider's own default and it is the right value here. The Android cap this mirrors is NOT the same thing, and the difference is worth stating because the two look alike. There the wait is for the Play Services CLIENT to connect -- infrastructure that either comes up or does not, where ten seconds of nothing means it is not coming -- and it runs on the caller's thread with a value to return. Here the wait is for a FIX, which is a physical measurement that is slow for honest reasons. The unbounded wait that remains on this port is not something the location button introduced: getCurrentLocationSync has always reached getCurrentLocation when a listener is installed, so every J2ME application that calls it already waits exactly this long. The button does not make it worse, and fixing it for everyone means giving the API a deadline it does not currently have -- which is its own change, for its own reasons, not a detail of this one. 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: 49de9f3c0d
ℹ️ 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".
Master had moved 18 commits ahead and the PR was CONFLICTING, which is why its checks had stopped: GitHub could not build the merge ref, so the pull_request workflows were never created at all and only the PR-ref CodeQL run fired. Four green CodeQL jobs are not a green build, and reading them as one is how this went unnoticed for several pushes. Two textual conflicts, both the same shape: the Android source exclusion list, where this branch added com/codename1/impl/android/locationbutton/** and master added com/codename1/impl/android/biometrics/**. Resolved as the union in build.xml and nbproject/project.properties -- both packages are optional, both must stay out of the port jar. Two SILENT conflicts that git merged clean and the suites caught: compileSdkInt gained an eighth parameter on this branch and master added a caller written against the seven-parameter form -- pruneBiometricSourcesForCompileSdk. Text-clean, compile-broken. It gets usesLocationButton like every other caller, because without it that pruning decides against a compile SDK that ignores the location button's own raise. And the cast-semantics baseline was mangled in both directions: the merge dropped master's AndroidImplementation#getLocationManager entry AND four biometrics entries this branch carried, so the gate reported five findings that are neither branch's new debt. Rebuilt as the union of the two baselines -- nothing added that was not already baselined on one side or the other. Verified on the merged tree: 6634 core tests, 2246 plugin tests, SpotBugs zero in both modules from regenerated reports, quality gate, copyright, control characters and cast semantics all clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The note this removes told the reader which Android Gradle plugin the artifact wants, which one we generate, which platforms the build images carry, and that the library "can't be resolved yet". None of that is theirs. It is our migration, written into their documentation, and it made a seamless feature read like a warning. What the reader needs is three sentences: Google recommends this control for one-time location, it works on every platform Codename One targets, and on Android it uses the system control -- automatically, when the build servers get there, with nothing to change in the app. The one genuinely material fact left in is the minimum Android version the control requires, because that is the developer's own compatibility floor. The exclusivity hint keeps its section, since that IS a decision only the developer can make, but loses the paragraph explaining what happens on a toolchain that cannot carry the control. The hint means something where the system control grants precise location and nothing anywhere else, so it is safe to set once and leave set -- which is all they need to know. 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: 4cfd065a25
ℹ️ 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
+34
to
+36
| `LocationButton` is a component you add to a form like any other. On Android 17 | ||
| (API 37) and later it's drawn by the system rather than by the app, and the tap | ||
| is what grants precise location for that session. Everywhere else -- older |
There was a problem hiding this comment.
For every Android build generated by this revision, the guide's API-37 claim is false: AndroidGradleBuilder compares its fixed AGP 8.13.2 against the 9.1.0 minimum at lines 3475–3476, then clears usesLocationButton and builds the ordinary permission-requesting fallback. Consequently an app following this section on Android 17 does not receive the session-scoped system control and may remain subject to the Play policy described below. Document the current toolchain fallback here until the builder can actually emit the AGP 9/API-37 project.
Useful? React with 👍 / 👎.
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
3 participants
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 #5667.
What this adds
com.codename1.location.LocationButton— aContaineryou add to a form. On Android 17 (API 37) it is drawn by the system, so a tap grants precise location for that session only; everywhere else (older Android, iOS, simulator, desktop, browser) it is an ordinary Codename One button that asks for location the usual way. Same code on every port, nothing to branch on.It reaches the platform through
com.codename1.location.spi.LocationButtonBridgeandDisplay, deliberately not throughgetLocationManager()— that asks forACCESS_FINE_LOCATIONbefore it returns anything, and building the button must not ask for the permission the button exists to avoid holding.The Android implementation lives in
com/codename1/impl/android/locationbutton, a package the port jar excludes andAndroidGradleBuilderdeletes for applications that never reference the class — the arrangement ARCore and SQLCipher already use.Build gating
Referencing the class is the whole opt-in:
USE_LOCATION_BUTTON, both standard location permissions declared and uncapped, the AndroidX library,minSdk24, compile SDK 37, the implementation package kept. An application that never names it sees no manifest entry, no dependency, no SDK raise and no package — verified by generatinghellocodenameoneboth ways.usesPermissionFlags="onlyForLocationButton"is never inferred. It is the same question the Play Console asks, and the two ways of getting it wrong are not symmetrical: too permissive costs a declaration, too restrictive silently refuses everygetCurrentLocationSyncon Android 17 only. It comes fromandroid.locationButton.exclusive, and the build refuses that hint alongside geofencing or background location.The toolchain, and why the build refuses
androidx.core.locationbutton:1.0.0-alpha01declaresminCompileSdk 37andminAndroidGradlePluginVersion 9.1.0in its aar metadata. AGP'scheckAarMetadataenforces both, is not a warning, and has no opt-out — reproduced by building the sample: AGP 8.13.2 fails at:app:checkDebugAarMetadatabefore compiling a line.So the builders refuse with a sentence naming the location button rather than emitting a project AGP rejects.
android.androidGradlePluginVersion(new hint, default unchanged) is the escape hatch for a project that has moved to AGP 9 itself.I walked the AGP 9 path end to end locally to find out what that move costs. With AGP 9.4.0 + Gradle 9.6.0 + compileSdk 37,
:app:checkDebugAarMetadatapasses — the library is accepted — after seven mechanical changes to the generated project: nojcenter(),mavenCentral()in the root buildscript, the Gradle 9.6 wrapper, no separate Kotlin plugin (AGP 9 has one built in),compileSdk =/minSdk =/targetSdk =assignments, nodexOptions, andproguard-android-optimize.txt. That migration is deliberately not in this PR: it changes the generated project for every module and every app, and it is worth landing on its own.A separate Android 17 blocker this turned up
:app:compileDebugJavaWithJavacthen fails — not on anything here. API 37 removesandroid.hardware.fingerprint.FingerprintManager, andAndroidBiometrics.javaandAndroidSecureStorage.javaare the only two users. All 68 errors trace to that one package.That blocks
compileSdk 37for every Codename One app, independent of this feature and of the AGP 9 move. It needs its own change.API 37 in CI
scripts/ci/check-android-location-button-package-compiles.shcompiles the deletable package against the real aar and a real API 37 platform. Stubs would be vacuous here — they get written from the same reading of the API as the code, so a wrong name appears in both and the gate passes on exactly the mistake it exists to catch.A new workflow (
location-button-android37.yml) installsplatforms;android-37.0and runs it with--require-api-37, so the platform cannot quietly become optional. Probed both ways: a wrong method name fails it, and an SDK without API 37 fails it rather than degrading to the olderandroid.jar.The gate is scoped to that one package rather than the whole port because of the
FingerprintManagerremoval above — a port-wide API 37 gate would be red on master until that is fixed.Verification
androidx.core.locationbuttonaar and API 37'sandroid.jar.Location.asciidocchapter (migration example and a persistent-location checklist).Needs the BuildDaemon PR
LocationButtonManifestFragmentsis mirrored in codenameone/BuildDaemon and differs only in its package line and four javadoc references. Hosted builds need both.