Repository navigation
Follow the core driver API the collectors are built on - #5
Merged
Merged
Conversation
The map is an AndroidView laid under a full-screen Compose overlay, and an event nobody consumed goes to it. So tapping the padding of a bubble, or its title, or anywhere its content did not itself handle, was read as a tap on the map: on the street tree page that selected whichever tree happened to sit under the bubble and swapped the contents for another one. "Hit the bubble and nothing wanted it" is not the same as "missed the bubble". The overlay Box now consumes what its content leaves behind, so the first case stops there and the second still falls through to the map. Consumption happens on the Main pass. Compose delivers Main from the child outwards, so by the time it arrives here the bubble's own buttons and clickable rows have already taken theirs; consuming on Initial would have killed them. Verified on the tablet with the Multiple Bubbles sample, whose bubbles close themselves when tapped: the tapped one still closes and its neighbour stays open. The Box is the bubble's bounding rectangle, so the corners beside the tail count as the bubble. That is a few pixels either way and the alternative is hit-testing the drawn shape, which is not worth it to make a corner pixel pan the map. Panning still works, and a tap on the map away from the bubble still dismisses it. All three InfoBubble entry points share this overlay, so all three are fixed.
The consuming modifier was on InfoBubbleOverlay, whose Box is the bounding rectangle. That made the corners either side of the tail count as bubble: an area half the bubble's width by the tail's height where the map is plainly visible and a tap did nothing. The shape is known to DrawInfoBubble, not to the overlay, so the consuming moves there. isInsideBubble is the same geometry the Canvas draws — the body down to height minus the tail, plus the triangle closing linearly to its tip. Corner rounding is ignored; at the default it is 4dp against a tail-side area of half the width by 8dp, which is what this is about. InfoBubbleCustom draws its own shape and so keeps its own hit behaviour. It has no tail this code knows about and no rectangle worth guessing at. DrawInfoBubbleHitTest pins the boundary. Consuming too much and consuming too little both look like "it sometimes ignores me" and neither logs anything, so the edges are worth stating: body inside, beside the tail outside, the tail itself inside and narrowing.
The tail is 8dp at the default. Hit-testing the triangle costs a line of geometry and a test to hold it up, and buys the case where someone aims at the point itself — where the consequence of missing is that the map below responds, not that something they were aiming at fails to move. The rule is now the body rectangle, which is what the consuming was for.
Dragging the street tree map was heavy on a Pixel 5a long after the tile rendering stopped being the reason. Frame stats put 270 to 380 ms of every janky frame between AnimationStart and PerformTraversalsStart, with the GPU at a 16 ms 90th percentile; simpleperf put three quarters of the main thread in androidx.collection.ScatterSet.contains, under advanceGlobalSnapshot, under a snapshotFlow's apply observer. That observer belonged to the collector. To find in-place changes it read every state's fingerprint inside one snapshotFlow, and Compose asks such an observer, on every snapshot commit anywhere in the app, whether any object it read is among the changed ones — one contains per object read. 144,183 markers of a dozen fields each is about 1.7 million probes, and a drag commits a snapshot every frame. The loop ran the wrong way round. Asking "of everything I read, did any of it change?" costs the size of the collection whether or not anything changed; being told "this one changed" costs one call per actual change, and during a drag markers do not change at all. So the states push: every mutable field goes through StateMutationSignal.notifying, which writes through to the same mutableStateOf — the field stays readable from a composition exactly as before — and reports the write. On a Pixel 5a, same 144k markers, same drag: janky frames 17.7% -> 1.4% 90th 350ms -> 21ms 99th 700ms -> 24ms slow UI thread 19 -> 2 which is where an empty map already was (0.2%, 16ms, 19ms). Quartering the markers had quartered the cost, so this was never about what was drawn. The collector keeps its own id-to-state map of who is reporting. Membership reaches `flow` through a debounce, so a state added and removed inside one window is not in that map when the removal arrives; clearing by id there found nothing and left the state reporting for the rest of the collector's life, which the removal test caught. Two things go with it. The FingerPrint type parameter, which existed only for the diff — fingerPrint() stays, several providers do their own diffing with it. And asFlow() on all six state types plus CollectAndUpdateEach, the per-state watcher this replaced twice over: nothing called either, and they are the shape that must not come back. The failure mode of the swap is silent — a setter that forgets to report leaves the renderer drawing the old value, with nothing logged and nothing thrown — so OverlayCollectorMutationTest states the contract: a write arrives, an equal write does not, repeats inside a window collapse to the latest, a removed state goes quiet, and two states changing together both arrive.
OverlayCollector is @InternalMapConductorApi in android-sdk-core now: it is the overlay collection wiring, not something an app calls, and its type parameters changed with the collector redesign. This module is that wiring, so it opts in per module the way android-for-* does. The baseline follows: the collectors lose their FingerPrint type argument, and CollectAndUpdateEach is gone with the redesign — states report their own writes instead of being read for them.
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.
mainstill builds against the oldOverlayCollectorshape:android-sdk-core'sdevelophas since collapsed that to a single type argument, with the states reporting their own changes:So
mainno longer compiles against the core it is published next to. Everyandroid-for-*provider CI clones the default branch of bothandroid-sdk-coreand this repo and runspublishToMavenLocal, so all ten provider PRs fail in_compose:compileReleaseKotlin, regardless of their own diffs:This branch already carries that catch-up work; it is only unmerged. Opening it so the default branch stops breaking its consumers.
Note: this is pre-existing work on this branch, not written here - it is being proposed for merge so provider CI can go green.