Follow-up from #808, #811 and #812. Not urgent, and not a bug report — a note about why those PRs took as many review rounds as they did.
Observation
HotspotManager currently carries 17 private var fields:
channel, wakeLock, wifiLock, currentGroup, callback, isStarting,
hasNotifiedStarted, isReceiverRegistered, savedSsid, savedPassword,
lastP2pState, createdActiveGroup, groupCreationPending,
recordedGeneratedName, teardownCallback, teardownInProgress, ownedGroupNameFive of those were added across #811 alone, each to answer a question an earlier fix had made askable:
| Field | Added because |
|---|---|
createdActiveGroup | removal must not touch another app's group |
groupCreationPending | a submitted createGroup can land after a stop |
recordedGeneratedName | below Q the marker is only learned from the first poll |
teardownCallback | the Aware hold must outlive an asynchronous teardown |
teardownInProgress | a detached channel means two different things |
Why it matters
Nearly every review finding on those PRs had the same shape: two of these fields disagreed, or one was read as meaning something it does not. A few examples:
ownedGroupName == nullmeant both "no group is ours" and "we have not learned the name yet" — states needing opposite handling on a recovery path.channel == nullmeant both "never initialised" and "already being torn down".hotspotManager == nullin the ViewModel was standing in for "we owe an Aware release".
Each was fixed correctly and in isolation, but the underlying shape is a state machine whose states are implied by combinations of independent booleans. The number of reachable combinations grows faster than the guards do, which is why the findings kept arriving.
Suggestion
Replace the lifecycle flags with an explicit sealed state — roughly Idle, Starting, CreatingGroup, Hosting, TearingDown — carrying the data each state actually needs (the channel, the group name, the pending completion). Illegal combinations then stop being representable rather than being guarded against one at a time.
HotspotStartupPolicy is a useful precedent from #808: pulling the retry and preflight decisions into a pure, Android-free object made them directly testable, and none of the subsequent findings landed in it.
Worth noting the credential and lock fields are incidental here — the cluster that causes trouble is the lifecycle one.
🤖 Generated with Claude Code
Activity
0No activity yet.