Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid Glide target re-use when loading the same image into the same view #6898

Merged
merged 3 commits into from
May 6, 2022

Conversation

mattleibow
Copy link
Member

@mattleibow mattleibow commented May 6, 2022

Description of Change

If all the models and properties are the same, Glide just re-triggers the previous target.

This causes issues with .NET and Task and async/await, so we set the callback (which is unique on each run) as a fallback image. This effectively makes each request unique so targets are not reused (but still uses the cache).

Issues Fixed

Fixes #6783

This is potentially the real cause of #6464 so we may need to revisit the eager cleanups

Testing

Since all the changes are in the java code, you can simply replace the maui.aar in the workload install folders and watch the magic happen.

mattleibow added 2 commits May 6, 2022 22:19
If all the models and properties are the same, Glide just re-triggers the previous target. This causes issues with .NET and Task and async/await, so we set the callback (which is unique on each run) as a fallback image. This effectively makes each request unique so targets are not reused (but still uses the cache).
@mattleibow mattleibow added t/bug Something isn't working platform/android 🤖 area-image Image loading, sources, caching p/0 Work that we can't release without labels May 6, 2022
@mattleibow mattleibow requested a review from Redth May 6, 2022 15:26
@mattleibow mattleibow self-assigned this May 6, 2022
@mattleibow mattleibow added this to the 6.0.300 milestone May 6, 2022
Comment on lines +216 to +221
// A special value to work around https://github.com/dotnet/maui/issues/6783 where targets
// are actually re-used if all the variables are the same.
// Adding this "error image" that will always load a null image makes each request unique,
// but does not affect the various caching levels.
builder = builder
.error(callback);
Copy link
Member Author

@mattleibow mattleibow May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the real-magic. We set the error image as the callback instance so it will always create a unique load path.

It is magic because we actually say "if there is an error, load the java object callback from C# as the image". This may seem weird, but we need a way to mark each builder as unique but strings and ints mean things. So the next best is just a java object we already know is unique.

Now since we can't actually load a java interface as an image on screen :) we need to add the loaders that handle these callbacks and just skip the work.

Comment on lines +38 to +39
// trigger the callback out of this target
post(() -> callback.onComplete(false, errorDrawable, this::clear));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Callbacks have to be on the post because if you try load a second image right after the first one, then you are still in the callback because the TaskCompletionSource.SetResult() continues on the same thread and thus all the code in C# that follows is effectively in the target still - which is not a Glide-supported operation.

Comment on lines +28 to +29
// add workaround loader for https://github.com/dotnet/maui/issues/6783
registry.prepend(ImageLoaderCallback.class, ImageLoaderCallback.class, new ImageLoaderCallbackModelLoaderFactory());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we added the callback as a fallback image, to avoid runtime errors in the log about unable to load the callback as an image, we create a dummy model loader that basically returns null.

@Redth Redth merged commit 259947a into main May 6, 2022
@Redth Redth deleted the dev/fix-6783 branch May 6, 2022 17:56
@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2023
@Eilon Eilon added area-controls-image Image control and removed area-image Image loading, sources, caching labels May 10, 2024
@samhouts samhouts added the fixed-in-6.0.312 Look for this fix in 6.0.312! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-image Image control fixed-in-6.0.312 Look for this fix in 6.0.312! p/0 Work that we can't release without platform/android 🤖 t/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android] Images aren't reloading correctly when you rebind to a CollectionView
4 participants