mirror of
https://github.com/immich-app/immich.git
synced 2026-05-18 03:10:24 +03:00
fix(mobile): images loads sometimes cancel too early (#27067)
* refactor listener tracking for image stream completers and fix early cancel call * fix: improve cache listener identification in image stream tracking * add documentation and test cases for listener tracking in ImageStreamCompleter * fix: remove unnecessary image provision flag from listener tracking * fix: override setImage method in cache aware listener tracker mixin * fix: rename test file
This commit is contained in:
@@ -3,24 +3,21 @@ import 'dart:ui' as ui;
|
|||||||
|
|
||||||
import 'package:flutter/foundation.dart' show InformationCollector;
|
import 'package:flutter/foundation.dart' show InformationCollector;
|
||||||
import 'package:flutter/painting.dart';
|
import 'package:flutter/painting.dart';
|
||||||
|
import 'package:immich_mobile/presentation/widgets/images/cache_aware_listener_tracker.mixin.dart';
|
||||||
|
|
||||||
/// A [MultiFrameImageStreamCompleter] with support for listener tracking
|
/// A [MultiFrameImageStreamCompleter] with support for listener tracking
|
||||||
/// which makes resource cleanup possible when no longer needed.
|
/// which makes resource cleanup possible when no longer needed.
|
||||||
/// Codec is disposed through the MultiFrameImageStreamCompleter's internals onDispose method
|
/// Codec is disposed through the MultiFrameImageStreamCompleter's internals onDispose method
|
||||||
class AnimatedImageStreamCompleter extends MultiFrameImageStreamCompleter {
|
class AnimatedImageStreamCompleter extends MultiFrameImageStreamCompleter with CacheAwareListenerTrackerMixin {
|
||||||
void Function()? _onLastListenerRemoved;
|
|
||||||
int _listenerCount = 0;
|
|
||||||
// True once any image or the codec has been provided.
|
|
||||||
// Until then the image cache holds one listener, so "last real listener gone"
|
|
||||||
// is _listenerCount == 1, not 0.
|
|
||||||
bool didProvideImage = false;
|
|
||||||
|
|
||||||
AnimatedImageStreamCompleter._({
|
AnimatedImageStreamCompleter._({
|
||||||
required super.codec,
|
required super.codec,
|
||||||
required super.scale,
|
required super.scale,
|
||||||
|
required bool hadInitialImage,
|
||||||
super.informationCollector,
|
super.informationCollector,
|
||||||
void Function()? onLastListenerRemoved,
|
void Function()? onLastListenerRemoved,
|
||||||
}) : _onLastListenerRemoved = onLastListenerRemoved;
|
}) {
|
||||||
|
setupListenerTracking(hadInitialImage: hadInitialImage, onLastListenerRemoved: onLastListenerRemoved);
|
||||||
|
}
|
||||||
|
|
||||||
factory AnimatedImageStreamCompleter({
|
factory AnimatedImageStreamCompleter({
|
||||||
required Stream<Object> stream,
|
required Stream<Object> stream,
|
||||||
@@ -33,23 +30,21 @@ class AnimatedImageStreamCompleter extends MultiFrameImageStreamCompleter {
|
|||||||
final self = AnimatedImageStreamCompleter._(
|
final self = AnimatedImageStreamCompleter._(
|
||||||
codec: codecCompleter.future,
|
codec: codecCompleter.future,
|
||||||
scale: scale,
|
scale: scale,
|
||||||
|
hadInitialImage: initialImage != null,
|
||||||
informationCollector: informationCollector,
|
informationCollector: informationCollector,
|
||||||
onLastListenerRemoved: onLastListenerRemoved,
|
onLastListenerRemoved: onLastListenerRemoved,
|
||||||
);
|
);
|
||||||
|
|
||||||
if (initialImage != null) {
|
if (initialImage != null) {
|
||||||
self.didProvideImage = true;
|
|
||||||
self.setImage(initialImage);
|
self.setImage(initialImage);
|
||||||
}
|
}
|
||||||
|
|
||||||
stream.listen(
|
stream.listen(
|
||||||
(item) {
|
(item) {
|
||||||
if (item is ImageInfo) {
|
if (item is ImageInfo) {
|
||||||
self.didProvideImage = true;
|
|
||||||
self.setImage(item);
|
self.setImage(item);
|
||||||
} else if (item is ui.Codec) {
|
} else if (item is ui.Codec) {
|
||||||
if (!codecCompleter.isCompleted) {
|
if (!codecCompleter.isCompleted) {
|
||||||
self.didProvideImage = true;
|
|
||||||
codecCompleter.complete(item);
|
codecCompleter.complete(item);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -70,27 +65,4 @@ class AnimatedImageStreamCompleter extends MultiFrameImageStreamCompleter {
|
|||||||
|
|
||||||
return self;
|
return self;
|
||||||
}
|
}
|
||||||
|
|
||||||
@override
|
|
||||||
void addListener(ImageStreamListener listener) {
|
|
||||||
super.addListener(listener);
|
|
||||||
_listenerCount++;
|
|
||||||
}
|
|
||||||
|
|
||||||
@override
|
|
||||||
void removeListener(ImageStreamListener listener) {
|
|
||||||
super.removeListener(listener);
|
|
||||||
_listenerCount--;
|
|
||||||
|
|
||||||
final bool onlyCacheListenerLeft = _listenerCount == 1 && !didProvideImage;
|
|
||||||
final bool noListenersAfterCodec = _listenerCount == 0 && didProvideImage;
|
|
||||||
|
|
||||||
if (onlyCacheListenerLeft || noListenersAfterCodec) {
|
|
||||||
final onLastListenerRemoved = _onLastListenerRemoved;
|
|
||||||
if (onLastListenerRemoved != null) {
|
|
||||||
_onLastListenerRemoved = null;
|
|
||||||
onLastListenerRemoved();
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -0,0 +1,84 @@
|
|||||||
|
import 'package:flutter/painting.dart';
|
||||||
|
|
||||||
|
/// Tracks listeners on an [ImageStreamCompleter] to safely cancel in-flight
|
||||||
|
/// network requests without interfering with [ImageCache] internals.
|
||||||
|
///
|
||||||
|
/// ### Problem
|
||||||
|
/// Cancelling fetches when the listener count drops to 1 (cache only) or 0
|
||||||
|
/// is unsafe due to three framework behaviours:
|
||||||
|
///
|
||||||
|
/// 1. **Memory-pressure eviction** — `ImageCache.clear()` removes the cache
|
||||||
|
/// listener while UI widgets still need the image. A count-based check
|
||||||
|
/// would cancel the active fetch, leaving the UI with no image.
|
||||||
|
/// 2. **Synchronous detach during `putIfAbsent`** — When an `initialImage`
|
||||||
|
/// is provided, the cache attaches, receives the frame, and detaches
|
||||||
|
/// synchronously *before* the UI widget can attach. Count reaches 0 and
|
||||||
|
/// would trigger a false cancel.
|
||||||
|
/// 3. **Listener misidentification** — After the cache detaches (via 1 or 2),
|
||||||
|
/// the next UI listener could be mistaken for the cache listener, causing
|
||||||
|
/// incorrect cancellations when that widget is disposed.
|
||||||
|
///
|
||||||
|
/// ### Solution: First-Listener Heuristic
|
||||||
|
/// The cache is always the first listener attached (via `putIfAbsent`). This
|
||||||
|
/// mixin records that identity once and uses it for all subsequent decisions:
|
||||||
|
///
|
||||||
|
/// * **Identity locking** — The first listener is assumed to be the cache.
|
||||||
|
/// Once identified, `_hasIdentifiedCacheListener` prevents reassignment.
|
||||||
|
/// * **Targeted cancellation** — Cancel only when the identified cache
|
||||||
|
/// listener is the sole remaining listener and no image has been delivered.
|
||||||
|
/// * **Sync-removal bypass** — When `hadInitialImage` is set, the first
|
||||||
|
/// synchronous removal of the cache listener is ignored so the fetch
|
||||||
|
/// survives until the UI attaches.
|
||||||
|
mixin CacheAwareListenerTrackerMixin on ImageStreamCompleter {
|
||||||
|
void Function()? _onLastListenerRemoved;
|
||||||
|
int _listenerCount = 0;
|
||||||
|
bool _hadInitialImage = false;
|
||||||
|
bool _hasIgnoredFirstSyncRemoval = false;
|
||||||
|
ImageStreamListener? _cacheListener;
|
||||||
|
bool _hasIdentifiedCacheListener = false;
|
||||||
|
|
||||||
|
/// Initializes the tracking state. Must be called in the subclass constructor.
|
||||||
|
void setupListenerTracking({required bool hadInitialImage, void Function()? onLastListenerRemoved}) {
|
||||||
|
_hadInitialImage = hadInitialImage;
|
||||||
|
_onLastListenerRemoved = onLastListenerRemoved;
|
||||||
|
}
|
||||||
|
|
||||||
|
@override
|
||||||
|
void addListener(ImageStreamListener listener) {
|
||||||
|
if (!_hasIdentifiedCacheListener) {
|
||||||
|
_hasIdentifiedCacheListener = true;
|
||||||
|
_cacheListener = listener;
|
||||||
|
}
|
||||||
|
|
||||||
|
_listenerCount++;
|
||||||
|
super.addListener(listener);
|
||||||
|
}
|
||||||
|
|
||||||
|
@override
|
||||||
|
void removeListener(ImageStreamListener listener) {
|
||||||
|
super.removeListener(listener);
|
||||||
|
_listenerCount--;
|
||||||
|
|
||||||
|
final bool isCacheListener = listener == _cacheListener;
|
||||||
|
if (isCacheListener) {
|
||||||
|
_cacheListener = null;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (_hadInitialImage && !_hasIgnoredFirstSyncRemoval && isCacheListener) {
|
||||||
|
_hasIgnoredFirstSyncRemoval = true;
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
final bool onlyCacheListenerLeft = _listenerCount == 1 && _cacheListener != null;
|
||||||
|
|
||||||
|
final bool completelyAbandoned = _listenerCount == 0;
|
||||||
|
|
||||||
|
if (onlyCacheListenerLeft || completelyAbandoned) {
|
||||||
|
final onLastListenerRemoved = _onLastListenerRemoved;
|
||||||
|
if (onLastListenerRemoved != null) {
|
||||||
|
_onLastListenerRemoved = null;
|
||||||
|
onLastListenerRemoved();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -6,14 +6,10 @@ import 'dart:async';
|
|||||||
|
|
||||||
import 'package:flutter/foundation.dart';
|
import 'package:flutter/foundation.dart';
|
||||||
import 'package:flutter/painting.dart';
|
import 'package:flutter/painting.dart';
|
||||||
|
import 'package:immich_mobile/presentation/widgets/images/cache_aware_listener_tracker.mixin.dart';
|
||||||
|
|
||||||
/// An ImageStreamCompleter with support for loading multiple images.
|
/// An ImageStreamCompleter with support for loading multiple images.
|
||||||
class OneFramePlaceholderImageStreamCompleter extends ImageStreamCompleter {
|
class OneFramePlaceholderImageStreamCompleter extends ImageStreamCompleter with CacheAwareListenerTrackerMixin {
|
||||||
void Function()? _onLastListenerRemoved;
|
|
||||||
int _listenerCount = 0;
|
|
||||||
// True once setImage() has been called at least once.
|
|
||||||
bool didProvideImage = false;
|
|
||||||
|
|
||||||
/// The constructor to create an OneFramePlaceholderImageStreamCompleter. The [images]
|
/// The constructor to create an OneFramePlaceholderImageStreamCompleter. The [images]
|
||||||
/// should be the primary images to display (typically asynchronously as they load).
|
/// should be the primary images to display (typically asynchronously as they load).
|
||||||
/// The [initialImage] is an optional image that will be emitted synchronously
|
/// The [initialImage] is an optional image that will be emitted synchronously
|
||||||
@@ -24,14 +20,14 @@ class OneFramePlaceholderImageStreamCompleter extends ImageStreamCompleter {
|
|||||||
InformationCollector? informationCollector,
|
InformationCollector? informationCollector,
|
||||||
void Function()? onLastListenerRemoved,
|
void Function()? onLastListenerRemoved,
|
||||||
}) {
|
}) {
|
||||||
|
setupListenerTracking(hadInitialImage: initialImage != null, onLastListenerRemoved: onLastListenerRemoved);
|
||||||
|
|
||||||
if (initialImage != null) {
|
if (initialImage != null) {
|
||||||
didProvideImage = true;
|
|
||||||
setImage(initialImage);
|
setImage(initialImage);
|
||||||
}
|
}
|
||||||
_onLastListenerRemoved = onLastListenerRemoved;
|
|
||||||
images.listen(
|
images.listen(
|
||||||
(image) {
|
(image) {
|
||||||
didProvideImage = true;
|
|
||||||
setImage(image);
|
setImage(image);
|
||||||
},
|
},
|
||||||
onError: (Object error, StackTrace stack) {
|
onError: (Object error, StackTrace stack) {
|
||||||
@@ -45,26 +41,4 @@ class OneFramePlaceholderImageStreamCompleter extends ImageStreamCompleter {
|
|||||||
},
|
},
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
@override
|
|
||||||
void addListener(ImageStreamListener listener) {
|
|
||||||
super.addListener(listener);
|
|
||||||
_listenerCount = _listenerCount + 1;
|
|
||||||
}
|
|
||||||
|
|
||||||
@override
|
|
||||||
void removeListener(ImageStreamListener listener) {
|
|
||||||
super.removeListener(listener);
|
|
||||||
_listenerCount = _listenerCount - 1;
|
|
||||||
|
|
||||||
final bool onlyCacheListenerLeft = _listenerCount == 1 && !didProvideImage;
|
|
||||||
final bool noListenersAfterImage = _listenerCount == 0 && didProvideImage;
|
|
||||||
|
|
||||||
final onLastListenerRemoved = _onLastListenerRemoved;
|
|
||||||
|
|
||||||
if (onLastListenerRemoved != null && (noListenersAfterImage || onlyCacheListenerLeft)) {
|
|
||||||
_onLastListenerRemoved = null;
|
|
||||||
onLastListenerRemoved();
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -0,0 +1,183 @@
|
|||||||
|
import 'dart:ui' as ui;
|
||||||
|
|
||||||
|
import 'package:flutter/painting.dart';
|
||||||
|
import 'package:flutter_test/flutter_test.dart';
|
||||||
|
import 'package:immich_mobile/presentation/widgets/images/cache_aware_listener_tracker.mixin.dart';
|
||||||
|
|
||||||
|
class TestImageCompleter extends ImageStreamCompleter with CacheAwareListenerTrackerMixin {
|
||||||
|
bool wasCancelled = false;
|
||||||
|
|
||||||
|
TestImageCompleter({required bool hadInitialImage}) {
|
||||||
|
setupListenerTracking(
|
||||||
|
hadInitialImage: hadInitialImage,
|
||||||
|
onLastListenerRemoved: () {
|
||||||
|
wasCancelled = true;
|
||||||
|
},
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
@override
|
||||||
|
void setImage(ImageInfo image) {
|
||||||
|
super.setImage(image);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
void main() {
|
||||||
|
late ImageCache cache;
|
||||||
|
late ImageStreamListener uiListener;
|
||||||
|
|
||||||
|
setUp(() {
|
||||||
|
// Create a fresh, real Flutter ImageCache for every test
|
||||||
|
cache = ImageCache();
|
||||||
|
uiListener = ImageStreamListener((_, __) {});
|
||||||
|
});
|
||||||
|
|
||||||
|
group('CacheAwareListenerTrackerMixin with Real ImageCache', () {
|
||||||
|
|
||||||
|
testWidgets('cancels fetch when UI detaches before completion', (WidgetTester tester) async {
|
||||||
|
final completer = TestImageCompleter(hadInitialImage: false);
|
||||||
|
final key = Object();
|
||||||
|
|
||||||
|
// 1. Request image from the real cache (simulating the provider)
|
||||||
|
final stream = cache.putIfAbsent(key, () => completer)!;
|
||||||
|
|
||||||
|
// 2. UI attaches
|
||||||
|
stream.addListener(uiListener);
|
||||||
|
expect(completer.wasCancelled, isFalse);
|
||||||
|
|
||||||
|
// 3. Simulate asynchronous network delay...
|
||||||
|
await tester.pump(const Duration(milliseconds: 150));
|
||||||
|
|
||||||
|
// 4. User scrolls away before network finishes. UI detaches.
|
||||||
|
stream.removeListener(uiListener);
|
||||||
|
|
||||||
|
expect(completer.wasCancelled, isTrue);
|
||||||
|
});
|
||||||
|
|
||||||
|
testWidgets('survives cache eviction while UI listener is still attached', (WidgetTester tester) async {
|
||||||
|
final completer = TestImageCompleter(hadInitialImage: false);
|
||||||
|
final key = Object();
|
||||||
|
|
||||||
|
// 1. Request image and attach UI
|
||||||
|
final stream = cache.putIfAbsent(key, () => completer)!;
|
||||||
|
stream.addListener(uiListener);
|
||||||
|
|
||||||
|
// 2. Simulate app going to background -> OS Memory Warning -> Cache clears
|
||||||
|
cache.clear();
|
||||||
|
|
||||||
|
// Even though the real cache just aggressively detached its listener,
|
||||||
|
// the stream MUST survive because the UI widget is still on screen!
|
||||||
|
expect(completer.wasCancelled, isFalse);
|
||||||
|
|
||||||
|
// 3. UI widget finally detaches
|
||||||
|
stream.removeListener(uiListener);
|
||||||
|
expect(completer.wasCancelled, isTrue);
|
||||||
|
});
|
||||||
|
|
||||||
|
testWidgets('survives synchronous cache detach during putIfAbsent with initialImage', (WidgetTester tester) async {
|
||||||
|
final completer = TestImageCompleter(hadInitialImage: true);
|
||||||
|
final key = Object();
|
||||||
|
|
||||||
|
// Run image creation outside FakeAsync zone to avoid hang
|
||||||
|
late ui.Image dummyImage;
|
||||||
|
await tester.runAsync(() async {
|
||||||
|
dummyImage = await createTestImage(width: 1, height: 1);
|
||||||
|
});
|
||||||
|
|
||||||
|
final initialImageInfo = ImageInfo(image: dummyImage);
|
||||||
|
|
||||||
|
final stream = cache.putIfAbsent(key, () {
|
||||||
|
completer.setImage(initialImageInfo);
|
||||||
|
return completer;
|
||||||
|
})!;
|
||||||
|
|
||||||
|
expect(completer.wasCancelled, isFalse);
|
||||||
|
|
||||||
|
stream.addListener(uiListener);
|
||||||
|
expect(completer.wasCancelled, isFalse);
|
||||||
|
|
||||||
|
stream.removeListener(uiListener);
|
||||||
|
expect(completer.wasCancelled, isTrue);
|
||||||
|
});
|
||||||
|
|
||||||
|
testWidgets('fires cleanup on full abandonment even after successful fetch', (WidgetTester tester) async {
|
||||||
|
final completer = TestImageCompleter(hadInitialImage: false);
|
||||||
|
final key = Object();
|
||||||
|
|
||||||
|
final stream = cache.putIfAbsent(key, () => completer)!;
|
||||||
|
stream.addListener(uiListener);
|
||||||
|
|
||||||
|
await tester.pump(const Duration(milliseconds: 100));
|
||||||
|
|
||||||
|
// Run image creation outside FakeAsync zone to avoid hang
|
||||||
|
late ui.Image dummyImage;
|
||||||
|
await tester.runAsync(() async {
|
||||||
|
dummyImage = await createTestImage(width: 1, height: 1);
|
||||||
|
});
|
||||||
|
|
||||||
|
completer.setImage(ImageInfo(image: dummyImage));
|
||||||
|
|
||||||
|
stream.removeListener(uiListener);
|
||||||
|
|
||||||
|
// The stream is completely abandoned (0 listeners), so it fires the cleanup hook.
|
||||||
|
// Since the image is already downloaded, canceling the network token is a safe no-op.
|
||||||
|
expect(completer.wasCancelled, isTrue);
|
||||||
|
});
|
||||||
|
|
||||||
|
testWidgets('Multiple UI listeners — only all detached, should cancel', (WidgetTester tester) async {
|
||||||
|
final completer = TestImageCompleter(hadInitialImage: false);
|
||||||
|
final key = Object();
|
||||||
|
|
||||||
|
final stream = cache.putIfAbsent(key, () => completer)!;
|
||||||
|
|
||||||
|
final uiListener2 = ImageStreamListener((_, __) {});
|
||||||
|
stream.addListener(uiListener);
|
||||||
|
stream.addListener(uiListener2);
|
||||||
|
|
||||||
|
// First UI detach leaves cache + one UI → no cancel
|
||||||
|
stream.removeListener(uiListener);
|
||||||
|
expect(completer.wasCancelled, isFalse);
|
||||||
|
|
||||||
|
// Second UI detach leaves only cache → cancel
|
||||||
|
stream.removeListener(uiListener2);
|
||||||
|
expect(completer.wasCancelled, isTrue);
|
||||||
|
});
|
||||||
|
|
||||||
|
testWidgets('Listener misidentification: new listener after cache eviction is not treated as cache', (WidgetTester tester) async {
|
||||||
|
final completer = TestImageCompleter(hadInitialImage: false);
|
||||||
|
final key = Object();
|
||||||
|
|
||||||
|
final stream = cache.putIfAbsent(key, () => completer)!;
|
||||||
|
|
||||||
|
// UI attaches
|
||||||
|
stream.addListener(uiListener);
|
||||||
|
|
||||||
|
// Cache eviction removes the cache listener
|
||||||
|
cache.clear();
|
||||||
|
expect(completer.wasCancelled, isFalse);
|
||||||
|
|
||||||
|
// A second UI listener attaches — must NOT be treated as cache
|
||||||
|
final uiListener2 = ImageStreamListener((_, __) {});
|
||||||
|
stream.addListener(uiListener2);
|
||||||
|
|
||||||
|
// Remove first UI listener; second UI still active → no cancel
|
||||||
|
stream.removeListener(uiListener);
|
||||||
|
expect(completer.wasCancelled, isFalse);
|
||||||
|
|
||||||
|
// Remove second UI listener; completely abandoned → cancel
|
||||||
|
stream.removeListener(uiListener2);
|
||||||
|
expect(completer.wasCancelled, isTrue);
|
||||||
|
});
|
||||||
|
|
||||||
|
testWidgets('No UI listener ever attaches (cache-only) — cache detaches should cancel', (WidgetTester tester) async {
|
||||||
|
final completer = TestImageCompleter(hadInitialImage: false);
|
||||||
|
final key = Object();
|
||||||
|
|
||||||
|
cache.putIfAbsent(key, () => completer);
|
||||||
|
|
||||||
|
// Cache eviction removes the only listener
|
||||||
|
cache.clear();
|
||||||
|
expect(completer.wasCancelled, isTrue);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user