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

Disable caching when loading images from streams #13111

Merged
merged 3 commits into from
Mar 13, 2023
Merged

Disable caching when loading images from streams #13111

merged 3 commits into from
Mar 13, 2023

Conversation

hartez
Copy link
Contributor

@hartez hartez commented Feb 3, 2023

Description of Change

Disables all caching when loading images from streams, because there's no available data for creating a reasonable cache key.

See this comment for a deeper explanation.

Other references:

Issues Fixed

Fixes #8676
Related to #9773

@hartez hartez marked this pull request as draft February 3, 2023 17:38
@hartez
Copy link
Contributor Author

hartez commented Feb 3, 2023

Making this a draft so we don't accidentally merge it before I can get someone to write proper tests.

@hartez
Copy link
Contributor Author

hartez commented Feb 3, 2023

We could write a device test to verify that caching is disabled for FromInputStream() on Android. We'd have to force the problem during a single application run, so we'd need to write a deliberately bad subclass of MemoryStream that always returns the same values for ToString() and GetHashCode(), and always returns true from Equals(). Then we could load image data into two instances of it, pass them both to FromInputStream(), and verify that the image data we got back was different.

For reference, the Glide ObjectKey source: https://github.com/bumptech/glide/blob/master/library/src/main/java/com/bumptech/glide/signature/ObjectKey.java

@Eilon Eilon added the area-image Image loading, sources, caching label Feb 3, 2023
@hartez hartez marked this pull request as ready for review March 10, 2023 21:58
@github-actions
Copy link
Contributor

Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines.

@espenrl
Copy link
Contributor

espenrl commented Apr 26, 2023

@hartez Will this one be backported to .NET 7? I have this scenario where a user can rotate an image 90 degrees, and having to use the same filename/path on storage means it will be cached and the user will not see the rotated image.

@espenrl
Copy link
Contributor

espenrl commented May 19, 2023

@hartez I know this is a breaking change, but would you consider backporting this to .NET 7 under a feature flag? I make a business app for a company and the app has the ability to rotate images. The image is rotated, stored on disk (at the same path which is key), Image.Uri get a property changed event - and it works great on Windows and iOS. But not Android. I think this PR would solve that.

And regarding the image path, it can't be changed, it is tied to backend synchronization logic of a third party.

@hartez hartez added the backport/suggested The PR author or issue review has suggested that the change should be backported. label May 19, 2023
@hartez
Copy link
Contributor Author

hartez commented May 24, 2023

/backport to net7.0

@github-actions
Copy link
Contributor

Started backporting to net7.0: https://github.com/dotnet/maui/actions/runs/5070757070

@github-actions
Copy link
Contributor

@hartez backporting to net7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Disable caching when loading images from streams
Applying: Automated test
.git/rebase-apply/patch:29: trailing whitespace.
		
.git/rebase-apply/patch:33: trailing whitespace.
			await InvokeOnMainThreadAsync(async () => { 
.git/rebase-apply/patch:56: trailing whitespace.
	class TestMemoryStream : MemoryStream 
.git/rebase-apply/patch:99: trailing whitespace.
		static bool IsMatching(Bitmap bitmap1, Bitmap bitmap2) 
warning: 4 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	src/TestUtils/src/DeviceTests/AssertionExtensions.Android.cs
Falling back to patching base and 3-way merge...
Auto-merging src/TestUtils/src/DeviceTests/AssertionExtensions.Android.cs
CONFLICT (content): Merge conflict in src/TestUtils/src/DeviceTests/AssertionExtensions.Android.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Automated test
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Contributor

@hartez an error occurred while backporting to net7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

hartez added a commit that referenced this pull request May 25, 2023
github-actions bot pushed a commit that referenced this pull request May 28, 2023
rmarinho pushed a commit that referenced this pull request May 28, 2023
@hartez hartez added the backport/approved After some discussion or review, this PR or change was approved to be backported. label May 30, 2023
@espenrl
Copy link
Contributor

espenrl commented May 30, 2023

Thanks for the backport @hartez

@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 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-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149! 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 backport/approved After some discussion or review, this PR or change was approved to be backported. backport/suggested The PR author or issue review has suggested that the change should be backported. fixed-in-8.0.0-preview.3.8149 Look for this fix in 8.0.0-preview.3.8149!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image caching should be configurable for all ImageSources CollectionView is displaying incorrect images
5 participants