fix(mobile): asset state remain in gallery view after being deleted (#10603)

* fix(mobile): asset doesn't get removed from state renderList * fix delete last assets * refactor
2025-01-14 15:45:55 +02:00 · 2024-06-26 21:15:26 -07:00 · 2024-06-26 21:15:26 -07:00 · d8175d8da8
commit d8175d8da8
parent 922430da36
8 changed files with 158 additions and 140 deletions
--- a/mobile/lib/pages/common/gallery_viewer.page.dart
+++ b/mobile/lib/pages/common/gallery_viewer.page.dart
@ -19,6 +19,7 @@ import 'package:immich_mobile/providers/asset_viewer/video_player_value_provider
 import 'package:immich_mobile/providers/haptic_feedback.provider.dart';
 import 'package:immich_mobile/providers/image/immich_remote_image_provider.dart';
 import 'package:immich_mobile/services/app_settings.service.dart';
+import 'package:immich_mobile/widgets/asset_grid/asset_grid_data_structure.dart';
 import 'package:immich_mobile/widgets/asset_viewer/advanced_bottom_sheet.dart';
 import 'package:immich_mobile/widgets/asset_viewer/bottom_gallery_bar.dart';
 import 'package:immich_mobile/widgets/asset_viewer/exif_sheet/exif_bottom_sheet.dart';
@ -34,17 +35,15 @@ import 'package:isar/isar.dart';
@RoutePage()
 // ignore: must_be_immutable
 class GalleryViewerPage extends HookConsumerWidget {
-  final Asset Function(int index) loadAsset;
-  final int totalAssets;
  final int initialIndex;
  final int heroOffset;
  final bool showStack;
+  final RenderList renderList;

  GalleryViewerPage({
    super.key,
-    required this.initialIndex,
-    required this.loadAsset,
-    required this.totalAssets,
+    required this.renderList,
+    this.initialIndex = 0,
    this.heroOffset = 0,
    this.showStack = false,
  }) : controller = PageController(initialPage: initialIndex);
@ -54,6 +53,8 @@ class GalleryViewerPage extends HookConsumerWidget {
  @override
  Widget build(BuildContext context, WidgetRef ref) {
    final settings = ref.watch(appSettingsServiceProvider);
+    final loadAsset = renderList.loadAsset;
+    final totalAssets = useState(renderList.totalAssets);
    final isLoadPreview = useState(AppSettingsEnum.loadPreview.defaultValue);
    final isLoadOriginal = useState(AppSettingsEnum.loadOriginal.defaultValue);
    final shouldLoopVideo = useState(AppSettingsEnum.loopVideo.defaultValue);
@ -62,6 +63,7 @@ class GalleryViewerPage extends HookConsumerWidget {
    final localPosition = useState<Offset?>(null);
    final currentIndex = useState(initialIndex);
    final currentAsset = loadAsset(currentIndex.value);
+
    // Update is playing motion video
    ref.listen(videoPlaybackValueProvider.select((v) => v.state), (_, state) {
      isPlayingVideo.value = state == VideoPlaybackState.playing;
@ -112,13 +114,19 @@ class GalleryViewerPage extends HookConsumerWidget {
        debugPrint('Error precaching next image: $exception, $stackTrace');
      }

-      if (index < totalAssets && index >= 0) {
-        final asset = loadAsset(index);
-        await precacheImage(
-          ImmichImage.imageProvider(asset: asset),
-          context,
-          onError: onError,
-        );
+      try {
+        if (index < totalAssets.value && index >= 0) {
+          final asset = loadAsset(index);
+          await precacheImage(
+            ImmichImage.imageProvider(asset: asset),
+            context,
+            onError: onError,
+          );
+        }
+      } catch (e) {
+        // swallow error silently
+        debugPrint('Error precaching next image: $e');
+        context.maybePop();
      }
    }

@ -297,7 +305,7 @@ class GalleryViewerPage extends HookConsumerWidget {
                      ? const ScrollPhysics() // Use bouncing physics for iOS
                      : const ClampingScrollPhysics() // Use heavy physics for Android
                  ),
-              itemCount: totalAssets,
+              itemCount: totalAssets.value,
              scrollDirection: Axis.horizontal,
              onPageChanged: (value) async {
                final next = currentIndex.value < value ? value + 1 : value - 1;
@ -406,11 +414,13 @@ class GalleryViewerPage extends HookConsumerWidget {
                    ),
                  ),
                  BottomGalleryBar(
+                    renderList: renderList,
                    totalAssets: totalAssets,
                    controller: controller,
                    showStack: showStack,
                    stackIndex: stackIndex.value,
                    asset: asset,
+                    assetIndex: currentIndex.value,
                    showVideoPlayerControls: !asset.isImage && !isMotionPhoto,
                  ),
                ],
--- a/mobile/lib/pages/search/map/map.page.dart
+++ b/mobile/lib/pages/search/map/map.page.dart
@ -16,6 +16,7 @@ import 'package:immich_mobile/models/map/map_marker.model.dart';
 import 'package:immich_mobile/providers/map/map_marker.provider.dart';
 import 'package:immich_mobile/providers/map/map_state.provider.dart';
 import 'package:immich_mobile/utils/map_utils.dart';
+import 'package:immich_mobile/widgets/asset_grid/asset_grid_data_structure.dart';
 import 'package:immich_mobile/widgets/map/map_app_bar.dart';
 import 'package:immich_mobile/widgets/map/map_asset_grid.dart';
 import 'package:immich_mobile/widgets/map/map_bottom_sheet.dart';
@ -178,12 +179,17 @@ class MapPage extends HookConsumerWidget {
        return;
      }

+      // Since we only have a single asset, we can just show GroupAssetBy.none
+      final renderList = await RenderList.fromAssets(
+        [asset],
+        GroupAssetsBy.none,
+      );
+
      context.pushRoute(
        GalleryViewerRoute(
          initialIndex: 0,
-          loadAsset: (index) => asset,
-          totalAssets: 1,
          heroOffset: 0,
+          renderList: renderList,
        ),
      );
    }
--- a/mobile/lib/routing/router.dart
+++ b/mobile/lib/routing/router.dart
@ -59,6 +59,7 @@ import 'package:immich_mobile/routing/backup_permission_guard.dart';
 import 'package:immich_mobile/routing/custom_transition_builders.dart';
 import 'package:immich_mobile/routing/duplicate_guard.dart';
 import 'package:immich_mobile/services/api.service.dart';
+import 'package:immich_mobile/widgets/asset_grid/asset_grid_data_structure.dart';
 import 'package:isar/isar.dart';
 import 'package:maplibre_gl/maplibre_gl.dart';
 import 'package:photo_manager/photo_manager.dart' hide LatLng;
--- a/mobile/lib/routing/router.gr.dart
+++ b/mobile/lib/routing/router.gr.dart
@ -183,9 +183,8 @@ abstract class _$AppRouter extends RootStackRouter {
        routeData: routeData,
        child: GalleryViewerPage(
          key: args.key,
+          renderList: args.renderList,
          initialIndex: args.initialIndex,
-          loadAsset: args.loadAsset,
-          totalAssets: args.totalAssets,
          heroOffset: args.heroOffset,
          showStack: args.showStack,
        ),
@ -870,9 +869,8 @@ class FavoritesRoute extends PageRouteInfo<void> {
 class GalleryViewerRoute extends PageRouteInfo<GalleryViewerRouteArgs> {
  GalleryViewerRoute({
    Key? key,
-    required int initialIndex,
-    required Asset Function(int) loadAsset,
-    required int totalAssets,
+    required RenderList renderList,
+    int initialIndex = 0,
    int heroOffset = 0,
    bool showStack = false,
    List<PageRouteInfo>? children,
@ -880,9 +878,8 @@ class GalleryViewerRoute extends PageRouteInfo<GalleryViewerRouteArgs> {
          GalleryViewerRoute.name,
          args: GalleryViewerRouteArgs(
            key: key,
+            renderList: renderList,
            initialIndex: initialIndex,
-            loadAsset: loadAsset,
-            totalAssets: totalAssets,
            heroOffset: heroOffset,
            showStack: showStack,
          ),
@ -898,28 +895,25 @@ class GalleryViewerRoute extends PageRouteInfo<GalleryViewerRouteArgs> {
 class GalleryViewerRouteArgs {
  const GalleryViewerRouteArgs({
    this.key,
-    required this.initialIndex,
-    required this.loadAsset,
-    required this.totalAssets,
+    required this.renderList,
+    this.initialIndex = 0,
    this.heroOffset = 0,
    this.showStack = false,
  });

  final Key? key;

+  final RenderList renderList;
+
  final int initialIndex;

-  final Asset Function(int) loadAsset;
-
-  final int totalAssets;
-
  final int heroOffset;

  final bool showStack;

  @override
  String toString() {
-    return 'GalleryViewerRouteArgs{key: $key, initialIndex: $initialIndex, loadAsset: $loadAsset, totalAssets: $totalAssets, heroOffset: $heroOffset, showStack: $showStack}';
+    return 'GalleryViewerRouteArgs{key: $key, renderList: $renderList, initialIndex: $initialIndex, heroOffset: $heroOffset, showStack: $showStack}';
  }
 }

--- a/mobile/lib/widgets/asset_grid/asset_grid_data_structure.dart
+++ b/mobile/lib/widgets/asset_grid/asset_grid_data_structure.dart
@ -311,4 +311,12 @@ class RenderList {
    GroupAssetsBy groupBy,
  ) =>
      _buildRenderList(assets, null, groupBy);
+
+  /// Deletes an asset from the render list and clears the buffer
+  /// This is only a workaround for deleted images still appearing in the gallery
+  void deleteAsset(Asset deleteAsset) {
+    allAssets?.remove(deleteAsset);
+    _buf.clear();
+    _bufOffset = 0;
+  }
 }
--- a/mobile/lib/widgets/asset_grid/immich_asset_grid_view.dart
+++ b/mobile/lib/widgets/asset_grid/immich_asset_grid_view.dart
@ -2,10 +2,12 @@ import 'dart:collection';
 import 'dart:developer';
 import 'dart:math';

+import 'package:auto_route/auto_route.dart';
 import 'package:collection/collection.dart';
 import 'package:easy_localization/easy_localization.dart';
 import 'package:flutter/material.dart';
 import 'package:flutter/rendering.dart';
+import 'package:flutter/services.dart';
 import 'package:hooks_riverpod/hooks_riverpod.dart';
 import 'package:immich_mobile/extensions/build_context_extensions.dart';
 import 'package:immich_mobile/extensions/collection_extensions.dart';
@ -815,6 +817,7 @@ class _AssetRow extends StatelessWidget {
      key: key,
      children: assets.mapIndexed((int index, Asset asset) {
        final bool last = index + 1 == assetsPerRow;
+        final isSelected = isSelectionActive && selectedAssets.contains(asset);
        return Container(
          width: width * widthDistribution[index],
          height: width,
@ -822,21 +825,40 @@ class _AssetRow extends StatelessWidget {
            bottom: margin,
            right: last ? 0.0 : margin,
          ),
-          child: AssetIndexWrapper(
-            rowIndex: rowStartIndex + index,
-            sectionIndex: sectionIndex,
-            child: ThumbnailImage(
-              asset: asset,
-              index: absoluteOffset + index,
-              loadAsset: renderList.loadAsset,
-              totalAssets: renderList.totalAssets,
-              multiselectEnabled: selectionActive,
-              isSelected: isSelectionActive && selectedAssets.contains(asset),
-              onSelect: () => onSelect?.call(asset),
-              onDeselect: () => onDeselect?.call(asset),
-              showStorageIndicator: showStorageIndicator,
-              heroOffset: heroOffset,
-              showStack: showStack,
+          child: GestureDetector(
+            onTap: () {
+              if (selectionActive) {
+                if (isSelected) {
+                  onDeselect?.call(asset);
+                } else {
+                  onSelect?.call(asset);
+                }
+              } else {
+                context.pushRoute(
+                  GalleryViewerRoute(
+                    renderList: renderList,
+                    initialIndex: absoluteOffset + index,
+                    heroOffset: heroOffset,
+                    showStack: showStack,
+                  ),
+                );
+              }
+            },
+            onLongPress: () {
+              onSelect?.call(asset);
+              HapticFeedback.heavyImpact();
+            },
+            child: AssetIndexWrapper(
+              rowIndex: rowStartIndex + index,
+              sectionIndex: sectionIndex,
+              child: ThumbnailImage(
+                asset: asset,
+                multiselectEnabled: selectionActive,
+                isSelected: isSelectionActive && selectedAssets.contains(asset),
+                showStorageIndicator: showStorageIndicator,
+                heroOffset: heroOffset,
+                showStack: showStack,
+              ),
            ),
          ),
        );
--- a/mobile/lib/widgets/asset_grid/thumbnail_image.dart
+++ b/mobile/lib/widgets/asset_grid/thumbnail_image.dart
@ -1,40 +1,42 @@
-import 'package:auto_route/auto_route.dart';
 import 'package:flutter/material.dart';
 import 'package:hooks_riverpod/hooks_riverpod.dart';
 import 'package:immich_mobile/extensions/build_context_extensions.dart';
-import 'package:immich_mobile/routing/router.dart';
 import 'package:immich_mobile/entities/asset.entity.dart';
-import 'package:immich_mobile/providers/haptic_feedback.provider.dart';
 import 'package:immich_mobile/widgets/common/immich_thumbnail.dart';
 import 'package:immich_mobile/utils/storage_indicator.dart';
 import 'package:isar/isar.dart';

 class ThumbnailImage extends ConsumerWidget {
+  /// The asset to show the thumbnail image for
  final Asset asset;
-  final int index;
-  final Asset Function(int index) loadAsset;
-  final int totalAssets;
+
+  /// Whether to show the storage indicator icont over the image or not
  final bool showStorageIndicator;
+
+  /// Whether to show the show stack icon over the image or not
  final bool showStack;
+
+  /// Whether to show the checkmark indicating that this image is selected
  final bool isSelected;
+
+  /// Can override [isSelected] and never show the selection indicator
  final bool multiselectEnabled;
-  final Function? onSelect;
-  final Function? onDeselect;
+
+  /// If we are allowed to deselect this image
+  final bool canDeselect;
+
+  /// The offset index to apply to this hero tag for animation
  final int heroOffset;

  const ThumbnailImage({
    super.key,
    required this.asset,
-    required this.index,
-    required this.loadAsset,
-    required this.totalAssets,
    this.showStorageIndicator = true,
    this.showStack = false,
    this.isSelected = false,
    this.multiselectEnabled = false,
-    this.onDeselect,
-    this.onSelect,
    this.heroOffset = 0,
+    this.canDeselect = true,
  });

  @override
@ -147,11 +149,7 @@ class ThumbnailImage extends ConsumerWidget {
      }
      return Container(
        decoration: BoxDecoration(
-          border: Border.all(
-            width: 0,
-            color: onDeselect == null ? Colors.grey : assetContainerColor,
-          ),
-          color: onDeselect == null ? Colors.grey : assetContainerColor,
+          color: canDeselect ? assetContainerColor : Colors.grey,
        ),
        child: ClipRRect(
          borderRadius: const BorderRadius.only(
@ -165,79 +163,52 @@ class ThumbnailImage extends ConsumerWidget {
      );
    }

-    return GestureDetector(
-      onTap: () {
-        if (multiselectEnabled) {
-          if (isSelected) {
-            onDeselect?.call();
-          } else {
-            onSelect?.call();
-          }
-        } else {
-          context.pushRoute(
-            GalleryViewerRoute(
-              initialIndex: index,
-              loadAsset: loadAsset,
-              totalAssets: totalAssets,
-              heroOffset: heroOffset,
-              showStack: showStack,
-            ),
-          );
-        }
-      },
-      onLongPress: () {
-        onSelect?.call();
-        ref.read(hapticFeedbackProvider.notifier).heavyImpact();
-      },
-      child: Stack(
-        children: [
-          AnimatedContainer(
-            duration: const Duration(milliseconds: 300),
-            curve: Curves.decelerate,
-            decoration: BoxDecoration(
-              border: multiselectEnabled && isSelected
-                  ? Border.all(
-                      color: onDeselect == null
-                          ? Colors.grey
-                          : assetContainerColor,
-                      width: 8,
-                    )
-                  : const Border(),
-            ),
-            child: buildImage(),
+    return Stack(
+      children: [
+        AnimatedContainer(
+          duration: const Duration(milliseconds: 300),
+          curve: Curves.decelerate,
+          decoration: BoxDecoration(
+            border: multiselectEnabled && isSelected
+                ? Border.all(
+                    color: canDeselect ? assetContainerColor : Colors.grey,
+                    width: 8,
+                  )
+                : const Border(),
          ),
-          if (multiselectEnabled)
-            Padding(
-              padding: const EdgeInsets.all(3.0),
-              child: Align(
-                alignment: Alignment.topLeft,
-                child: buildSelectionIcon(asset),
-              ),
+          child: buildImage(),
+        ),
+        if (multiselectEnabled)
+          Padding(
+            padding: const EdgeInsets.all(3.0),
+            child: Align(
+              alignment: Alignment.topLeft,
+              child: buildSelectionIcon(asset),
            ),
-          if (showStorageIndicator)
-            Positioned(
-              right: 8,
-              bottom: 5,
-              child: Icon(
-                storageIcon(asset),
-                color: Colors.white,
-                size: 18,
-              ),
+          ),
+        if (showStorageIndicator)
+          Positioned(
+            right: 8,
+            bottom: 5,
+            child: Icon(
+              storageIcon(asset),
+              color: Colors.white,
+              size: 18,
            ),
-          if (asset.isFavorite)
-            const Positioned(
-              left: 8,
-              bottom: 5,
-              child: Icon(
-                Icons.favorite,
-                color: Colors.white,
-                size: 18,
-              ),
+          ),
+        if (asset.isFavorite)
+          const Positioned(
+            left: 8,
+            bottom: 5,
+            child: Icon(
+              Icons.favorite,
+              color: Colors.white,
+              size: 18,
            ),
-          if (!asset.isImage) buildVideoIcon(),
-          if (asset.stackChildrenCount > 0) buildStackIcon(),
-        ],
-      ),
+          ),
+        if (!asset.isImage) buildVideoIcon(),
+        if (asset.stackChildrenCount > 0) buildStackIcon(),
+      ],
    );
  }
 }
--- a/mobile/lib/widgets/asset_viewer/bottom_gallery_bar.dart
+++ b/mobile/lib/widgets/asset_viewer/bottom_gallery_bar.dart
@ -10,6 +10,7 @@ import 'package:immich_mobile/providers/asset_viewer/asset_stack.provider.dart';
 import 'package:immich_mobile/providers/asset_viewer/image_viewer_page_state.provider.dart';
 import 'package:immich_mobile/providers/asset_viewer/show_controls.provider.dart';
 import 'package:immich_mobile/services/asset_stack.service.dart';
+import 'package:immich_mobile/widgets/asset_grid/asset_grid_data_structure.dart';
 import 'package:immich_mobile/widgets/asset_viewer/video_controls.dart';
 import 'package:immich_mobile/widgets/asset_grid/delete_dialog.dart';
 import 'package:immich_mobile/routing/router.dart';
@ -21,20 +22,24 @@ import 'package:immich_mobile/widgets/common/immich_toast.dart';

 class BottomGalleryBar extends ConsumerWidget {
  final Asset asset;
+  final int assetIndex;
  final bool showStack;
  final int stackIndex;
-  final int totalAssets;
+  final ValueNotifier<int> totalAssets;
  final bool showVideoPlayerControls;
  final PageController controller;
+  final RenderList renderList;

  const BottomGalleryBar({
    super.key,
    required this.showStack,
    required this.stackIndex,
    required this.asset,
+    required this.assetIndex,
    required this.controller,
    required this.totalAssets,
    required this.showVideoPlayerControls,
+    required this.renderList,
  });

  @override
@ -108,16 +113,17 @@ class BottomGalleryBar extends ConsumerWidget {
          force: force,
        );
        if (isDeleted && isParent) {
-          if (totalAssets == 1) {
+          // Workaround for asset remaining in the gallery
+          renderList.deleteAsset(asset);
+
+          // `assetIndex == totalAssets.value - 1` handle the case of removing the last asset
+          // to not throw the error when the next preCache index is called
+          if (totalAssets.value == 1 || assetIndex == totalAssets.value - 1) {
            // Handle only one asset
            context.maybePop();
-          } else {
-            // Go to next page otherwise
-            controller.nextPage(
-              duration: const Duration(milliseconds: 100),
-              curve: Curves.fastLinearToSlowEaseIn,
-            );
          }
+
+          totalAssets.value -= 1;
        }
        return isDeleted;
      }