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.

Sign up for GitHub

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

Jump to bottom

Implement image caching functionality for iOS #4515

Merged
merged 4 commits into from
Feb 15, 2022
Merged

Implement image caching functionality for iOS #4515

merged 4 commits into from
Feb 15, 2022

Conversation

rachelkang
Copy link
Member

Description of Change

Implements #1966

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)
  • Targets a single property for a single control (or intertwined few properties)
  • Adds the property to the appropriate interface
  • Avoids any changes not essential to the handler property
  • Adds the mapping to the PropertyMapper in the handler
  • Adds the mapping method to the WinUI, Android, iOS, and Standard aspects of the handler
  • Implements the actual property updates (usually in extension methods in the Platform section of Core)
  • Tags ported renderer methods with [PortHandler]
  • Adds an example of the property to the sample project (MainPage)
  • Adds the property to the stub class
  • Implements basic property tests in DeviceTests

Does this PR touch anything that might affect accessibility?

  • Does this PR introduce a new control? (If yes, add an example using SemanticProperties to the SemanticsPage)
  • APIs that modify focusability?
  • APIs that modify any text property on a control?
  • Does this PR modify view nesting or view arrangement in anyway?
  • Is there the smallest possibility that your PR will change accessibility?
  • I'm not sure, please help me

If any of the above checkboxes apply to your PR, then the PR will need to provide testing to demonstrate that accessibility still works.

@rachelkang rachelkang added platform/iOS 🍎 area-image Image loading, sources, caching labels Feb 4, 2022
@rachelkang rachelkang added this to the 6.0.300-preview.14 milestone Feb 4, 2022
@rachelkang rachelkang requested a review from Redth February 4, 2022 19:05
Redth
Redth reviewed Feb 4, 2022
View reviewed changes
src/Core/src/ImageSources/UriImageSourceService/UriImageSourceService.iOS.cs Outdated
using UIKit;

namespace Microsoft.Maui
{
public partial class UriImageSourceService
{
internal string CacheDirectory = Essentials.FileSystem.CacheDirectory + "/com.microsoft.maui.sample/MauiUriImages/";
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if CachDirectory has a trailing /, but we could be extra safe and use Path.Combine() here to build up the path and let the sdk figure out the path separators.

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

Redth
Redth reviewed Feb 4, 2022
View reviewed changes
src/Core/src/ImageSources/UriImageSourceService/UriImageSourceService.iOS.cs Outdated
if (imageData == null)
throw new InvalidOperationException("Unable to load image stream data.");

await CacheImage(imageData, pathToImageCache);
Copy link
Member

Choose a reason for hiding this comment

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

Can you put a .ConfigureAwait(false) on the end of this call?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed the Task / await stuff since it's not needed

pictos
pictos reviewed Feb 8, 2022
View reviewed changes
src/Core/src/ImageSources/UriImageSourceService/UriImageSourceService.iOS.cs Outdated
using UIKit;

namespace Microsoft.Maui
{
public partial class UriImageSourceService
{
internal string CacheDirectory = Path.Combine(Essentials.FileSystem.CacheDirectory, "com.microsoft.maui.sample", "MauiUriImages");
Copy link
Contributor

@pictos pictos Feb 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hello, there.
I don't know the context behind this name, but do we want to keep the sample in the path?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be cache instead of sample and in the future use this path as a root directory for other caches (?)

Copy link
Member Author

Choose a reason for hiding this comment

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

updated, thanks for the feedback!

Redth
Redth reviewed Feb 9, 2022
View reviewed changes
src/Core/src/ImageSources/UriImageSourceService/UriImageSourceService.iOS.cs Outdated
if (imageData == null)
throw new InvalidOperationException("Unable to load image stream data.");

CacheImage(imageData, pathToImageCache);
Copy link
Member

Choose a reason for hiding this comment

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

I think we need a check here to ensure caching is enabled like in Android:

maui/src/Core/src/ImageSources/UriImageSourceService/UriImageSourceService.Android.cs

Line 32 in 12f17cf

if (!imageSource.CachingEnabled)

Redth
Redth reviewed Feb 9, 2022
View reviewed changes
src/Core/src/ImageSources/UriImageSourceService/UriImageSourceService.iOS.cs Outdated

NSData? imageData;

if (IsImageCached(pathToImageCache))
Copy link
Member

Choose a reason for hiding this comment

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

We should check here to see if caching is enabled for the image source... Android shows how to check the setting:

maui/src/Core/src/ImageSources/UriImageSourceService/UriImageSourceService.Android.cs

Line 32 in 12f17cf

if (!imageSource.CachingEnabled)

Probably faster to check this than to look if the file exists, if caching is disabled anyway.

@Redth Redth merged commit a46c6d0 into main Feb 15, 2022
@Redth Redth deleted the image-uri-caching branch February 15, 2022 16:47
@Redth Redth mentioned this pull request Feb 15, 2022
Closed
@Eilon
Copy link
Member

Eilon commented Feb 15, 2022

Just happened to come across this: how does a developer clear the cache if they want to force retrieving a new image? Of course they can set CachingEnabled=false and get a new image, but the cache will be stale, and will forever be tainted, no? I'm worried given that the caching seems enabled by default, that maybe this could lead to weird bugs in apps? Caching is a very hard problem and I am concerned this could lead to hard-to-find and hard-to-fix bugs.

@rachelkang
Copy link
Member Author

Thanks for the feedback @Eilon! Opened an issue here #4766 - feel free to add more info

@janseris
Copy link

Came across this and thought its for all platforms. Can it be for all platforms? Why is currently only iOS in plan?

@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 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.200-preview.14.2 Look for this fix in 6.0.200-preview.14.2! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pictos pictos pictos left review comments

@Redth Redth Redth approved these changes

Assignees
No one assigned
Labels
area-controls-image Image control fixed-in-6.0.200-preview.14.2 Look for this fix in 6.0.200-preview.14.2! platform/iOS 🍎
Projects
No open projects
[ARCHIVED] MAUI Planning
Status: Done
Milestone
6.0.300-preview.14
Development

Successfully merging this pull request may close these issues.
6 participants
@rachelkang @Eilon @janseris @Redth @pictos @samhouts