From aefa62b2349e56582fbe81de74aaf9d3d94b0bf6 Mon Sep 17 00:00:00 2001 From: Alex Date: Wed, 13 Aug 2025 11:35:42 -0500 Subject: [PATCH] fix: asset_viewer page viewing experience (#20889) * fix: zoomed in effect on swiped when bottom sheet is open * fix: memory leaked * fix: asset out of range when swiping in asset_viewer --- .../lib/domain/services/timeline.service.dart | 30 +++++ .../asset_viewer/asset_viewer.page.dart | 111 ++++++++++++------ .../widgets/images/local_image_provider.dart | 23 +++- .../photo_view/src/photo_view_wrappers.dart | 5 +- 4 files changed, 126 insertions(+), 43 deletions(-) diff --git a/mobile/lib/domain/services/timeline.service.dart b/mobile/lib/domain/services/timeline.service.dart index 53a8bc671b..ab623720b7 100644 --- a/mobile/lib/domain/services/timeline.service.dart +++ b/mobile/lib/domain/services/timeline.service.dart @@ -169,6 +169,36 @@ class TimelineService { return _buffer.elementAt(index - _bufferOffset); } + /// Gets an asset at the given index, automatically loading the buffer if needed. + /// This is an async version that can handle out-of-range indices by loading the appropriate buffer. + Future getAssetAsync(int index) async { + if (index < 0 || index >= _totalAssets) { + return null; + } + + if (hasRange(index, 1)) { + return _buffer.elementAt(index - _bufferOffset); + } + + // Load the buffer containing the requested index + try { + final assets = await loadAssets(index, 1); + return assets.isNotEmpty ? assets.first : null; + } catch (e) { + return null; + } + } + + /// Safely gets an asset at the given index without throwing a RangeError. + /// Returns null if the index is out of bounds or not currently in the buffer. + /// For automatic buffer loading, use getAssetAsync instead. + BaseAsset? getAssetSafe(int index) { + if (index < 0 || index >= _totalAssets || !hasRange(index, 1)) { + return null; + } + return _buffer.elementAt(index - _bufferOffset); + } + Future dispose() async { await _bucketSubscription?.cancel(); _bucketSubscription = null; diff --git a/mobile/lib/presentation/widgets/asset_viewer/asset_viewer.page.dart b/mobile/lib/presentation/widgets/asset_viewer/asset_viewer.page.dart index a56c778e48..3555aff6e6 100644 --- a/mobile/lib/presentation/widgets/asset_viewer/asset_viewer.page.dart +++ b/mobile/lib/presentation/widgets/asset_viewer/asset_viewer.page.dart @@ -127,20 +127,21 @@ class _AssetViewerState extends ConsumerState { _delayedOperations.clear(); } - // This is used to calculate the scale of the asset when the bottom sheet is showing. - // It is a small increment to ensure that the asset is slightly zoomed in when the - // bottom sheet is showing, which emulates the zoom effect. - double get _getScaleForBottomSheet => (viewController?.prevValue.scale ?? viewController?.value.scale ?? 1.0) + 0.01; - double _getVerticalOffsetForBottomSheet(double extent) => (context.height * extent) - (context.height * _kBottomSheetMinimumExtent); Future _precacheImage(int index) async { - if (!mounted || index < 0 || index >= totalAssets) { + if (!mounted) { + return; + } + + final timelineService = ref.read(timelineServiceProvider); + final asset = await timelineService.getAssetAsync(index); + + if (asset == null || !mounted) { return; } - final asset = ref.read(timelineServiceProvider).getAsset(index); final screenSize = Size(context.width, context.height); // Precache both thumbnail and full image for smooth transitions @@ -152,8 +153,15 @@ class _AssetViewerState extends ConsumerState { ); } - void _onAssetChanged(int index) { - final asset = ref.read(timelineServiceProvider).getAsset(index); + void _onAssetChanged(int index) async { + // Validate index bounds and try to get asset, loading buffer if needed + final timelineService = ref.read(timelineServiceProvider); + final asset = await timelineService.getAssetAsync(index); + + if (asset == null) { + return; + } + // Always holds the current asset from the timeline ref.read(assetViewerProvider.notifier).setAsset(asset); // The currentAssetNotifier actually holds the current asset that is displayed @@ -217,19 +225,15 @@ class _AssetViewerState extends ConsumerState { final verticalOffset = (context.height * bottomSheetController.size) - (context.height * _kBottomSheetMinimumExtent); controller.position = Offset(0, -verticalOffset); + // Apply the zoom effect when the bottom sheet is showing + initialScale = controller.scale; + controller.scale = (controller.scale ?? 1.0) + 0.01; } } void _onPageChanged(int index, PhotoViewControllerBase? controller) { _onAssetChanged(index); viewController = controller; - - // If the bottom sheet is showing, we need to adjust scale the asset to - // emulate the zoom effect - if (showingBottomSheet) { - initialScale = controller?.scale; - controller?.scale = _getScaleForBottomSheet; - } } void _onDragStart( @@ -412,16 +416,22 @@ class _AssetViewerState extends ConsumerState { } } - void _onAssetReloadEvent() { - setState(() { - final index = pageController.page?.round() ?? 0; - final newAsset = ref.read(timelineServiceProvider).getAsset(index); - final currentAsset = ref.read(currentAssetNotifier); - // Do not reload / close the bottom sheet if the asset has not changed - if (newAsset.heroTag == currentAsset?.heroTag) { - return; - } + void _onAssetReloadEvent() async { + final index = pageController.page?.round() ?? 0; + final timelineService = ref.read(timelineServiceProvider); + final newAsset = await timelineService.getAssetAsync(index); + if (newAsset == null) { + return; + } + + final currentAsset = ref.read(currentAssetNotifier); + // Do not reload / close the bottom sheet if the asset has not changed + if (newAsset.heroTag == currentAsset?.heroTag) { + return; + } + + setState(() { _onAssetChanged(pageController.page!.round()); sheetCloseController?.close(); }); @@ -430,7 +440,7 @@ class _AssetViewerState extends ConsumerState { void _openBottomSheet(BuildContext ctx, {double extent = _kBottomSheetMinimumExtent}) { ref.read(assetViewerProvider.notifier).setBottomSheet(true); initialScale = viewController?.scale; - viewController?.updateMultiple(scale: _getScaleForBottomSheet); + // viewController?.updateMultiple(scale: (viewController?.scale ?? 1.0) + 0.01); previousExtent = _kBottomSheetMinimumExtent; sheetCloseController = showBottomSheet( context: ctx, @@ -468,16 +478,29 @@ class _AssetViewerState extends ConsumerState { } Widget _placeholderBuilder(BuildContext ctx, ImageChunkEvent? progress, int index) { - BaseAsset asset = ref.read(timelineServiceProvider).getAsset(index); + final timelineService = ref.read(timelineServiceProvider); + final asset = timelineService.getAssetSafe(index); + + // If asset is not available in buffer, show a loading container + if (asset == null) { + return Container( + width: double.infinity, + height: double.infinity, + color: backgroundColor, + child: const Center(child: CircularProgressIndicator()), + ); + } + + BaseAsset displayAsset = asset; final stackChildren = ref.read(stackChildrenNotifier(asset)).valueOrNull; if (stackChildren != null && stackChildren.isNotEmpty) { - asset = stackChildren.elementAt(ref.read(assetViewerProvider.select((s) => s.stackIndex))); + displayAsset = stackChildren.elementAt(ref.read(assetViewerProvider.select((s) => s.stackIndex))); } return Container( width: double.infinity, height: double.infinity, color: backgroundColor, - child: Thumbnail(asset: asset, fit: BoxFit.contain), + child: Thumbnail(asset: displayAsset, fit: BoxFit.contain), ); } @@ -493,18 +516,34 @@ class _AssetViewerState extends ConsumerState { PhotoViewGalleryPageOptions _assetBuilder(BuildContext ctx, int index) { scaffoldContext ??= ctx; - BaseAsset asset = ref.read(timelineServiceProvider).getAsset(index); + final timelineService = ref.read(timelineServiceProvider); + final asset = timelineService.getAssetSafe(index); + + // If asset is not available in buffer, return a placeholder + if (asset == null) { + return PhotoViewGalleryPageOptions.customChild( + heroAttributes: PhotoViewHeroAttributes(tag: 'loading_$index'), + child: Container( + width: ctx.width, + height: ctx.height, + color: backgroundColor, + child: const Center(child: CircularProgressIndicator()), + ), + ); + } + + BaseAsset displayAsset = asset; final stackChildren = ref.read(stackChildrenNotifier(asset)).valueOrNull; if (stackChildren != null && stackChildren.isNotEmpty) { - asset = stackChildren.elementAt(ref.read(assetViewerProvider.select((s) => s.stackIndex))); + displayAsset = stackChildren.elementAt(ref.read(assetViewerProvider.select((s) => s.stackIndex))); } final isPlayingMotionVideo = ref.read(isPlayingMotionVideoProvider); - if (asset.isImage && !isPlayingMotionVideo) { - return _imageBuilder(ctx, asset); + if (displayAsset.isImage && !isPlayingMotionVideo) { + return _imageBuilder(ctx, displayAsset); } - return _videoBuilder(ctx, asset); + return _videoBuilder(ctx, displayAsset); } PhotoViewGalleryPageOptions _imageBuilder(BuildContext ctx, BaseAsset asset) { @@ -515,8 +554,6 @@ class _AssetViewerState extends ConsumerState { heroAttributes: PhotoViewHeroAttributes(tag: '${asset.heroTag}_$heroOffset'), filterQuality: FilterQuality.high, tightMode: true, - initialScale: PhotoViewComputedScale.contained * 0.999, - minScale: PhotoViewComputedScale.contained * 0.999, disableScaleGestures: showingBottomSheet, onDragStart: _onDragStart, onDragUpdate: _onDragUpdate, @@ -545,9 +582,7 @@ class _AssetViewerState extends ConsumerState { onTapDown: _onTapDown, heroAttributes: PhotoViewHeroAttributes(tag: '${asset.heroTag}_$heroOffset'), filterQuality: FilterQuality.high, - initialScale: PhotoViewComputedScale.contained * 0.99, maxScale: 1.0, - minScale: PhotoViewComputedScale.contained * 0.99, basePosition: Alignment.center, child: SizedBox( width: ctx.width, diff --git a/mobile/lib/presentation/widgets/images/local_image_provider.dart b/mobile/lib/presentation/widgets/images/local_image_provider.dart index 4da4b927f1..5508937dab 100644 --- a/mobile/lib/presentation/widgets/images/local_image_provider.dart +++ b/mobile/lib/presentation/widgets/images/local_image_provider.dart @@ -11,7 +11,6 @@ import 'package:immich_mobile/domain/services/setting.service.dart'; import 'package:immich_mobile/extensions/codec_extensions.dart'; import 'package:immich_mobile/infrastructure/repositories/asset_media.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/storage.repository.dart'; -import 'package:immich_mobile/presentation/widgets/images/image_provider.dart'; import 'package:immich_mobile/presentation/widgets/images/one_frame_multi_image_stream_completer.dart'; import 'package:immich_mobile/presentation/widgets/timeline/constants.dart'; import 'package:immich_mobile/providers/image/cache/thumbnail_image_cache_manager.dart'; @@ -107,7 +106,6 @@ class LocalFullImageProvider extends ImageProvider { ImageStreamCompleter loadImage(LocalFullImageProvider key, ImageDecoderCallback decode) { return OneFramePlaceholderImageStreamCompleter( _codec(key, decode), - initialImage: getCachedImage(LocalThumbProvider(id: key.id, updatedAt: key.updatedAt)), informationCollector: () => [ DiagnosticsProperty('Id', key.id), DiagnosticsProperty('Updated at', key.updatedAt), @@ -117,13 +115,30 @@ class LocalFullImageProvider extends ImageProvider { } // Streams in each stage of the image as we ask for it - Stream _codec(LocalFullImageProvider key, ImageDecoderCallback decode) { + Stream _codec(LocalFullImageProvider key, ImageDecoderCallback decode) async* { try { - return switch (key.type) { + // First, yield the thumbnail image from LocalThumbProvider + final thumbProvider = LocalThumbProvider(id: key.id, updatedAt: key.updatedAt); + try { + final thumbCodec = await thumbProvider._codec( + thumbProvider, + thumbProvider.cacheManager ?? ThumbnailImageCacheManager(), + decode, + ); + final thumbImageInfo = await thumbCodec.getImageInfo(); + yield thumbImageInfo; + } catch (_) {} + + // Then proceed with the main image loading stream + final mainStream = switch (key.type) { AssetType.image => _decodeProgressive(key, decode), AssetType.video => _getThumbnailCodec(key, decode), _ => throw StateError('Unsupported asset type ${key.type}'), }; + + await for (final imageInfo in mainStream) { + yield imageInfo; + } } catch (error, stack) { Logger('ImmichLocalImageProvider').severe('Error loading local image ${key.id}', error, stack); throw const ImageLoadingException('Could not load image from local storage'); diff --git a/mobile/lib/widgets/photo_view/src/photo_view_wrappers.dart b/mobile/lib/widgets/photo_view/src/photo_view_wrappers.dart index 4f283e55fd..65037dde96 100644 --- a/mobile/lib/widgets/photo_view/src/photo_view_wrappers.dart +++ b/mobile/lib/widgets/photo_view/src/photo_view_wrappers.dart @@ -86,6 +86,7 @@ class _ImageWrapperState extends State { Size? _imageSize; Object? _lastException; StackTrace? _lastStack; + bool _didLoadSynchronously = false; @override void dispose() { @@ -130,9 +131,11 @@ class _ImageWrapperState extends State { _loadingProgress = null; _lastException = null; _lastStack = null; + + _didLoadSynchronously = synchronousCall; } - synchronousCall ? setupCB() : setState(setupCB); + synchronousCall && !_didLoadSynchronously ? setupCB() : setState(setupCB); } void handleError(dynamic error, StackTrace? stackTrace) {