-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[Android] Fix: Prevent Duplicate MapSource Invocations on setting Image's Source to Local File Paths #28405
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
[Android] Fix: Prevent Duplicate MapSource Invocations on setting Image's Source to Local File Paths #28405
Conversation
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you include a test? Like tap a Button to load the image source, use the Image PropertyChanged event to check the source property and increase a counter. In that way, verify that MapSource is only invoked once.
int sourceChangedCounter = 0;
image.PropertyChanged += (sender, args) =>
{
if (args.PropertyName == nameof(Image.Source))
{
sourceChangedCounter++;
}
};
Thank you for the suggestion, @jsuarezruiz. I could write a test similar to what you described, but I believe it wouldn’t accurately represent the issue I’m trying to address. The line below invokes the
Since |
To replicate the issue in the Sandbox app, follow the steps below: 1. Add Image to ResourcesAdd 2. Define UI in
|
imageHandler.UpdateValue(nameof(IImage.Source)); |
5. Test the Behavior
- Run the app.
- Click "To Local Storage" to generate and save image files locally.
- Click "Load Images" to bind the list of image paths to the
CollectionView
. - Scroll through the
CollectionView
.
Without the PR changes:
- The
UpdateValue()
method will be invoked multiple times while scrolling. - This is unintended and incorrect behavior.
With the PR changes:
- The
UpdateValue()
method will not be invoked during scrolling.
This can also explain my degradating performance in my CV's with local images |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use another field for the value and also is this testable?
685db9f
to
95732d4
Compare
…ge's Source to Local File Paths (#28405) * add extra condition * use _sourceCancellation
…ge's Source to Local File Paths (#28405) * add extra condition * use _sourceCancellation
…ge's Source to Local File Paths (#28405) * add extra condition * use _sourceCancellation
Description of Change
This PR addresses an side effect introduced by #24023 where setting the image source to a local file path triggers duplicate calls to the MapSource mapper. This behavior can lead to performance degradation—especially in image-heavy scenarios such as CollectionView or CarouselView.In such cases, multiple calls could potentially compound the load and result in a significant performance hit when images are loaded within ItemsViews.
Problem:
When the platform view attaches to the window, the handler checks if the image drawable is null and then calls
UpdateValue(nameof(IImage.Source))
to reload the image. With local file paths, this can happen even if the image is already in the process of loading, causing MapSource to be fired multiple times unnecessarily.Fix:
To mitigate this, introduced an
IsLoading
flag in the ImageSourceServiceResultManager. The flag is set to true when a load begins (viaBeginLoad()
) and reset to false upon load completion (viaCompleteLoad()
). InOnPlatformViewAttachedToWindow
, we now check this flag before triggering a reload: