From 35c2a90fcacd98f3135319152aef27ef88ddaf46 Mon Sep 17 00:00:00 2001 From: Santo Shakil Date: Sun, 9 Aug 2026 23:54:55 +0600 Subject: [PATCH] fix(mobile): video playback controls dead for backed up videos opened from search (#30587) * key video player state by asset id instead of hero tag * expose id on BaseAsset instead of a separate playerKey --- .../domain/models/asset/base_asset.model.dart | 1 + .../models/asset/local_asset.model.dart | 1 + .../models/asset/remote_asset.model.dart | 1 + .../pages/drift_slideshow.page.dart | 14 +-- .../asset_viewer/bottom_bar.widget.dart | 2 +- .../asset_viewer/video_viewer.widget.dart | 4 +- .../asset_viewer/asset_viewer.provider.dart | 6 +- .../test/domain/models/base_asset_test.dart | 27 ++++++ .../widgets/asset_viewer/bottom_bar_test.dart | 85 +++++++++++++++++++ 9 files changed, 128 insertions(+), 13 deletions(-) create mode 100644 mobile/test/presentation/widgets/asset_viewer/bottom_bar_test.dart diff --git a/mobile/lib/domain/models/asset/base_asset.model.dart b/mobile/lib/domain/models/asset/base_asset.model.dart index d7d74daa25..ac89fe9a0a 100644 --- a/mobile/lib/domain/models/asset/base_asset.model.dart +++ b/mobile/lib/domain/models/asset/base_asset.model.dart @@ -77,6 +77,7 @@ sealed class BaseAsset { // Overridden in subclasses AssetState get storage; + String get id; String? get localId; String? get remoteId; String get heroTag; diff --git a/mobile/lib/domain/models/asset/local_asset.model.dart b/mobile/lib/domain/models/asset/local_asset.model.dart index 0ad4c8f39b..15039761a8 100644 --- a/mobile/lib/domain/models/asset/local_asset.model.dart +++ b/mobile/lib/domain/models/asset/local_asset.model.dart @@ -1,6 +1,7 @@ part of 'base_asset.model.dart'; class LocalAsset extends BaseAsset { + @override final String id; final String? remoteAssetId; final String? cloudId; diff --git a/mobile/lib/domain/models/asset/remote_asset.model.dart b/mobile/lib/domain/models/asset/remote_asset.model.dart index 387a817eab..5707329ba3 100644 --- a/mobile/lib/domain/models/asset/remote_asset.model.dart +++ b/mobile/lib/domain/models/asset/remote_asset.model.dart @@ -4,6 +4,7 @@ enum AssetVisibility { timeline, hidden, archive, locked } // Model for an asset stored in the server class RemoteAsset extends BaseAsset { + @override final String id; final String? localAssetId; final String? thumbHash; diff --git a/mobile/lib/presentation/pages/drift_slideshow.page.dart b/mobile/lib/presentation/pages/drift_slideshow.page.dart index e6c78796d3..48b29b3499 100644 --- a/mobile/lib/presentation/pages/drift_slideshow.page.dart +++ b/mobile/lib/presentation/pages/drift_slideshow.page.dart @@ -93,8 +93,8 @@ class _DriftSlideshowPageState extends ConsumerState with Si if (asset.isImage) { _createTimer(); - } else if (ref.read(videoPlayerProvider(asset.heroTag)).status == VideoPlaybackStatus.paused) { - unawaited(ref.read(videoPlayerProvider(asset.heroTag).notifier).play()); + } else if (ref.read(videoPlayerProvider(asset.id)).status == VideoPlaybackStatus.paused) { + unawaited(ref.read(videoPlayerProvider(asset.id).notifier).play()); } else { unawaited(_nextPage()); } @@ -113,7 +113,7 @@ class _DriftSlideshowPageState extends ConsumerState with Si final asset = widget.timeline.getAssetSafe(_index)!; if (!asset.isImage) { - unawaited(ref.read(videoPlayerProvider(asset.heroTag).notifier).pause()); + unawaited(ref.read(videoPlayerProvider(asset.id).notifier).pause()); } setState(() { @@ -300,7 +300,7 @@ class _DriftSlideshowPageState extends ConsumerState with Si borderRadius: BorderRadius.zero, minHeight: 5, value: - ref.read(videoPlayerProvider(asset.heroTag).select((s) => s.position)).inMilliseconds / + ref.read(videoPlayerProvider(asset.id).select((s) => s.position)).inMilliseconds / asset.duration.inMilliseconds, ); } @@ -374,13 +374,13 @@ class _DriftSlideshowPageState extends ConsumerState with Si builder: (context, value, _) => buildPhotoView(scale * (1.0 + value * _kenBurnsZoom)), ); } else { - final status = ref.read(videoPlayerProvider(asset.heroTag).select((s) => s.status)); - final position = ref.read(videoPlayerProvider(asset.heroTag)).position; + final status = ref.read(videoPlayerProvider(asset.id).select((s) => s.status)); + final position = ref.read(videoPlayerProvider(asset.id)).position; if (status == VideoPlaybackStatus.completed && isCurrent && position.inMicroseconds > 0) { unawaited(_nextPage()); } else if (status == VideoPlaybackStatus.playing) { - unawaited(ref.read(videoPlayerProvider(asset.heroTag).notifier).setLoop(false)); + unawaited(ref.read(videoPlayerProvider(asset.id).notifier).setLoop(false)); } return PhotoView.customChild( 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 4ff839881a..15b90e58ea 100644 --- a/mobile/lib/presentation/widgets/asset_viewer/bottom_bar.widget.dart +++ b/mobile/lib/presentation/widgets/asset_viewer/bottom_bar.widget.dart @@ -94,7 +94,7 @@ class ViewerBottomBar extends ConsumerWidget { mainAxisSize: MainAxisSize.min, children: [ if (asset.isImage) OcrToggleButton(asset: asset), - if (asset.isVideo) VideoControls(videoPlayerName: asset.heroTag), + if (asset.isVideo) VideoControls(videoPlayerName: asset.id), if (!isReadonlyModeEnabled) ImmichColorOverride( color: Colors.white, diff --git a/mobile/lib/presentation/widgets/asset_viewer/video_viewer.widget.dart b/mobile/lib/presentation/widgets/asset_viewer/video_viewer.widget.dart index 8b3d8a9978..9011888b54 100644 --- a/mobile/lib/presentation/widgets/asset_viewer/video_viewer.widget.dart +++ b/mobile/lib/presentation/widgets/asset_viewer/video_viewer.widget.dart @@ -47,7 +47,7 @@ class _NativeVideoViewerState extends ConsumerState with Widg bool _isVideoReady = false; bool _shouldPlayOnForeground = true; - VideoPlayerNotifier get _notifier => ref.read(videoPlayerProvider(widget.asset.heroTag).notifier); + VideoPlayerNotifier get _notifier => ref.read(videoPlayerProvider(widget.asset.id).notifier); @override void initState() { @@ -304,7 +304,7 @@ class _NativeVideoViewerState extends ConsumerState with Widg @override Widget build(BuildContext context) { final isCasting = ref.watch(castProvider.select((c) => c.isCasting)); - final status = ref.watch(videoPlayerProvider(widget.asset.heroTag).select((v) => v.status)); + final status = ref.watch(videoPlayerProvider(widget.asset.id).select((v) => v.status)); return IgnorePointer( child: Stack( diff --git a/mobile/lib/providers/asset_viewer/asset_viewer.provider.dart b/mobile/lib/providers/asset_viewer/asset_viewer.provider.dart index fd2a6aebd9..ac929d2b6a 100644 --- a/mobile/lib/providers/asset_viewer/asset_viewer.provider.dart +++ b/mobile/lib/providers/asset_viewer/asset_viewer.provider.dart @@ -67,9 +67,9 @@ class AssetViewerStateNotifier extends Notifier { } state = state.copyWith(showingDetails: showing, showingControls: showing ? true : state.showingControls); - final heroTag = state.currentAsset?.heroTag; - if (heroTag != null) { - final notifier = ref.read(videoPlayerProvider(heroTag).notifier); + final id = state.currentAsset?.id; + if (id != null) { + final notifier = ref.read(videoPlayerProvider(id).notifier); showing ? notifier.hold() : notifier.release(); } } diff --git a/mobile/test/domain/models/base_asset_test.dart b/mobile/test/domain/models/base_asset_test.dart index 4c2eb3ca38..6a6c836aa7 100644 --- a/mobile/test/domain/models/base_asset_test.dart +++ b/mobile/test/domain/models/base_asset_test.dart @@ -43,4 +43,31 @@ void main() { expect(localOnly.refersToSameAsset(remoteOnly), isTrue); }); }); + + group('BaseAsset.id', () { + test('survives the DB copy filling localId', () { + final searchCopy = RemoteAssetFactory.create(id: 'asset-1'); + final mergedCopy = searchCopy.copyWith(localId: 'local-1'); + + expect(searchCopy.heroTag, isNot(mergedCopy.heroTag)); + expect(mergedCopy.id, searchCopy.id); + }); + + test('survives a local asset gaining a remote id', () { + final localOnly = LocalAssetFactory.create(id: 'local-1'); + final uploaded = localOnly.copyWith(remoteId: 'asset-1'); + + expect(localOnly.heroTag, isNot(uploaded.heroTag)); + expect(uploaded.id, localOnly.id); + }); + + test('differs between assets', () { + final a = RemoteAssetFactory.create(id: 'asset-1'); + final b = RemoteAssetFactory.create(id: 'asset-2'); + final local = LocalAssetFactory.create(id: 'local-1'); + + expect(a.id, isNot(b.id)); + expect(local.id, isNot(a.id)); + }); + }); } diff --git a/mobile/test/presentation/widgets/asset_viewer/bottom_bar_test.dart b/mobile/test/presentation/widgets/asset_viewer/bottom_bar_test.dart new file mode 100644 index 0000000000..f2463c090d --- /dev/null +++ b/mobile/test/presentation/widgets/asset_viewer/bottom_bar_test.dart @@ -0,0 +1,85 @@ +import 'dart:async'; + +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:hooks_riverpod/hooks_riverpod.dart'; +import 'package:immich_mobile/constants/enums.dart'; +import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; +import 'package:immich_mobile/domain/services/timeline.service.dart'; +import 'package:immich_mobile/presentation/actions/action.dart'; +import 'package:immich_mobile/presentation/widgets/asset_viewer/bottom_bar.widget.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/infrastructure/asset.provider.dart'; +import 'package:immich_mobile/providers/infrastructure/readonly_mode.provider.dart'; +import 'package:immich_mobile/providers/infrastructure/timeline.provider.dart'; +import 'package:immich_mobile/providers/routes.provider.dart'; +import 'package:immich_mobile/services/gcast.service.dart'; +import 'package:immich_mobile/utils/asset_filter.dart'; +import 'package:mocktail/mocktail.dart'; +import 'package:native_video_player/native_video_player.dart'; + +import '../../../service.mocks.dart'; +import '../../../unit/factories/remote_asset_factory.dart'; +import '../../../widget_tester_extensions.dart'; + +class MockNativeVideoPlayerController extends Mock implements NativeVideoPlayerController {} + +class MockTimelineService extends Mock implements TimelineService {} + +class TestReadOnlyModeNotifier extends ReadOnlyModeNotifier { + @override + bool build() => true; +} + +void main() { + testWidgets('player stays bound after local id arrives', (tester) async { + final searchCopy = RemoteAssetFactory.create(type: .video); + final mergedCopy = searchCopy.copyWith(localId: 'local-1', isFavorite: true); + final assetService = MockAssetService(); + final controller = MockNativeVideoPlayerController(); + final timeline = MockTimelineService(); + final updates = StreamController(sync: true); + when(() => assetService.watchAsset(searchCopy)).thenAnswer((_) => updates.stream); + when(controller.play).thenAnswer((_) async {}); + when(controller.pause).thenAnswer((_) async {}); + when(() => timeline.origin).thenReturn(.search); + + late WidgetRef ref; + + await tester.pumpConsumerWidget( + Consumer( + builder: (context, widgetRef, _) { + ref = widgetRef; + return const ViewerBottomBar(); + }, + ), + overrides: [ + assetServiceProvider.overrideWithValue(assetService), + assetsActionProvider(ActionSource.viewer).overrideWithValue(const AssetFilter({})), + gCastServiceProvider.overrideWithValue(MockGCastService()), + inLockedViewProvider.overrideWithValue(true), + ownedAssetsActionProvider(ActionSource.viewer).overrideWithValue(const AssetFilter({})), + readonlyModeProvider.overrideWith(TestReadOnlyModeNotifier.new), + timelineServiceProvider.overrideWithValue(timeline), + ], + ); + + final viewer = ref.read(assetViewerProvider.notifier); + viewer.setAsset(searchCopy); + await tester.pump(); + ref.read(videoPlayerProvider(searchCopy.id).notifier).attachController(controller); + updates.add(mergedCopy); + await tester.pump(); + await tester.tap(find.byType(IconButton)); + await tester.pump(); + + verify(controller.play).called(1); + viewer.setShowingDetails(true); + await tester.pump(); + verify(controller.pause).called(1); + + await tester.pumpWidget(const SizedBox.shrink()); + unawaited(updates.close()); + }); +}