diff --git a/android/app/src/main/kotlin/es/freetimelab/pluriwave/AlarmScheduler.kt b/android/app/src/main/kotlin/es/freetimelab/pluriwave/AlarmScheduler.kt index 6fca362..de69524 100644 --- a/android/app/src/main/kotlin/es/freetimelab/pluriwave/AlarmScheduler.kt +++ b/android/app/src/main/kotlin/es/freetimelab/pluriwave/AlarmScheduler.kt @@ -336,19 +336,64 @@ class AlarmScheduler(private val context: Context) { } } + /** + * The occurrence a "close this one" action is really acting on, never a + * future one. + * + * Reported on-device: pressing Posponer left the alarm snoozed for ~1444 + * minutes (24h04m) instead of the configured few. The chain, all inside + * this file: [onAlarmFired] runs from the receiver BEFORE the ringing + * notification exists, and it persists `snoozeOriginMillis = null` plus a + * `triggerAtMillis` already advanced to TOMORROW by + * [computeNextTriggerMillis]. The snooze anchor was then plain + * `spec.snoozeOriginMillis ?: spec.triggerAtMillis`, so it picked up + * tomorrow. The old clamp (`if (target > now) target else now + minutes`) + * could not catch it: it only rescues anchors in the PAST, and an anchor + * +24h out sails straight through. + * + * [maxAheadMillis] is how far ahead an occurrence may legitimately sit for + * the calling surface: ~0 (just the shared imminence tolerance) for the + * ringing notification, but a full [PRE_NOTICE_MILLIS] for the pre-notice + * notification, whose occurrence has genuinely not happened yet. + * + * Mirrors `EstadoAlarmas._ocurrenciaSonando` on the Dart side, which was + * added in a9da855 for the exact same defect after 9c7cf4e had fixed only + * one of two adjacent callers. The native lane never got that guard. + * `lastHandledAtMillis` is the last fallback because [onAlarmFired] sets + * it to the occurrence that just rang -- note it is NOT purely native + * state (scheduleAlarm takes it from the Dart channel), so `now` has to + * remain the floor. + */ + private fun anchorOccurrenceMillis( + spec: NativeAlarmSpec, + now: Long, + maxAheadMillis: Long = 0L + ): Long { + val limit = now + maxAheadMillis + IMMINENT_TOLERANCE_MILLIS + fun usable(candidate: Long?): Long? = candidate?.takeIf { it <= limit } + return usable(spec.snoozeOriginMillis) + ?: usable(spec.triggerAtMillis) + ?: usable(spec.lastHandledAtMillis) + ?: now + } + /** * Snoozes using the SAME anchor as [postponeNext] (Design 2.2): the * occurrence time + minutes, clamped to now + minutes when the target is * already past. Returns the resulting snooze so the caller can report it * back to Flutter (single source of truth), or null if the spec is gone. + * + * The occurrence comes from [anchorOccurrenceMillis] with no forward + * allowance: this is the RINGING notification's button, so the occurrence + * it closes has already arrived. */ fun snooze(id: String, minutes: Int): NativeSnoozeResult? { cancelAutoSilence(id) val spec = readSpec(id) ?: return null val safeMinutes = sanitizeSnoozeMinutes(minutes) - val occurrenceAt = spec.snoozeOriginMillis ?: spec.triggerAtMillis - val target = occurrenceAt + safeMinutes * 60_000L val now = System.currentTimeMillis() + val occurrenceAt = anchorOccurrenceMillis(spec, now) + val target = occurrenceAt + safeMinutes * 60_000L val snoozeUntil = if (target > now) target else now + safeMinutes * 60_000L Log.d( tag, @@ -369,12 +414,24 @@ class AlarmScheduler(private val context: Context) { ) } + /** + * Postpones from the PRE-NOTICE notification, whose occurrence has + * legitimately not arrived yet -- it is armed [PRE_NOTICE_MILLIS] ahead. + * So unlike [snooze] this allows an anchor that far forward, but no + * further: an anchor beyond that window is a spec already advanced to a + * later day, which is exactly the state that produced the reported ~24h + * snooze. See [anchorOccurrenceMillis]. + */ fun postponeNext(id: String, minutes: Int): Long? { val spec = readSpec(id) ?: return null val safeMinutes = sanitizeSnoozeMinutes(minutes) - val occurrenceAt = spec.snoozeOriginMillis ?: spec.triggerAtMillis - val target = occurrenceAt + safeMinutes * 60_000L val now = System.currentTimeMillis() + val occurrenceAt = anchorOccurrenceMillis( + spec, + now, + maxAheadMillis = PRE_NOTICE_MILLIS + ) + val target = occurrenceAt + safeMinutes * 60_000L val snoozeUntil = if (target > now) target else now + safeMinutes * 60_000L Log.d( tag, diff --git a/lib/servicios/servicio_audio.dart b/lib/servicios/servicio_audio.dart index 45535ea..82508a8 100644 --- a/lib/servicios/servicio_audio.dart +++ b/lib/servicios/servicio_audio.dart @@ -328,6 +328,57 @@ List controlesEcualizadorPersonalizados({ ]; } +/// The handler's full transport `controls` list for a `playbackState` push. +/// +/// Top-level and public so tests exercise THIS function rather than a copy of +/// its shape. `servicio_audio_controles_notificacion_test.dart` used to +/// re-declare the list inline, which meant it stayed green while asserting a +/// shape `lib/` no longer produced — a guard that cannot see the thing it +/// guards. `PluriWaveAudioHandler` itself cannot be instantiated in a unit +/// test (a real `just_audio.AudioPlayer` needs platform MethodChannels), so +/// pulling the pure part out is the only way to test the real thing. +/// +/// ORDER MATTERS, and only for the car. +/// +/// On Android 13+ `createCustomAction` (AudioService.java:466-469) turns +/// [MediaControl.stop] into a `CUSTOM_ACTION_STOP` custom action too. So on a +/// modern phone the car receives TWO custom actions, in list order, and a head +/// unit that exposes a single custom-action slot shows only the first and +/// buries the rest in an overflow menu — which is why the equalizer toggle +/// stayed invisible on the playback screen even once it was back in this list +/// (reported on v1.2.14+136, which does contain it). +/// +/// The equalizer therefore goes BEFORE `stop`, and wins that slot on purpose: +/// the car already has its own path to stop playback and Auto's template +/// renders play/pause itself, while the equalizer is reachable no other way +/// from this screen. +/// +/// The phone notification is untouched by that ordering, on every API level. +/// `setState` (AudioService.java:513-521) splits this list by whether a +/// control carries a `customAction`: on 13+ `stop` goes to `customActions` +/// (never the notification) and the equalizer was never in `nativeActions` +/// anyway; below 13 the equalizer is the only custom action and `stop` stays +/// native. Either way `nativeActions` comes out as +/// `[prev?, play/pause, stop, next?]`, and `androidCompactActionIndices` +/// (`[colaActiva ? 1 : 0]`) still lands on play/pause. +List construirControlesTransporte({ + required bool colaActiva, + required bool playing, + required bool eqDisponible, + required bool eqActivo, + required AppLocalizations l10n, +}) => [ + if (colaActiva) MediaControl.skipToPrevious, + if (playing) MediaControl.pause else MediaControl.play, + ...controlesEcualizadorPersonalizados( + disponible: eqDisponible, + activo: eqActivo, + l10n: l10n, + ), + MediaControl.stop, + if (colaActiva) MediaControl.skipToNext, +]; + /// Content-style extras for the Ecualizador folder's items (decision /// `auto/ecualizador-diseno`), mirrors `ConstructorArbolAuto /// ._contentStyleLista` in `navegacion_auto.dart` — duplicated rather than @@ -673,6 +724,7 @@ class PluriWaveAudioHandler extends BaseAudioHandler speed: _player.speed, ), ); + _trazarEstadoPublicado(); }); _bufferedSub = _player.bufferedPositionStream.listen((pos) { @@ -704,51 +756,52 @@ class PluriWaveAudioHandler extends BaseAudioHandler }); } - /// The full transport `controls` list for a `playbackState` push (item 4): - /// the existing skip/play-pause/stop set, plus the equalizer's custom - /// actions appended at the end. Appending (rather than interleaving) keeps - /// [MediaControl.skipToPrevious]/play-pause/stop/[MediaControl.skipToNext] - /// at their existing indices 0-3, so `androidCompactActionIndices` - /// (`[colaActiva ? 1 : 0]`) stays correct unchanged. + String? _ultimaTrazaEstado; + + /// Logs the state actually handed to `AudioService.setState`, once per real + /// change (this fires on every player event, so unconditional logging would + /// bury the signal). /// - /// A custom action here reaches the CAR ONLY, never the phone notification. - /// `AudioService.setState` (AudioService.java:513-520) splits the list in - /// two: `createCustomAction` returns non-null for a control carrying a - /// `customAction`, and that control goes into `customActions` — which feeds - /// `PlaybackStateCompat` and therefore the car's playback screen. Every - /// other control falls to the `else` branch and becomes a - /// `NotificationCompat.Action` in `nativeActions`, the list the media - /// notification is built from. The two never mix, so the equalizer toggle - /// cannot displace a transport button and cannot shift the indices - /// `androidCompactActionIndices` points at. + /// Exists for one open question that static reading could not settle: the + /// Android Auto playback screen shows PLAY while a station is audibly + /// playing. The car does NOT take that icon from `controls` — it takes it + /// from `PlaybackStateCompat.getState()` (AudioService.java:601-611), where + /// `ready` + `playing` is the only combination that yields `STATE_PLAYING`; + /// `idle` gives `STATE_NONE`, which is what a freshly created session + /// carries (:319) and what a car would render as a play button. Every + /// `playbackState.add` in this file was audited and none publishes + /// `playing: false` while audio runs, so the failing input is unknown and + /// any fix would be guesswork. /// - /// THE ONE RULE for anything added here with a `customAction`: its - /// `androidIcon` must name a drawable that really exists, and its `label` - /// must be non-empty in EVERY locale. `getResourceId` (:415-420) resolves - /// the icon by name through `getIdentifier` and yields 0 when it misses, - /// and `PlaybackStateCompat.CustomAction.Builder` throws on a 0 icon or an - /// empty label — a throw at :515 aborts `setState` before - /// `mediaSession.setPlaybackState` (:552), taking the whole media session - /// down with it. `servicio_audio_controles_notificacion_test.dart` holds - /// that line: it reads `android/app/src/main/res/drawable/` and fails if an - /// icon named here has no file behind it. + /// `eqDisponible` rides along because the equalizer custom action is gated + /// on it and the flag is otherwise unobservable — one car session answers + /// both questions at once: + /// adb logcat -s ServicioAudio + void _trazarEstadoPublicado() { + final s = playbackState.value; + final traza = + 'proc=${s.processingState.name} playing=${s.playing} ' + 'eqDisponible=$_eqDisponible eqActivo=$_ecualizadorActivo ' + 'custom=${s.controls.where((c) => c.customAction != null).length} ' + 'controles=${s.controls.length}'; + if (traza == _ultimaTrazaEstado) return; + _ultimaTrazaEstado = traza; + developer.log(traza, name: 'ServicioAudio'); + } + + /// Binds [construirControlesTransporte] — which holds the whole contract, + /// including why the equalizer must precede `stop` — to this handler's live + /// equalizer state. List _controlesTransporte({ required bool colaActiva, required bool playing, - }) => [ - if (colaActiva) MediaControl.skipToPrevious, - if (playing) MediaControl.pause else MediaControl.play, - MediaControl.stop, - if (colaActiva) MediaControl.skipToNext, - ..._controlesEqPersonalizados(), - ]; - - List _controlesEqPersonalizados() => - controlesEcualizadorPersonalizados( - disponible: _eqDisponible, - activo: _ecualizadorActivo, - l10n: _textos, - ); + }) => construirControlesTransporte( + colaActiva: colaActiva, + playing: playing, + eqDisponible: _eqDisponible, + eqActivo: _ecualizadorActivo, + l10n: _textos, + ); /// Re-pushes `playbackState` with a freshly built controls list (item 4): /// called whenever EQ availability/enabled state changes outside a diff --git a/test/servicios/servicio_audio_controles_notificacion_test.dart b/test/servicios/servicio_audio_controles_notificacion_test.dart index e6141e0..dd4c9e1 100644 --- a/test/servicios/servicio_audio_controles_notificacion_test.dart +++ b/test/servicios/servicio_audio_controles_notificacion_test.dart @@ -137,55 +137,114 @@ void main() { }); group('transport row keeps its shape', () { - /// Mirrors `_controlesTransporte`'s construction. Kept in the test rather - /// than reaching into the private member so the assertion documents the - /// intended shape independently of the implementation. + // Calls the REAL builder, never a copy of it. This group used to + // re-declare the list inline, which meant it stayed green while asserting + // a shape lib/ no longer produced — a guard blind to the thing it guards. List transporte({ required bool colaActiva, required bool playing, required bool eqDisponible, - }) => [ - if (colaActiva) MediaControl.skipToPrevious, - if (playing) MediaControl.pause else MediaControl.play, - MediaControl.stop, - if (colaActiva) MediaControl.skipToNext, - ...controlesEcualizadorPersonalizados( - disponible: eqDisponible, - activo: true, - l10n: lookupAppLocalizations(const Locale('es')), - ), - ]; + }) => construirControlesTransporte( + colaActiva: colaActiva, + playing: playing, + eqDisponible: eqDisponible, + eqActivo: true, + l10n: lookupAppLocalizations(const Locale('es')), + ); + + /// What `AudioService.setState` (AudioService.java:513-521) would route to + /// `nativeActions` — the ONLY list the phone's media notification is built + /// from, and the list `androidCompactActionIndices` indexes into. + /// + /// On Android 13+ `MediaControl.stop` also becomes a custom action + /// (:466-469), so pass [sdk33] to model that split. + List nativas( + List controles, { + required bool sdk33, + }) => + controles + .where((c) => c.customAction == null) + .where((c) => !(sdk33 && c == MediaControl.stop)) + .toList(); for (final colaActiva in [false, true]) { for (final playing in [false, true]) { - test('compact index still points at play/pause ' + test('compact index still points at play/pause on BOTH API levels ' '(colaActiva=$colaActiva playing=$playing)', () { final controles = transporte( colaActiva: colaActiva, playing: playing, eqDisponible: true, ); - - // androidCompactActionIndices is `[colaActiva ? 1 : 0]`. The EQ - // action is APPENDED, so the native transport buttons keep - // indices 0-3 and the collapsed shade still shows play/pause. final indiceCompacto = colaActiva ? 1 : 0; - expect(controles.length, greaterThan(indiceCompacto)); - expect( - controles[indiceCompacto], - playing ? MediaControl.pause : MediaControl.play, - ); - // The custom action must never sit among the transport buttons: - // `nativeActions` and `customActions` are built by walking this - // list in order, so an interleaved custom action would renumber - // the notification's own actions. - final indiceCustom = controles.indexWhere( - (c) => c.customAction != null, - ); - expect(indiceCustom, controles.length - 1); + for (final sdk33 in [false, true]) { + final row = nativas(controles, sdk33: sdk33); + expect(row.length, greaterThan(indiceCompacto)); + expect( + row[indiceCompacto], + playing ? MediaControl.pause : MediaControl.play, + reason: + 'androidCompactActionIndices is [colaActiva ? 1 : 0] and it ' + 'indexes nativeActions (AudioService.java:613-618, :637-639)', + ); + } }); } } + + test('the equalizer comes BEFORE stop, so it wins the first custom-action ' + 'slot on the car', () { + // Reported on v1.2.14+136: the toggle was in the binary but invisible on + // the head unit. On Android 13+ stop is ALSO a custom action, and it used + // to be first — a unit exposing one slot showed stop and buried the + // equalizer in an overflow menu. + final controles = transporte( + colaActiva: true, + playing: true, + eqDisponible: true, + ); + + final custom = controles.where( + (c) => c.customAction != null || c == MediaControl.stop, + ); + expect( + custom.first.customAction, + isNotNull, + reason: 'on Android 13+ this is the order the car receives them in', + ); + }); + + test('reordering did NOT disturb the notification row', () { + // The whole safety argument for the swap: nativeActions must come out + // [prev?, play/pause, stop, next?] on <13 and [prev?, play/pause, next?] + // on 13+, exactly as before the equalizer moved. + final controles = transporte( + colaActiva: true, + playing: true, + eqDisponible: true, + ); + + expect(nativas(controles, sdk33: false), [ + MediaControl.skipToPrevious, + MediaControl.pause, + MediaControl.stop, + MediaControl.skipToNext, + ]); + expect(nativas(controles, sdk33: true), [ + MediaControl.skipToPrevious, + MediaControl.pause, + MediaControl.skipToNext, + ]); + }); + + test('a device with no equalizer gets the exact pre-existing list', () { + expect(transporte(colaActiva: true, playing: true, eqDisponible: false), [ + MediaControl.skipToPrevious, + MediaControl.pause, + MediaControl.stop, + MediaControl.skipToNext, + ]); + }); }); }