Fix: Reconnect gesture did nothing unless Amply set the limit - #28
Merged
Conversation
The any-level arming basis gated on ChargingPreferences.lastPersistentPolicy alone, which stays null until Amply itself performs a persistent write. On a device whose protective limit was set natively (or by a previous install, or before the policy.v2 DataStore migration), the journal is absent, so the basis never armed and the unplug/replug gesture silently did nothing even though the limit genuinely was set. The evidence has to be tri-state, not a Boolean: the charging-policy hardware state is only reported while external power is present, so an unplug tick carries no evidence at all. Folded into a Boolean, that unplug tick would read as "not protective" and run the engine's revocation block before the powered->unpowered edge is recorded, so the reconnect window would never open. QuickFullChargeGesture.Input now carries a PolicyEvidence instead of a Boolean. Arming requires PROTECTIVE; revocation of an ANY_LEVEL basis requires an explicit opt-out or conclusive UNRESTRICTED evidence, so UNKNOWN preserves an open window (bounded by the 10s reconnect ceiling, and ChargeSessionManager.begin() still re-verifies live state and refuses when readback proves charging already reaches full). New pure GestureBasis resolves both the evidence and the limit percent from a verified observation first and the persistent journal second. Protection is !allowsFullCharge, never != Unrestricted, so PauseAtFull and FixedLimit(100) are not mistaken for a cap. protectivePolicyNow() is deliberately not a source: it defaults to FixedLimit(80) with no history and retains a stale baseline after a persistent Unrestricted write. ChargeSessionService resolves the adapter selection once inside the gesture branch and reuses it for the availability check and the hardware decode - it must stay behind the session early-return and the cheap enabled check, because DeviceInfo.current() resolves activities/providers, reads Samsung settings and queries UserManager. Every gesture tick now also logs its full input at VERBOSE so this class of failure is diagnosable from a debug log without a DataStore teardown.
The reconnect gesture's options were only reachable through a gear on the dashboard card, which is not where anyone looks for a preference. The screen is now Settings > Charging: the settings hub gains a "Charging" row (under General) whose subtitle reports whether the gesture is on, and the screen itself gains the gesture master toggle above the existing any-level toggle. The screen carries the dashboard card's availability gate (reconnectSupported && canApply) as canEnableGesture. Without it, settings would be a way to switch the gesture on for a device where the card correctly forbids it, leaving an "On" preference that either stops immediately or strands a useless foreground monitor. The master row stays interactive while the gesture is already enabled, so an unsupported configuration can always be turned back off, and it shows an unavailable subtitle otherwise. Enabling routes through the POST_NOTIFICATIONS request exactly as the dashboard card does. CHARGING is deliberately not in the BackHandler's "entered from the dashboard" group: it lives under the hub, so system Back returns there — the same place its top-bar Back goes. The dashboard card loses its gear and its callback; its trailing row now carries only the toggle indicator. The any-level copy no longer claims protection must be "set through Amply", which the previous commit made false.
The waiting notification said only "Waiting for a quick unplug and reconnect",
which gives no clue about the condition the gesture is waiting on. It now names
the limit ("at your 80% limit") whenever an authoritative source knows one, and
otherwise states the condition ("while your charge limit is holding").
The percent comes from GestureBasis.limitPercent — a verified observation
first, Amply's persistent journal second — so a natively-set adaptive policy is
never labelled with a number the journal happens to remember. On a fresh
install with no journal the unplugged waiting copy stays generic and gains the
number once plugged in and the hardware reports the policy.
The armed copy is unchanged: it is the time-critical "reconnect now within 10s"
instruction and must not be diluted.
An ANY_LEVEL latch previously survived UNKNOWN evidence indefinitely: the revocation condition only fired on an explicit opt-out or on conclusive UNRESTRICTED evidence, and the plugged branch only ever set armedBy. On a journal-less device a natively-removed limit decodes as UNKNOWN, so the basis stayed latched forever and a later 2-10s unplug/replug started a session; on a WSS-only device SessionStartDecider cannot refuse it, so Amply would "restore" a limit the user had deliberately removed. Inconclusive evidence now also revokes, but only on a tick where the conclusive signal was actually available: plugged, with no reconnect window open. The disconnectedAtMillis == null guard is load-bearing - the revocation block runs before the replug edge is handled, so without it a replug tick whose hardware has not re-reported charging state 4 yet would destroy the very trigger the gesture exists to deliver. Dropping costs nothing: the basis re-arms on the next protective tick. Fixes review finding F1.
limitPercent fell through to lastPersistentPolicy, Amply's journal of its own persistent writes. A user who applied 80% through Amply and then set the native Pixel charging setting to unrestricted hits Pixel's powered NORMAL state, which decodes as ChargeObservation.Unknown - so the journal answered and the notification claimed "at your 80% limit" indefinitely while no limit was configured. limitPercent now takes only the observation and answers only for a Verified fixed limit; the lastPersistent parameter is gone rather than left unused. Nothing loses a percentage it legitimately had: only the Pixel adapter implements decodeHardware and only Pixel supports the reconnect gesture, so an unverified state falls back to the generic "while your charge limit is holding" copy. evidence() keeps its journal fallback: the arming residual there is covered by SessionStartDecider and is a separate, accepted trade-off. Fixes review finding F2.
The gesture guard returns unless reconnectGestureSupported is true, which smart-casts the adapter to non-null, so the safe call on the hardware decode was flagged as unnecessary by the Kotlin compiler.
evaluateBattery collapsed ARMED and WAITING_FOR_RECONNECT into one
boolean and SessionNotifications.gesture rendered both with the single
gesture_notification_armed string, so a plugged-in device — which now
sits in ARMED indefinitely — always read "Reconnect now" and never
showed the any-level vs. limit distinction, which had landed on the
IDLE copy nobody sees.
gesture() now takes the decision:
- WAITING_FOR_RECONNECT keeps the short countdown, retexted to drop the
"unplug 2s" prefix (by then the user has already unplugged).
- ARMED gets new copy carrying the arming basis:
gesture_notification_armed_{any_level,limit,holding}.
- IDLE keeps the existing waiting copies.
Any-level-before-limit-percent precedence is unchanged in both groups.
The "you can turn this off" hint now attaches to every state except the
countdown — ARMED is a steady state that can persist for hours.
Adds a Robolectric test over the rendered content/big text.
Fixes review finding F3.
The armed gesture notification passed the latched arming basis (anyLevelBasis) into the copy. At a holding limit the LIMIT_HOLD branch wins the latch, so a device sitting at 80% with "Any charge level" on rendered the identical "Ready at your 80% limit" text as with the option off - understating a gesture that will in fact re-arm and fire at any charge level. The armed and waiting-for-reconnect states now pass the condition the gesture fires under: the any-level option being on AND its policy evidence currently PROTECTIVE. Requiring PROTECTIVE evidence avoids the opposite error of claiming "any charge level" when the option is on but nothing protective is detected. Idle copy keeps describing the enabled mode. The engine is untouched, and anyLevelBasis remains in the DEBUG log line, where the actually-latched basis is the right thing to record. Fixes review finding F4.
The gesture settings moved from a dedicated screen reached via the dashboard card's gear into the new Charging screen under the settings hub, so the committed store shot no longer matched the app. Re-rendered via generate_screenshots.sh + copy_screenshots.sh; only this shot changed, the dashboard shot was unaffected.
d4rken
added
bug
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
1 participant
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.
What changed
The reconnect gesture now actually fires. Previously it only armed if the protective charge limit had been applied through Amply itself. If the limit was already set on the phone — configured in system settings, left over from an earlier install, or predating an app update that reset stored preferences — the gesture silently did nothing: the ongoing notification sat on its idle text forever and a quick unplug/replug never started a full charge. Amply now also trusts the phone's live charging state, so a limit it didn't set itself still arms the gesture.
The gesture's options moved into a new "Charging" screen in Settings. The dashboard card no longer carries a settings shortcut — it keeps just its on/off switch. The new screen holds both the master switch and the "Any charge level" option, and it respects the same availability rules as the dashboard card, so the gesture can't be switched on where the card correctly forbids it.
The ongoing notification now tells you the condition it will fire under. It reads "Ready at your 80% limit…" or "Ready at any charge level…" depending on the "Any charge level" setting, and switches to a short "Reconnect now: replug within 10 seconds…" while the replug window is open. Previously all of these states shared one line of text.
Technical Context
Root cause. The any-level arming basis gated on the stored record of persistent policy writes, which is empty until Amply performs its first write. On an affected device that record is absent while the limit genuinely is active, so the basis never armed. Confirmed on-device: the stored record held no policy entry, while settings readback reported a verified 80% limit and the battery broadcast reported the corresponding charging-policy hardware state.
Why the evidence is tri-state. The obvious fix — OR the hardware signal into the existing boolean — does not work. The charging-policy extra is only reported while external power is present, so the unplug tick reports nothing, and the state machine evaluates its revocation rule before it records the powered→unpowered edge. A boolean would therefore revoke the basis on the very tick that should open the reconnect window. Evidence is now
PROTECTIVE/UNRESTRICTED/UNKNOWN: only an explicit opt-out or conclusive unrestricted evidence revokes, whileUNKNOWNis tolerated on an unplugged tick or inside an open window. A plugged tick that goes inconclusive does revoke — powered, the evidence is available, so an inconclusive reading means the configuration is no longer known to be protective; it simply re-arms on the next protective tick. ThedisconnectedAtMillis == nullguard in that condition is load-bearing: without it, a replug tick whose hardware has not yet re-reported its hold would destroy its own trigger.Protection is
!allowsFullCharge, not!= Unrestricted—PauseAtFulland a 100% fixed limit both reach full charge and must not arm a gesture whose purpose is to lift a cap.Notification percentage sourcing. The named percentage comes only from a verified observation, never from the write journal: the journal records what Amply last wrote, not what is configured now, so a limit removed in system settings would otherwise keep being claimed. The stored protective baseline is deliberately not a source either — it defaults to 80% with no history and retains a stale value after an unrestricted write. Consequence: an unverified state falls back to generic wording rather than naming a number.
Armed copy states the firing condition, not the latched basis. At the limit, the limit-hold basis wins the latch even when the any-level option is on and qualifying, so reporting the latched basis understated a gesture that will in fact fire at any level. The copy now derives from the option plus current protective evidence.
Adapter resolution stays inside the gesture branch, behind the session early-return and the cheap enabled check — resolving device info walks package/provider lookups, and hoisting it would newly charge active sessions, disabled gestures and watcher-only ticks for it under the command mutex. Net cost is unchanged.
Known residual (deliberate). With "Any charge level" on, inconclusive hardware evidence plus a stale protective journal still arms even if charging was since set unrestricted outside Amply. Session start refuses whenever the current policy is verifiable, so this only applies without Shizuku, and it is pre-existing behaviour. Failing closed would reverse the documented rule that a stale last-request must never block a session, and would disable the gesture entirely for users without Shizuku.
Review guidance. The state-machine change is the risk surface; the pure engine and its evidence resolver are covered by JVM tests, including regressions for the unplug-tick and open-window cases. The rest is UI wiring and copy. The system Back grouping was fixed, not just renamed, so both back paths from the new screen land on Settings.
Verification