From fbe631fe91a180cc52b7b3979a44e31677bbadf2 Mon Sep 17 00:00:00 2001 From: Thomas <9749173+uhthomas@users.noreply.github.com> Date: Mon, 6 Apr 2026 16:13:45 +0100 Subject: [PATCH] fix(mobile): convert video controls from hook to stateful widget (#27514) We are generally looking to move away from hooks as they are hard to reason about and have weird bugs. In particular, the timer did not properly capture the ref of the callback, and so it would execute on old state. A standard stateful widget does not have this problem, and is easier to organise. Co-authored-by: Alex --- .../asset_viewer/bottom_bar.widget.dart | 2 +- .../widgets/asset_viewer/video_controls.dart | 90 +++++++++++++------ 2 files changed, 63 insertions(+), 29 deletions(-) diff --git a/mobile/lib/presentation/widgets/asset_viewer/bottom_bar.widget.dart b/mobile/lib/presentation/widgets/asset_viewer/bottom_bar.widget.dart index 1f327bf79f..b51960bb05 100644 --- a/mobile/lib/presentation/widgets/asset_viewer/bottom_bar.widget.dart +++ b/mobile/lib/presentation/widgets/asset_viewer/bottom_bar.widget.dart @@ -74,7 +74,7 @@ class ViewerBottomBar extends ConsumerWidget { child: Column( mainAxisSize: MainAxisSize.min, children: [ - if (asset.isVideo) VideoControls(key: ValueKey(asset.heroTag), videoPlayerName: asset.heroTag), + if (asset.isVideo) VideoControls(videoPlayerName: asset.heroTag), if (!isReadonlyModeEnabled) Row(mainAxisAlignment: MainAxisAlignment.spaceEvenly, children: actions), ], diff --git a/mobile/lib/widgets/asset_viewer/video_controls.dart b/mobile/lib/widgets/asset_viewer/video_controls.dart index 946e64fcbb..89b0f0ec30 100644 --- a/mobile/lib/widgets/asset_viewer/video_controls.dart +++ b/mobile/lib/widgets/asset_viewer/video_controls.dart @@ -1,5 +1,6 @@ import 'dart:math'; +import 'package:async/async.dart'; import 'package:flutter/material.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/constants/colors.dart'; @@ -7,26 +8,63 @@ import 'package:immich_mobile/models/cast/cast_manager_state.dart'; import 'package:immich_mobile/providers/asset_viewer/asset_viewer.provider.dart'; import 'package:immich_mobile/providers/asset_viewer/video_player_provider.dart'; import 'package:immich_mobile/providers/cast.provider.dart'; -import 'package:immich_mobile/utils/hooks/timer_hook.dart'; import 'package:immich_mobile/extensions/duration_extensions.dart'; import 'package:immich_mobile/widgets/asset_viewer/animated_play_pause.dart'; -class VideoControls extends HookConsumerWidget { +class VideoControls extends ConsumerStatefulWidget { final String videoPlayerName; static const List _controlShadows = [Shadow(color: Colors.black87, blurRadius: 6, offset: Offset(0, 1))]; const VideoControls({super.key, required this.videoPlayerName}); - void _toggle(WidgetRef ref, bool isCasting) { - if (isCasting) { - ref.read(castProvider.notifier).toggle(); - } else { - ref.read(videoPlayerProvider(videoPlayerName).notifier).toggle(); + @override + ConsumerState createState() => _VideoControlsState(); +} + +class _VideoControlsState extends ConsumerState { + late final RestartableTimer _hideTimer; + + AutoDisposeStateNotifierProvider get _provider => + videoPlayerProvider(widget.videoPlayerName); + + @override + void initState() { + super.initState(); + _hideTimer = RestartableTimer(const Duration(seconds: 5), _onHideTimer); + } + + @override + void didUpdateWidget(covariant VideoControls oldWidget) { + super.didUpdateWidget(oldWidget); + if (oldWidget.videoPlayerName != widget.videoPlayerName) { + _hideTimer.reset(); } } - void _onSeek(WidgetRef ref, bool isCasting, double value) { + @override + void dispose() { + _hideTimer.cancel(); + super.dispose(); + } + + void _onHideTimer() { + if (!mounted) return; + if (ref.read(_provider).status == VideoPlaybackStatus.playing) { + ref.read(assetViewerProvider.notifier).setControls(false); + } + } + + void _toggle(bool isCasting) { + if (isCasting) { + ref.read(castProvider.notifier).toggle(); + return; + } + + ref.read(_provider.notifier).toggle(); + } + + void _onSeek(bool isCasting, double value) { final seekTo = Duration(microseconds: value.toInt()); if (isCasting) { @@ -34,38 +72,30 @@ class VideoControls extends HookConsumerWidget { return; } - ref.read(videoPlayerProvider(videoPlayerName).notifier).seekTo(seekTo); + ref.read(_provider.notifier).seekTo(seekTo); } @override - Widget build(BuildContext context, WidgetRef ref) { - final provider = videoPlayerProvider(videoPlayerName); + Widget build(BuildContext context) { final cast = ref.watch(castProvider); final isCasting = cast.isCasting; final (position, duration) = isCasting ? ref.watch(castProvider.select((c) => (c.currentTime, c.duration))) - : ref.watch(provider.select((v) => (v.position, v.duration))); + : ref.watch(_provider.select((v) => (v.position, v.duration))); - final videoStatus = ref.watch(provider.select((v) => v.status)); + final videoStatus = ref.watch(_provider.select((v) => v.status)); final isPlaying = isCasting ? cast.castState == CastState.playing : videoStatus == VideoPlaybackStatus.playing || videoStatus == VideoPlaybackStatus.buffering; final isFinished = !isCasting && videoStatus == VideoPlaybackStatus.completed; - final hideTimer = useTimer(const Duration(seconds: 5), () { - if (!context.mounted) return; - if (ref.read(provider).status == VideoPlaybackStatus.playing) { - ref.read(assetViewerProvider.notifier).setControls(false); - } - }); - ref.listen(assetViewerProvider.select((v) => v.showingControls), (prev, showing) { - if (showing && prev != showing) hideTimer.reset(); + if (showing && prev != showing) _hideTimer.reset(); }); - ref.listen(provider.select((v) => v.status), (_, __) => hideTimer.reset()); + ref.listen(_provider.select((v) => v.status), (_, __) => _hideTimer.reset()); - final notifier = ref.read(provider.notifier); + final notifier = ref.read(_provider.notifier); final isLoaded = duration != Duration.zero; return Padding( @@ -80,9 +110,13 @@ class VideoControls extends HookConsumerWidget { padding: const EdgeInsets.all(12), constraints: const BoxConstraints(), icon: isFinished - ? const Icon(Icons.replay, color: Colors.white, shadows: _controlShadows) - : AnimatedPlayPause(color: Colors.white, playing: isPlaying, shadows: _controlShadows), - onPressed: () => _toggle(ref, isCasting), + ? const Icon(Icons.replay, color: Colors.white, shadows: VideoControls._controlShadows) + : AnimatedPlayPause( + color: Colors.white, + playing: isPlaying, + shadows: VideoControls._controlShadows, + ), + onPressed: () => _toggle(isCasting), ), const Spacer(), Text( @@ -91,7 +125,7 @@ class VideoControls extends HookConsumerWidget { color: Colors.white, fontWeight: FontWeight.w500, fontFeatures: [FontFeature.tabularFigures()], - shadows: _controlShadows, + shadows: VideoControls._controlShadows, ), ), const SizedBox(width: 12), @@ -107,7 +141,7 @@ class VideoControls extends HookConsumerWidget { padding: EdgeInsets.zero, onChangeStart: (_) => notifier.hold(), onChangeEnd: (_) => notifier.release(), - onChanged: isLoaded ? (value) => _onSeek(ref, isCasting, value) : null, + onChanged: isLoaded ? (value) => _onSeek(isCasting, value) : null, ), ], ),