From 26658e615e58335ce8db880a15e7d47ad8fc1402 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 16:00:10 -0400 Subject: [PATCH 1/2] Find an image's picture by its picture id, not its item id A placed image may name the picture it shows (assetId, Hive field 11); without one, its own id names the picture, as before, so every image, .ica file and backup made so far reads exactly as it did. pictureId is the id the picture is stored, uploaded and found under: rendering, the page transition, screenshots, video export, the upload queue, cloud export, local-to-cloud migration and the cleanup of unused picture files all use it. Item identity (undo, transitions, sync rows, hero tags) stays the image's own id. Nothing makes such images yet; copies will (see the server half). Co-Authored-By: Claude Opus 5.5 --- lib/collab/cloud_media_models.dart | 2 +- lib/const/placed_classes.dart | 13 +++ lib/const/placed_classes.g.dart | 2 + lib/hive/hive_adapters.g.dart | 7 +- lib/hive/hive_adapters.g.yaml | 4 +- .../cloud_media_upload_queue_provider.dart | 25 +++-- lib/providers/image_provider.dart | 9 +- lib/providers/strategy_provider.dart | 2 +- lib/screenshot/page_screenshot.dart | 5 +- .../video_export/video_export_source.dart | 7 +- lib/strategy/strategy_cloud_migration.dart | 6 +- lib/strategy/strategy_import_export.dart | 6 +- lib/strategy/strategy_page_source.dart | 4 +- .../draggable_widgets/image/image_widget.dart | 12 ++- .../image/placed_image_builder.dart | 2 + lib/widgets/page_transition_overlay.dart | 1 + test/canonical_coordinates_test.dart | 1 + test/color_persistence_test.dart | 43 ++++++++ test/strategy_image_source_test.dart | 1 + test/strategy_integrity_test.dart | 97 +++++++++++++++++++ test/web_media_bytes_upload_test.dart | 43 ++++++++ 21 files changed, 265 insertions(+), 27 deletions(-) diff --git a/lib/collab/cloud_media_models.dart b/lib/collab/cloud_media_models.dart index 0e1713ab..6bb26a37 100644 --- a/lib/collab/cloud_media_models.dart +++ b/lib/collab/cloud_media_models.dart @@ -170,7 +170,7 @@ Set collectStrategyImageAssetIds(StrategyDataLike strategy) { final assetIds = {}; for (final page in strategy.pages) { for (final image in page.imageData) { - assetIds.add(image.id); + assetIds.add(image.pictureId); } for (final link in page.lineUpLinks) { for (final image in link.images) { diff --git a/lib/const/placed_classes.dart b/lib/const/placed_classes.dart index ee5b22d3..8b3db22b 100644 --- a/lib/const/placed_classes.dart +++ b/lib/const/placed_classes.dart @@ -218,10 +218,21 @@ class PlacedImage extends PlacedWidget { this.sizeVersion, this.tagColorValue, this.link = '', + this.assetId, }); final double aspectRatio; + /// The picture this image shows, when it isn't the image's own id: a + /// copy of an image is a new item showing its original's picture, with + /// nothing copied or uploaded. Absent on every image made before copies + /// shared pictures. Use [pictureId] to find the picture. + @JsonKey(includeIfNull: false) + final String? assetId; + + /// The id the image's picture is stored, uploaded and found under. + String get pictureId => assetId ?? id; + final String? fileExtension; double scale; @@ -264,6 +275,7 @@ class PlacedImage extends PlacedWidget { int? tagColorValue, bool? isDeleted, String? link, + String? assetId, }) { final cloned = PlacedImage( position: position ?? this.position, @@ -273,6 +285,7 @@ class PlacedImage extends PlacedWidget { fileExtension: fileExtension ?? this.fileExtension, sizeVersion: sizeVersion ?? this.sizeVersion, tagColorValue: tagColorValue ?? this.tagColorValue, + assetId: assetId ?? this.assetId, ); // Base class field // cloned.isDeleted = isDeleted ?? this.isDeleted; diff --git a/lib/const/placed_classes.g.dart b/lib/const/placed_classes.g.dart index 0c1e8765..e2aebc8d 100644 --- a/lib/const/placed_classes.g.dart +++ b/lib/const/placed_classes.g.dart @@ -54,6 +54,7 @@ PlacedImage _$PlacedImageFromJson(Map json) => PlacedImage( sizeVersion: (json['sizeVersion'] as num?)?.toInt(), tagColorValue: (json['tagColorValue'] as num?)?.toInt(), link: json['link'] as String? ?? '', + assetId: json['assetId'] as String?, )..isDeleted = json['isDeleted'] as bool? ?? false; Map _$PlacedImageToJson(PlacedImage instance) => @@ -62,6 +63,7 @@ Map _$PlacedImageToJson(PlacedImage instance) => 'isDeleted': instance.isDeleted, 'position': const OffsetConverter().toJson(instance.position), 'aspectRatio': instance.aspectRatio, + if (instance.assetId case final value?) 'assetId': value, 'fileExtension': instance.fileExtension, 'scale': instance.scale, 'sizeVersion': instance.sizeVersion, diff --git a/lib/hive/hive_adapters.g.dart b/lib/hive/hive_adapters.g.dart index e9baf321..2ffb967c 100644 --- a/lib/hive/hive_adapters.g.dart +++ b/lib/hive/hive_adapters.g.dart @@ -218,13 +218,14 @@ class PlacedImageAdapter extends TypeAdapter { sizeVersion: (fields[10] as num?)?.toInt(), tagColorValue: (fields[9] as num?)?.toInt(), link: fields[3] == null ? '' : fields[3] as String, + assetId: fields[11] as String?, )..isDeleted = fields[5] as bool; } @override void write(BinaryWriter writer, PlacedImage obj) { writer - ..writeByte(9) + ..writeByte(10) ..writeByte(1) ..write(obj.aspectRatio) ..writeByte(2) @@ -242,7 +243,9 @@ class PlacedImageAdapter extends TypeAdapter { ..writeByte(9) ..write(obj.tagColorValue) ..writeByte(10) - ..write(obj.sizeVersion); + ..write(obj.sizeVersion) + ..writeByte(11) + ..write(obj.assetId); } @override diff --git a/lib/hive/hive_adapters.g.yaml b/lib/hive/hive_adapters.g.yaml index 7fa3e40c..6a40ad03 100644 --- a/lib/hive/hive_adapters.g.yaml +++ b/lib/hive/hive_adapters.g.yaml @@ -75,7 +75,7 @@ types: index: 7 PlacedImage: typeId: 5 - nextIndex: 11 + nextIndex: 12 fields: aspectRatio: index: 1 @@ -95,6 +95,8 @@ types: index: 9 sizeVersion: index: 10 + assetId: + index: 11 MapValue: typeId: 6 nextIndex: 13 diff --git a/lib/providers/collab/cloud_media_upload_queue_provider.dart b/lib/providers/collab/cloud_media_upload_queue_provider.dart index 0a4f6e0a..98c1aeed 100644 --- a/lib/providers/collab/cloud_media_upload_queue_provider.dart +++ b/lib/providers/collab/cloud_media_upload_queue_provider.dart @@ -449,21 +449,24 @@ class CloudMediaUploadQueueNotifier assetPublicId: assetPublicId, ); + // By picture: a copy of an image shows its original's picture, which + // is uploaded once, for the original. for (final image in placedImages) { - final asset = assetsById[image.id]; + final pictureId = image.pictureId; + final asset = assetsById[pictureId]; final hasActiveRemote = asset?.uploadStatus == 'active' && (asset?.url?.isNotEmpty ?? false); - if (hasActiveRemote || _getJob(image.id) != null) { + if (hasActiveRemote || _getJob(pictureId) != null) { continue; } final bytes = await _findMediaBytes( - keyFor(image.id), + keyFor(pictureId), fileExtension: image.fileExtension ?? '', ); if (bytes == null) { _logMedia( - 'reconcile.local_missing image=${image.id} ' + 'reconcile.local_missing image=$pictureId ' 'strategy=$strategyPublicId status=${asset?.uploadStatus ?? 'none'}', ); continue; @@ -471,7 +474,7 @@ class CloudMediaUploadQueueNotifier await enqueueJobForLocalBytes( strategyPublicId: strategyPublicId, - assetPublicId: image.id, + assetPublicId: pictureId, fileExtension: image.fileExtension ?? '', ); } @@ -1192,10 +1195,10 @@ class CloudMediaUploadQueueNotifier bool _opReferencesAsset(StrategyOp op, String assetPublicId) { if (op is ElementAddOp) { - return op.elementPublicId == assetPublicId; + return _pictureOf(op.elementPublicId, op.payload) == assetPublicId; } if (op is ElementPatchOp) { - return op.elementPublicId == assetPublicId; + return _pictureOf(op.elementPublicId, op.payload) == assetPublicId; } if (op is LineupAddOp) { return _jsonContainsAssetId(op.payload, assetPublicId); @@ -1206,6 +1209,14 @@ class CloudMediaUploadQueueNotifier return false; } + /// The picture an element op shows when it is an image: its payload's + /// `assetId`, else the element's own id (see PlacedImage.pictureId). + static String _pictureOf(String elementPublicId, CloudPayload? payload) { + final assetId = + payload == null ? null : cloudPayloadData(payload)['assetId']; + return assetId is String && assetId.isNotEmpty ? assetId : elementPublicId; + } + bool _jsonContainsAssetId(Object? value, String assetPublicId) { if (value is Map) { if (value['id'] == assetPublicId) return true; diff --git a/lib/providers/image_provider.dart b/lib/providers/image_provider.dart index 1048e924..5017bcf4 100644 --- a/lib/providers/image_provider.dart +++ b/lib/providers/image_provider.dart @@ -679,7 +679,7 @@ class PlacedImageSerializer { /// /// It uses the application support directory, creates a custom folder based /// on [strategyID] and an `images` subfolder, and forms the filename from the - /// image's [id] and [fileExtension]. + /// image's picture id ([PlacedImage.pictureId]) and [fileExtension]. static Future _computeFilePath( PlacedImage image, String strategyID) async { // Get the system's application support directory. @@ -699,8 +699,11 @@ class PlacedImageSerializer { await imagesDirectory.create(recursive: true); } - // The final file path: [id][fileExtension] - return path.join(imagesDirectory.path, '${image.id}${image.fileExtension}'); + // The final file path: [pictureId][fileExtension] + return path.join( + imagesDirectory.path, + '${image.pictureId}${image.fileExtension}', + ); } static String? detectImageFormat(Uint8List bytes) { diff --git a/lib/providers/strategy_provider.dart b/lib/providers/strategy_provider.dart index 0fa31be6..f634ba8c 100644 --- a/lib/providers/strategy_provider.dart +++ b/lib/providers/strategy_provider.dart @@ -1127,7 +1127,7 @@ class StrategyProvider extends Notifier { if (!kIsWeb) { List allImageIds = []; for (final page in newStrat.pages) { - allImageIds.addAll(page.imageData.map((image) => image.id)); + allImageIds.addAll(page.imageData.map((image) => image.pictureId)); for (final link in page.lineUpLinks) { allImageIds.addAll(link.images.map((image) => image.id)); } diff --git a/lib/screenshot/page_screenshot.dart b/lib/screenshot/page_screenshot.dart index 620182f5..0242d2ba 100644 --- a/lib/screenshot/page_screenshot.dart +++ b/lib/screenshot/page_screenshot.dart @@ -47,10 +47,11 @@ Future captureEditorPage(WidgetRef ref) async { final images = await resolveCaptureImages( { + // By picture, as the captured page's images look them up. for (final image in page.imageData) - image.id: readStrategyImageSource( + image.pictureId: readStrategyImageSource( ref, - (id: image.id, fileExtension: image.fileExtension), + (id: image.pictureId, fileExtension: image.fileExtension), ), }, fetch: (imageId, url, client) => downloadCloudImageBytes( diff --git a/lib/services/video_export/video_export_source.dart b/lib/services/video_export/video_export_source.dart index 5b073d0e..203af513 100644 --- a/lib/services/video_export/video_export_source.dart +++ b/lib/services/video_export/video_export_source.dart @@ -149,18 +149,19 @@ Future loadVideoExportSource( final images = await resolveCaptureImages( { for (final page in pages) + // By picture, as the captured pages' images look them up. for (final image in page.imageData) - image.id: resolveStrategyImageSource( + image.pictureId: resolveStrategyImageSource( localFilePath: findLocalImageFile( storageDirectory: state.storageDirectory, - imageId: image.id, + imageId: image.pictureId, fileExtension: image.fileExtension, ), isCloudStrategy: isCloud, // The whole strategy was just read, and this device has // nothing left to upload. assetsLoaded: true, - remoteAsset: assets[image.id], + remoteAsset: assets[image.pictureId], uploadMayBeQueuedHere: false, ), }, diff --git a/lib/strategy/strategy_cloud_migration.dart b/lib/strategy/strategy_cloud_migration.dart index e225702e..63167b1f 100644 --- a/lib/strategy/strategy_cloud_migration.dart +++ b/lib/strategy/strategy_cloud_migration.dart @@ -57,7 +57,11 @@ void appendMigratedPageOps( for (final image in page.imageData) { final elementId = nextUniqueMigrationId(image.id, usedElementIds); - final payload = cloudImagePayloadFromPlacedImage(image) + // A renamed image keeps showing its picture, which is stored under its + // old id. + final payload = cloudImagePayloadFromPlacedImage( + elementId == image.id ? image : image.copyWith(assetId: image.pictureId), + ) ..putIfAbsent('elementType', () => 'image') ..['id'] = elementId; ops.add( diff --git a/lib/strategy/strategy_import_export.dart b/lib/strategy/strategy_import_export.dart index f483ac64..b66a9acc 100644 --- a/lib/strategy/strategy_import_export.dart +++ b/lib/strategy/strategy_import_export.dart @@ -2408,7 +2408,11 @@ class StrategyImportExportService { if (element.deleted || element.elementType != 'image') { continue; } - assetIds.add(element.publicId); + // The picture it shows (PlacedImage.pictureId). + final assetId = cloudPayloadData(element.payload)['assetId']; + assetIds.add( + assetId is String && assetId.isNotEmpty ? assetId : element.publicId, + ); } final lineups = lineUpGraphFromRemoteLineups( diff --git a/lib/strategy/strategy_page_source.dart b/lib/strategy/strategy_page_source.dart index 1d0457e8..3db8653f 100644 --- a/lib/strategy/strategy_page_source.dart +++ b/lib/strategy/strategy_page_source.dart @@ -261,7 +261,7 @@ class CloudStrategyPageSource implements StrategyPageSource { break; case 'image': final hydrated = PlacedImage.fromJson(payload); - final remoteAsset = snapshot.assetsById[hydrated.id]; + final remoteAsset = snapshot.assetsById[hydrated.pictureId]; images.add(hydrated); if (remoteAsset != null) { ref.read(cloudMediaCacheProvider.notifier).ensureAssetCached( @@ -440,7 +440,7 @@ class CloudStrategyPageSource implements StrategyPageSource { break; case 'image': final hydrated = PlacedImage.fromJson(payload); - final remoteAsset = snapshot.assetsById[hydrated.id]; + final remoteAsset = snapshot.assetsById[hydrated.pictureId]; images.add(hydrated); if (remoteAsset != null) { ref.read(cloudMediaCacheProvider.notifier).ensureAssetCached( diff --git a/lib/widgets/draggable_widgets/image/image_widget.dart b/lib/widgets/draggable_widgets/image/image_widget.dart index f32a87ce..329cc58a 100644 --- a/lib/widgets/draggable_widgets/image/image_widget.dart +++ b/lib/widgets/draggable_widgets/image/image_widget.dart @@ -45,7 +45,6 @@ class _ImageFullScreenOverlay extends StatelessWidget { @override Widget build(BuildContext context) { - return CallbackShortcuts( bindings: { const SingleActivator(LogicalKeyboardKey.escape): () { @@ -129,13 +128,20 @@ class ImageWidget extends ConsumerStatefulWidget { required this.scale, required this.fileExtension, required this.id, + required this.pictureId, this.tagColorValue, this.isFeedback = false, }); final double aspectRatio; final double scale; final String? fileExtension; + + /// The placed image's id, which its hero tag carries: unique on a page. final String id; + + /// The id of the picture it shows (PlacedImage.pictureId), which images + /// on several pages, or a copy and its original, can share. + final String pictureId; final int? tagColorValue; final bool isFeedback; @@ -161,7 +167,7 @@ class _ImageWidgetState extends ConsumerState { .clamp(1.0, double.infinity); final source = watchStrategyImageSource( ref, - (id: widget.id, fileExtension: widget.fileExtension), + (id: widget.pictureId, fileExtension: widget.fileExtension), ); final image = source.imageProvider; @@ -170,7 +176,7 @@ class _ImageWidgetState extends ConsumerState { // while its cloud URL loads, and no other image's frame carries // over. LocalImageFile() || RemoteImageUrl() || ImageBytes() => Image( - key: ValueKey(widget.id), + key: ValueKey(widget.pictureId), image: image!, fit: BoxFit.contain, gaplessPlayback: true, diff --git a/lib/widgets/draggable_widgets/image/placed_image_builder.dart b/lib/widgets/draggable_widgets/image/placed_image_builder.dart index eba8db3f..ba7e58e2 100644 --- a/lib/widgets/draggable_widgets/image/placed_image_builder.dart +++ b/lib/widgets/draggable_widgets/image/placed_image_builder.dart @@ -189,6 +189,7 @@ class _PlacedImageBuilderState extends State { scale: localScale!, fileExtension: widget.placedImage.fileExtension, id: widget.placedImage.id, + pictureId: widget.placedImage.pictureId, tagColorValue: widget.placedImage.tagColorValue, ), ), @@ -220,6 +221,7 @@ class _PlacedImageBuilderState extends State { aspectRatio: widget.placedImage.aspectRatio, scale: localScale!, id: widget.placedImage.id, + pictureId: widget.placedImage.pictureId, tagColorValue: widget.placedImage.tagColorValue, ), ), diff --git a/lib/widgets/page_transition_overlay.dart b/lib/widgets/page_transition_overlay.dart index e1730767..c3fa48ff 100644 --- a/lib/widgets/page_transition_overlay.dart +++ b/lib/widgets/page_transition_overlay.dart @@ -774,6 +774,7 @@ class PlacedWidgetPreview { aspectRatio: w.aspectRatio, scale: scale ?? w.scale, id: w.id, + pictureId: w.pictureId, tagColorValue: w.tagColorValue, ); } diff --git a/test/canonical_coordinates_test.dart b/test/canonical_coordinates_test.dart index d49c2fe9..aaec9148 100644 --- a/test/canonical_coordinates_test.dart +++ b/test/canonical_coordinates_test.dart @@ -388,6 +388,7 @@ void main() { ImageWidget( key: const ValueKey('image-card'), id: image.id, + pictureId: image.pictureId, aspectRatio: image.aspectRatio, scale: image.scale, fileExtension: image.fileExtension, diff --git a/test/color_persistence_test.dart b/test/color_persistence_test.dart index e4329b4f..8929e98c 100644 --- a/test/color_persistence_test.dart +++ b/test/color_persistence_test.dart @@ -366,5 +366,48 @@ void main() { expect(restored.link, isEmpty); }); + + test('a placed image stored before picture ids shows its own picture', () { + final restored = PlacedImageAdapter().read( + _legacyFieldReader({ + 1: 1.5, + 2: 200.0, + 3: '', + 4: 'legacy-image', + 5: false, + 6: const Offset(3, 4), + 8: '.png', + 9: 0xFF3B82F6, + 10: worldSizedMediaVersion, + }), + ); + + expect(restored.assetId, isNull); + expect(restored.pictureId, 'legacy-image'); + // Its JSON, and so its cloud payload, is as it was. + expect(restored.toJson().containsKey('assetId'), isFalse); + }); + + test("a placed image showing another image's picture keeps it", () { + final restored = PlacedImageAdapter().read( + _legacyFieldReader({ + 1: 1.5, + 2: 200.0, + 3: '', + 4: 'copy-image', + 5: false, + 6: const Offset(3, 4), + 8: '.png', + 10: worldSizedMediaVersion, + 11: 'original-image', + }), + ); + + expect(restored.pictureId, 'original-image'); + final reloaded = PlacedImage.fromJson(restored.toJson()); + expect(reloaded.id, 'copy-image'); + expect(reloaded.pictureId, 'original-image'); + expect(restored.copyWith(scale: 300).pictureId, 'original-image'); + }); }); } diff --git a/test/strategy_image_source_test.dart b/test/strategy_image_source_test.dart index 55373476..8d0dfc25 100644 --- a/test/strategy_image_source_test.dart +++ b/test/strategy_image_source_test.dart @@ -202,6 +202,7 @@ Widget _imageApp({ // ignore: prefer_const_constructors ImageWidget( id: _imageId, + pictureId: _imageId, aspectRatio: 16 / 9, scale: 320, fileExtension: '.png', diff --git a/test/strategy_integrity_test.dart b/test/strategy_integrity_test.dart index 7fbe6b7d..2b836236 100644 --- a/test/strategy_integrity_test.dart +++ b/test/strategy_integrity_test.dart @@ -16,6 +16,7 @@ import 'package:icarus/const/hive_boxes.dart'; import 'package:icarus/const/drawing_element.dart'; import 'package:icarus/const/line_provider.dart'; import 'package:icarus/const/maps.dart'; +import 'package:icarus/const/coordinate_system.dart'; import 'package:icarus/const/placed_classes.dart'; import 'package:icarus/const/settings.dart'; import 'package:icarus/const/utilities.dart'; @@ -772,6 +773,102 @@ void main() { expect(reExported, equals(exported)); }); + test( + 'an image showing another image\'s picture round-trips, and opening ' + 'keeps the picture while only the copy shows it', () async { + final harness = await _IcaHarness.open(); + addTearDown(harness.close); + Map image(String id, {String? assetId}) => { + 'id': id, + 'isDeleted': false, + 'position': {'dx': 500.0, 'dy': 600.0}, + 'aspectRatio': 1.0, + 'fileExtension': '.png', + 'scale': 220.0, + 'tagColorValue': null, + if (assetId != null) 'assetId': assetId, + }; + final payload = { + 'versionNumber': '${Settings.versionNumber}', + 'mapData': 'ascent', + 'pages': [ + { + 'id': 'page-1', + 'sortIndex': '0', + 'name': 'Page 1', + 'isAutoNamed': true, + 'drawingData': [], + 'agentData': [], + 'abilityData': [], + 'textData': [], + 'imageData': [image('img-1'), image('img-copy', assetId: 'img-1')], + 'utilityData': [], + 'isAttack': 'true', + 'settings': {'agentSize': 35.0, 'abilitySize': 25.0}, + 'lineUpData': [], + }, + ], + }; + final picture = [137, 80, 78, 71, 13, 10, 26, 10]; + final json = utf8.encode(jsonEncode(payload)); + final archive = Archive() + ..addFile(ArchiveFile('Pictures.json', json.length, json)) + ..addFile(ArchiveFile('img-1.png', picture.length, picture)); + final file = File(path.join(harness.directory.path, 'Pictures.ica')); + await file.writeAsBytes(ZipEncoder().encodeBytes(archive)); + + final imported = await harness.importIca(file); + final images = { + for (final image in imported.pages.single.imageData) image.id: image, + }; + expect(images['img-1']!.pictureId, 'img-1'); + expect(images['img-copy']!.assetId, 'img-1'); + expect(images['img-copy']!.pictureId, 'img-1'); + + // Exported: the copy names the picture, which is packed once. + final exported = await harness.exportIca(imported); + final exportedImages = ((await _readIcaJson(exported))['pages'] as List) + .cast>() + .single['imageData'] as List; + expect( + {for (final image in exportedImages) image['id']: image['assetId']}, + {'img-1': null, 'img-copy': 'img-1'}, + ); + expect((await _icaAttachments(exported)).keys, ['img-1.png']); + final reImported = await harness.importIca(exported); + expect( + reImported.pages.single.imageData + .singleWhere((image) => image.id == 'img-copy') + .pictureId, + 'img-1', + ); + + // The original is deleted; opening the strategy keeps the picture the + // copy still shows. + final onlyCopy = reImported.copyWith(pages: [ + reImported.pages.single.copyWith( + imageData: [ + reImported.pages.single.imageData + .singleWhere((image) => image.id == 'img-copy'), + ], + ), + ]); + await harness.strategies.put(onlyCopy.id, onlyCopy); + CoordinateSystem(playAreaSize: const Size(1920, 1080)); + await harness.container + .read(strategyProvider.notifier) + .loadFromHive(onlyCopy.id); + expect( + File(path.join( + harness.directory.path, + onlyCopy.id, + 'images', + 'img-1.png', + )).existsSync(), + isTrue, + ); + }); + test('custom shape utility dimensions support undo and redo', () { final rectangle = PlacedUtility( id: 'rectangle-undo', diff --git a/test/web_media_bytes_upload_test.dart b/test/web_media_bytes_upload_test.dart index 978ef07e..a47314a3 100644 --- a/test/web_media_bytes_upload_test.dart +++ b/test/web_media_bytes_upload_test.dart @@ -549,6 +549,49 @@ void main() { ); }); + test("a copy showing another image's picture uploads it once, under it", + () async { + final bytesStore = MemoryPendingMediaBytesStore(); + final mediaStore = MemoryDurableCloudMediaOutboxStore(); + final container = + _webSession(mediaStore: mediaStore, bytesStore: bytesStore); + addTearDown(container.dispose); + final queue = container.read(cloudMediaUploadQueueProvider.notifier); + PlacedImage image(String id, {String? assetId}) => PlacedImage( + id: id, + position: Offset.zero, + aspectRatio: 1, + scale: 1, + fileExtension: '.png', + assetId: assetId, + ); + await container + .read(pendingMediaBytesProvider.notifier) + .put(_key('original'), _imageBytes); + + // Only the copy is on the page: its picture is the original's. + await queue.reconcilePageMedia( + strategyPublicId: 'strategy-a', + placedImages: [image('copy', assetId: 'original')], + assetsById: const {}, + ); + expect( + container + .read(cloudMediaUploadQueueProvider) + .jobs + .map((job) => job.assetPublicId), + ['original'], + ); + + // With the original beside it, still the one upload. + await queue.reconcilePageMedia( + strategyPublicId: 'strategy-a', + placedImages: [image('original'), image('copy', assetId: 'original')], + assetsById: const {}, + ); + expect(container.read(cloudMediaUploadQueueProvider).jobs, hasLength(1)); + }); + group('the painted copy goes once attached and served from the cloud', () { Future race({required bool urlFirst}) async { final gate = Completer(); From 9f4b327e7a09e03c7e0ea5cee4bf7a44d8fded68 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 16:40:11 -0400 Subject: [PATCH 2/2] Ask the server for images' picture ids This client keeps PlacedImage.assetId, so it reads snapshots and op results with it (acceptsPictureIds); older clients get them without. Co-Authored-By: Claude Opus 5.5 --- lib/collab/convex_strategy_repository.dart | 6 +++ test/collab/referenced_asset_ids_test.dart | 47 ++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/lib/collab/convex_strategy_repository.dart b/lib/collab/convex_strategy_repository.dart index 89c6330b..db86f107 100644 --- a/lib/collab/convex_strategy_repository.dart +++ b/lib/collab/convex_strategy_repository.dart @@ -163,6 +163,7 @@ class ConvexStrategyRepository { strategyPublicId: strategyPublicId, pagePublicId: pagePublicId, shareToken: _optional(shareToken), + acceptsPictureIds: const ConvexOptional.present(true), ) .fetch(), ); @@ -179,6 +180,8 @@ class ConvexStrategyRepository { strategyPublicId: strategyPublicId, pagePublicId: pagePublicId, shareToken: _optional(shareToken), + // This client keeps an image's picture id (PlacedImage.assetId). + acceptsPictureIds: const ConvexOptional.present(true), ) .watch() .map(_pageSnapshot); @@ -226,6 +229,7 @@ class ConvexStrategyRepository { // This client checks image references apart, so it can take a // snapshot without the pages in the server's trash. acceptsTrashedPagesLeftOut: const ConvexOptional.present(true), + acceptsPictureIds: const ConvexOptional.present(true), ) .fetch(), )); @@ -351,6 +355,8 @@ class ConvexStrategyRepository { accountSubject: _optional(accountSubject), // This client restores deleted pages, so it sends such a delete again. checkTrashedPageDeletes: const ConvexOptional.present(true), + // This client keeps an image's picture id (PlacedImage.assetId). + acceptsPictureIds: const ConvexOptional.present(true), ); return result.results.map(_opAck).toList(growable: false); } diff --git a/test/collab/referenced_asset_ids_test.dart b/test/collab/referenced_asset_ids_test.dart index c9ebafd5..1324d8f7 100644 --- a/test/collab/referenced_asset_ids_test.dart +++ b/test/collab/referenced_asset_ids_test.dart @@ -1,4 +1,5 @@ import 'package:flutter_test/flutter_test.dart'; +import 'package:icarus/collab/collab_models.dart'; import 'package:icarus/collab/convex_strategy_repository.dart'; import 'package:icarus/collab/generated/generated.dart'; import 'package:icarus/collab/transport/convex_transport.dart'; @@ -36,6 +37,46 @@ void main() { isTrue); }); + test("reads and writes as a client that keeps images' picture ids", () async { + final transport = _RecordingTransport(); + final repository = ConvexStrategyRepository(IcarusConvexApi(transport)); + + await expectLater( + repository.fetchFullSnapshot('strategy-a'), throwsStateError); + await expectLater( + repository.fetchPageSnapshot( + strategyPublicId: 'strategy-a', + pagePublicId: 'page-1', + ), + throwsStateError, + ); + await expectLater( + repository.applyBatch( + strategyPublicId: 'strategy-a', + clientId: 'client-a', + ops: const [ + ElementDeleteOp( + opId: 'op-1', + pagePublicId: 'page-1', + elementPublicId: 'image-1', + expectedElementRevision: 1, + ), + ], + ), + throwsStateError, + ); + + expect(transport.calls.map((call) => call.$1), [ + 'strategy:getFullSnapshot', + 'page:getSnapshot', + 'ops:applyBatch', + ]); + for (final (name, args) in transport.calls) { + expect((args.value['acceptsPictureIds'] as ConvexBoolean?)?.value, isTrue, + reason: name); + } + }); + test('cannot tell if any batch cannot', () async { final transport = _ReferencesTransport({'image-3'}, nullBatch: 1); final repository = ConvexStrategyRepository(IcarusConvexApi(transport)); @@ -83,6 +124,12 @@ final class _RecordingTransport implements ConvexTransport { throw StateError('not answered in this test'); } + @override + Future mutation(String name, ConvexObject args) async { + calls.add((name, args)); + throw StateError('not answered in this test'); + } + @override dynamic noSuchMethod(Invocation invocation) => throw UnimplementedError(); }