fix(mobile): handle image stream completion when no image is emitted#25895
fix(mobile): handle image stream completion when no image is emitted#25895LeLunZ wants to merge 12 commits intoimmich-app:mainfrom
Conversation
…to SortOrder enum
|
Label error. Requires exactly 1 of: changelog:.*. Found: 📱mobile. A maintainer will add the required label. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a mobile app issue where full-screen images get stuck in a loading state when users click on thumbnails before they finish loading. The fix adds an onDone handler to the OneFramePlaceholderImageStreamCompleter that reports a silent error when the image stream completes without emitting any frames or errors, unblocking waiting code and allowing image loading to proceed.
Changes:
- Added boolean flags
_emittedImageand_emittedErrorto track stream state - Added
onDonehandler that reports an error when stream completes without emitting anything - Wrapped stream callbacks to set the tracking flags appropriately
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mobile/lib/presentation/widgets/images/one_frame_multi_image_stream_completer.dart
Show resolved
Hide resolved
|
Handling this in the stream completer with an error seems odd. I think this should be handled in the image provider. It seems like a control flow / sequencing issue if throwing an error like that makes it work. |
|
@mertalev thx for the fast answer. The problem I found is that the image loading is stuck at I placed a log at the @override
void dispose() {
final imageProvider = widget.imageProvider;
if (imageProvider is CancellableImageProviderMixin) {
print('Cancelling image provider with asset ${imageProvider} ${imageProvider.isCancelled}');
imageProvider.cancel();
}Then logs in @override
Stream<ImageInfo> initialImageStream() async* {
final cachedOperation = this.cachedOperation;
print('Checking for cached image for assetId: ${assetId}');
if (cachedOperation == null) {
print('No cached image for assetId: ${assetId}');
return;
}
try {
print('Loading initial cached image for assetId: ${assetId}');
final cachedImage = await cachedOperation.valueOrCancellation();
print('Loaded initial cached image for assetId: ${assetId}: $cachedImage');
if (cachedImage != null && !isCancelled) {
yield cachedImage;
}
} catch (e, stack) {
_log.severe('Error loading initial image', e, stack);
} finally {
this.cachedOperation = null;
}
}The logs look like this As you can see from the logs it cancels the image from the dispose method of the Thumbnail widget (interestingly twice?) Then For me it seems when the original OneFramePlaceholderImageStreamCompleter of the RemoteThumbProvider doesn't get an image (the original provider is cancelled), and also doesn't error, it goes stale. I think important to understand is that
Both doesn't happen when the image request is cancelled. Also |
|
Important finding: This never runs in the cancel method of the CancellableImageProvider: final operation = cachedOperation;
if (operation != null) {
print('Cancelling cached operation');
cachedOperation = null;
operation.cancel();
} |
|
@mertalev I studied the code a bit more, and found the issue + a fix. Issue: The problem: the Thumbnail widget calls So now the In the current immich application there are 2 ways the image request can get cancelled:
That makes it problematical, as no Widget actually owns an image/imagestream or whatever. So it shouldn't be able to forcefully cancel a request. The fix: Instead of letting widgets call So I think I implemented the quickest fix with the No need to call Another question (which doesn't have anything to do with the issue but just in general): From Or the other way around, why even have the |
|
And it seems I messed up and also put other changes in the PR. Really weird. Will close this and create a new one. Sry |
Description
Fix fullscreen image stuck state by handling “stream completed without frames” in
OneFramePlaceholderImageStreamCompleter.When a thumbnail request is cancelled mid-load, the image stream can finish without emitting an
ImageInfoand without an error. In that casegetInitialImage()never completes itscachedOperation, soRemoteFullImageProviderblocks indefinitely oninitialImageStream()and image never advances to preview/original.This PR adds an
onDonehandler that reports a silent error when the stream completes without producing any frames or errors, unblockinggetInitialImage()waiters and allowing fullscreen loading to proceed.Other questions that are still open:
Another thing which I thought about what would be even better:
But I am not sure how/if thats currently doable.
I don't really get the whole image loading pipeline, so please let me know what you think.
Fixes #25723
How Has This Been Tested?
Checklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)Please describe to which degree, if any, an LLM was used in creating this pull request.
Getting a quick overview of the code, and some insights on where to investigate.