Merge pull request #735 from submersion-app/worktree-photo-attach-windows

fix(media): repair photo attachment on Windows and Linux

Author: ericgriffin

lib/features/media/data/repositories/media_repository.dart

@@ -705,6 +705,34 @@ class MediaRepository {
     }
   }
 
+  /// Get the set of local file paths already linked to a specific dive.
+  ///
+  /// The desktop counterpart to [getLinkedAssetIdsForDive]: Windows / Linux
+  /// imports are `localFile` rows with a null `platform_asset_id`, so the
+  /// asset-id query cannot see them and duplicate detection has to key on the
+  /// path instead. The path is also the more stable key -- the desktop
+  /// picker's synthetic asset id embeds the file's mtime, so it changes
+  /// whenever the file is touched.
+  Future<Set<String>> getLinkedLocalPathsForDive(String diveId) async {
+    try {
+      final result = await _db
+          .customSelect(
+            'SELECT local_path FROM media '
+            'WHERE dive_id = ? AND local_path IS NOT NULL',
+            variables: [Variable.withString(diveId)],
+          )
+          .get();
+      return result.map((row) => row.data['local_path'] as String).toSet();
+    } catch (e, stackTrace) {
+      _log.error(
+        'Failed to get linked local paths for dive: $diveId',
+        error: e,
+        stackTrace: stackTrace,
+      );
+      rethrow;
+    }
+  }
+
   /// Get GPS coordinates from media attached to a dive.
   ///
   /// Returns a list of (latitude, longitude, takenAt) tuples from photos

lib/features/media/data/services/media_import_service.dart

@@ -112,15 +112,31 @@ class MediaImportService {
       'Starting import of ${selectedAssets.length} assets for dive ${dive.id}',
     );
 
-    // Fetch already-linked asset IDs for this dive
-    final existingAssetIds = await _mediaRepository.getLinkedAssetIdsForDive(
-      dive.id,
-    );
+    // Two dedupe keys, one per origin. Gallery picks are matched on their
+    // platform asset id; desktop picks are localFile rows with a null
+    // platform_asset_id (see [_createMediaItemFromAsset]), invisible to that
+    // query, so they are matched on the path instead.
+    //
+    // Each lookup only feeds one branch of the filter below, so query a
+    // lookup only when the selection actually contains that kind of asset:
+    // a mobile pick has no paths to compare and a desktop pick has no
+    // gallery ids, and either way the unused set would be dead work.
+    bool hasPath(AssetInfo a) => a.filePath != null && a.filePath!.isNotEmpty;
+    final anyPaths = selectedAssets.any(hasPath);
+    final anyGallery = selectedAssets.any((a) => !hasPath(a));
+
+    final existingAssetIds = anyGallery
+        ? await _mediaRepository.getLinkedAssetIdsForDive(dive.id)
+        : const <String>{};
+    final existingPaths = anyPaths
+        ? await _mediaRepository.getLinkedLocalPathsForDive(dive.id)
+        : const <String>{};
 
     // Filter out duplicates before processing
-    final newAssets = selectedAssets
-        .where((a) => !existingAssetIds.contains(a.id))
-        .toList();
+    final newAssets = selectedAssets.where((a) {
+      if (hasPath(a)) return !existingPaths.contains(a.filePath);
+      return !existingAssetIds.contains(a.id);
+    }).toList();
     final skippedCount = selectedAssets.length - newAssets.length;
 
     if (skippedCount > 0) {
@@ -174,12 +190,35 @@ class MediaImportService {
   MediaItem _createMediaItemFromAsset(AssetInfo asset, String diveId) {
     final now = DateTime.now();
 
+    // Windows / Linux have no platform photo library: the picker opens a file
+    // dialog and the asset's id is a synthetic key into an in-memory map on
+    // the picker service. Persisting such a row with the default
+    // platformGallery sourceType left every path column blank and sent
+    // display through PlatformGalleryResolver -> photo_manager, which has no
+    // desktop-Windows backend, so the photo rendered "File not found" and the
+    // pointer died with the process. When the picker hands us a real path,
+    // store a localFile row instead: LocalFileResolver reads localPath
+    // straight off disk on every desktop platform and survives a restart.
+    final path = asset.filePath;
+    final isLocalFile = path != null && path.isNotEmpty;
+
     return MediaItem(
       id: '',
       diveId: diveId,
-      platformAssetId: asset.id,
+      // Deliberately null for a localFile row. The desktop picker's asset id
+      // is a synthetic in-memory key, and several features gate purely on
+      // `platformAssetId != null` -- MediaItem.isGalleryPhoto,
+      // PhotoViewerPage's write-metadata action, resolvedFilePathProvider's
+      // gallery fast path -- so carrying it would route a Windows file
+      // through photo_manager, which has no backend there. Duplicate
+      // detection for these rows keys on localPath instead.
+      platformAssetId: isLocalFile ? null : asset.id,
       originalFilename: asset.filename,
       mediaType: asset.isVideo ? MediaType.video : MediaType.photo,
+      sourceType: isLocalFile
+          ? MediaSourceType.localFile
+          : MediaSourceType.platformGallery,
+      localPath: isLocalFile ? path : null,
       latitude: asset.latitude,
       longitude: asset.longitude,
       takenAt: asset.createDateTime,

lib/features/media/data/services/photo_picker_service.dart

@@ -49,6 +49,17 @@ class AssetInfo {
   /// Original filename if available.
   final String? filename;
 
+  /// Absolute filesystem path, when the asset came from a file dialog rather
+  /// than a platform photo library.
+  ///
+  /// Null on iOS / Android / macOS, where [id] is a real photo_manager asset
+  /// ID and the library owns the file. Non-null on Windows / Linux, where
+  /// there is no platform gallery and [id] is only a synthetic key into an
+  /// in-memory map that dies with the process. Importers must persist this
+  /// path so the row survives a restart -- see [AssetInfo] usage in
+  /// `MediaImportService`.
+  final String? filePath;
+
   const AssetInfo({
     required this.id,
     required this.type,
@@ -59,6 +70,7 @@ class AssetInfo {
     this.latitude,
     this.longitude,
     this.filename,
+    this.filePath,
   });
 
   /// Whether this asset is a video.

lib/features/media/data/services/photo_picker_service_desktop.dart

@@ -1,15 +1,27 @@
 import 'dart:io';
 import 'dart:typed_data';
 
+import 'package:flutter/foundation.dart' show compute;
+import 'package:image/image.dart' as img;
 import 'package:image_picker/image_picker.dart';
+import 'package:path/path.dart' as p;
 
+import 'package:submersion/features/media/data/services/capture_time_reader.dart';
 import 'package:submersion/features/media/data/services/photo_picker_service.dart';
 import 'package:submersion/features/media/domain/value_objects/media_source_metadata.dart';
 
 /// Photo picker implementation for Windows and Linux using image_picker.
 ///
 /// This is a fallback implementation that doesn't support date-filtered
 /// gallery browsing. Users must manually browse and select files.
+///
+/// Windows and Linux have no platform photo library, so every [AssetInfo]
+/// this service produces carries its [AssetInfo.filePath]. That path is the
+/// only durable pointer to the file: [AssetInfo.id] is a synthetic key into
+/// [_filePathCache], which lives and dies with the process. Importers must
+/// persist the path (as a `localFile` row) rather than the id, or the photo
+/// resolves through photo_manager -- which has no Windows backend -- and
+/// renders "File not found".
 class PhotoPickerServiceDesktop implements PhotoPickerService {
   final ImagePicker _picker = ImagePicker();
 
@@ -44,43 +56,44 @@ class PhotoPickerServiceDesktop implements PhotoPickerService {
       return [];
     }
 
-    final List<AssetInfo> results = [];
-
-    for (final file in files) {
-      final path = file.path;
-      final ioFile = File(path);
-
-      // Check if file exists
-      if (!await ioFile.exists()) continue;
-
-      // Get file metadata
-      final stat = await ioFile.stat();
-      final modified = stat.modified;
-
-      // Generate a unique ID for this file
-      final id = '${modified.millisecondsSinceEpoch}_${path.hashCode}';
-      _filePathCache[id] = path;
-
-      // Determine if it's a video based on extension
-      final extension = path.toLowerCase().split('.').last;
-      final isVideo = ['mp4', 'mov', 'avi', 'mkv', 'webm'].contains(extension);
-
-      results.add(
-        AssetInfo(
-          id: id,
-          type: isVideo ? AssetType.video : AssetType.image,
-          createDateTime: modified,
-          width: 0, // Not available without decoding
-          height: 0,
-          durationSeconds: null,
-          latitude: null,
-          longitude: null,
-          filename: path.split(Platform.pathSeparator).last,
-        ),
-      );
+    // Reading capture time and dimensions means reading each file's bytes,
+    // which would jank the UI thread for a pick of large photos (the old
+    // stat()-only implementation was cheap enough to inline). Do the batch on
+    // a background isolate, then register the paths here -- the isolate only
+    // ever mutates its own copy of [_filePathCache].
+    final assets = await compute(
+      _extractAssets,
+      files.map((f) => f.path).toList(),
+    );
+    for (final asset in assets) {
+      final path = asset.filePath;
+      if (path != null) _filePathCache[asset.id] = path;
     }
+    return assets;
+  }
 
-    return results;
+  /// Builds the [AssetInfo] for one file chosen from the desktop file dialog
+  /// and registers its path under the returned [AssetInfo.id].
+  ///
+  /// Returns null when [ioFile] does not exist.
+  ///
+  /// The capture time comes from the file's own container metadata (JPEG /
+  /// HEIC EXIF `DateTimeOriginal`, or the MP4/MOV `mvhd`) via
+  /// [readLocalCaptureTime], falling back to the mtime only when the file
+  /// carries no capture time at all. Reporting the mtime unconditionally --
+  /// as this service used to -- dates a photo to when it was copied off the
+  /// camera card rather than when it was shot, which pushes it outside the
+  /// dive window and leaves it unmatched.
+  ///
+  /// [readLocalCaptureTime] returns wall-clock-UTC, but [AssetInfo] is
+  /// contractually LOCAL (photo_manager's convention on mobile, which
+  /// consumers such as `TripMediaScanner.toWallClockUtc` reinterpret). The
+  /// components are therefore carried across verbatim into a local DateTime;
+  /// returning the UTC value directly would double-convert it.
+  AssetInfo? assetInfoForFile(File ioFile) {
+    final asset = _assetInfoForPath(ioFile.path);
+    if (asset != null) _filePathCache[asset.id] = asset.filePath!;
+    return asset;
   }
 
   @override
@@ -121,3 +134,103 @@ class PhotoPickerServiceDesktop implements PhotoPickerService {
   @override
   Future<MediaSourceMetadata?> getAssetMetadata(String assetId) async => null;
 }
+
+/// Batch entry point for [compute]: top-level so it can cross the isolate
+/// boundary (an instance method would close over `this`). Paths that no
+/// longer resolve are dropped.
+List<AssetInfo> _extractAssets(List<String> paths) {
+  final results = <AssetInfo>[];
+  for (final path in paths) {
+    final asset = _assetInfoForPath(path);
+    // Null means the picked path vanished between dialog and read.
+    if (asset != null) results.add(asset);
+  }
+  return results;
+}
+
+/// Pure metadata read for one path. Top-level and cache-free so it is safe to
+/// run on a [compute] isolate; [PhotoPickerServiceDesktop.assetInfoForFile]
+/// wraps it to register the path on the main isolate.
+AssetInfo? _assetInfoForPath(String path) {
+  final ioFile = File(path);
+  if (!ioFile.existsSync()) return null;
+
+  final modified = ioFile.lastModifiedSync();
+  final mime = _mimeFromExtension(p.extension(path).toLowerCase());
+
+  final capturedUtc = readLocalCaptureTime(ioFile, mime);
+  final createDateTime = capturedUtc == null
+      ? modified
+      : DateTime(
+          capturedUtc.year,
+          capturedUtc.month,
+          capturedUtc.day,
+          capturedUtc.hour,
+          capturedUtc.minute,
+          capturedUtc.second,
+          capturedUtc.millisecond,
+          // Today's readers are all second-granularity (EXIF
+          // DateTimeOriginal, mvhd creation_time), so this is defensive --
+          // but dropping sub-second components on a reinterpretation that
+          // exists only to swap the isUtc flag would be a silent lossy step.
+          capturedUtc.microsecond,
+        );
+
+  final size = _dimensionsOf(ioFile, mime);
+
+  return AssetInfo(
+    // Keyed on mtime + path so re-picking the same file in one session reuses
+    // the entry. Only meaningful in-process; the durable pointer is the path.
+    id: '${modified.millisecondsSinceEpoch}_${path.hashCode}',
+    type: mime.startsWith('video/') ? AssetType.video : AssetType.image,
+    createDateTime: createDateTime,
+    // AssetResolutionService's timestamp+dimensions tiers reject a 0x0 row
+    // outright, so real dimensions are what keep those tiers usable.
+    width: size?.width ?? 0,
+    height: size?.height ?? 0,
+    durationSeconds: null,
+    latitude: null,
+    longitude: null,
+    filename: p.basename(path),
+    filePath: path,
+  );
+}
+
+/// Reads pixel dimensions from an image's header.
+///
+/// Only the header is parsed -- `startDecode` stops before any pixel decode --
+/// but the decoder API takes bytes, so the file is read in full first. That
+/// read is why the batch runs on a [compute] isolate; on the main thread a
+/// pick of large images would visibly jank the UI.
+///
+/// Returns null for videos and for anything the decoder cannot read.
+({int width, int height})? _dimensionsOf(File file, String mime) {
+  if (!mime.startsWith('image/')) return null;
+  try {
+    final bytes = file.readAsBytesSync();
+    // Extension first, then content sniffing for files whose name lies.
+    final decoder =
+        img.findDecoderForNamedImage(file.path) ??
+        img.findDecoderForData(bytes);
+    final info = decoder?.startDecode(bytes);
+    if (info == null) return null;
+    return (width: info.width, height: info.height);
+  } on Object {
+    // Unsupported or truncated container: dimensions are optional here.
+    return null;
+  }
+}
+
+String _mimeFromExtension(String ext) => switch (ext) {
+  '.jpg' || '.jpeg' => 'image/jpeg',
+  '.png' => 'image/png',
+  '.heic' => 'image/heic',
+  '.heif' => 'image/heif',
+  '.webp' => 'image/webp',
+  '.gif' => 'image/gif',
+  '.mp4' => 'video/mp4',
+  '.mov' => 'video/quicktime',
+  '.m4v' => 'video/x-m4v',
+  '.avi' || '.mkv' || '.webm' => 'video/x-generic',
+  _ => 'application/octet-stream',
+};

lib/features/media/presentation/helpers/photo_import_helper.dart

@@ -67,6 +67,8 @@ class PhotoImportHelper {
       ),
       buffer: Duration.zero,
       alreadyLinkedIds: alreadyLinkedIds,
+      // Lets the Files tab link photos this dive's date window rejected.
+      diveId: dive.id,
     );
     // coverage:ignore-end