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

fix: [iOS] [Android] Fix set bitmap stream after delay not update Image - set bitmap URI not update ImageBrush #8370

Conversation

ghost
Copy link

@ghost ghost commented Mar 17, 2022

GitHub Issue (If applicable): closes #5453
closes #7288

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Changing the stream of a BitmapImage wasn't firing Image Control`s OnSourceChanged event.
It was expected the same behavior like changing UriSource property of BitmapImage.

The same occurs whith ImageBrush when changing the URI of the bitmap.

What is the new behavior?

Now, changing the Stream of a BitmapImage will trigger the OnInvalidated event that make possible to Image Control to update itself, loading the bitmap as it should be.

The OnInvalidated event is also used to make possible Border / ImageBrush track the changes of its ImageSource.

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

@ghost ghost self-requested a review March 17, 2022 15:38
@gitpod-io
Copy link

gitpod-io bot commented Mar 17, 2022

@ghost ghost marked this pull request as draft March 17, 2022 15:47
@ghost ghost marked this pull request as ready for review March 17, 2022 16:51
@ghost ghost marked this pull request as draft March 17, 2022 18:57
src/Uno.UI/UI/Xaml/Media/ImageSource.cs Outdated Show resolved Hide resolved
src/Uno.UI/UI/Xaml/Media/ImageSource.cs Outdated Show resolved Hide resolved
src/Uno.UI/UI/Xaml/Media/ImageSource.cs Outdated Show resolved Hide resolved
@ghost ghost force-pushed the dev/ICSI/5453-ios-setting-bitmap-source-after-assigned-wont-work branch 3 times, most recently from 1a4ed9e to ce14410 Compare March 22, 2022 17:07
@ghost ghost force-pushed the dev/ICSI/5453-ios-setting-bitmap-source-after-assigned-wont-work branch 2 times, most recently from a0dcb2f to 20bd722 Compare March 24, 2022 19:07
@ghost ghost marked this pull request as ready for review March 24, 2022 19:08
@ghost ghost force-pushed the dev/ICSI/5453-ios-setting-bitmap-source-after-assigned-wont-work branch from 20bd722 to 6ff5470 Compare March 25, 2022 10:54
@MartinZikmund
Copy link
Member

@iury-kc There are some relevant build issues (check the UWP build error messages)

@ghost ghost force-pushed the dev/ICSI/5453-ios-setting-bitmap-source-after-assigned-wont-work branch from 6ff5470 to 1f7689a Compare March 30, 2022 09:28
@ghost ghost changed the title fix: [iOS] Fix setting bitmap source after delay not updating Image fix: [iOS] [Android] Fix setting bitmap source after delay not updating Image Mar 31, 2022
@ghost ghost changed the title fix: [iOS] [Android] Fix setting bitmap source after delay not updating Image fix: [iOS] [Android] Fix set bitmap source after delay not update Image - set bitmap URI not update ImageBrush Mar 31, 2022
@ghost ghost changed the title fix: [iOS] [Android] Fix set bitmap source after delay not update Image - set bitmap URI not update ImageBrush fix: [iOS] [Android] Fix set bitmap stream after delay not update Image - set bitmap URI not update ImageBrush Mar 31, 2022
@ghost
Copy link
Author

ghost commented Mar 31, 2022

As the Unit Tests are not passing, probably by delay issues, I created UI Tests. Squashed and wainting Azure tests results.

@ghost ghost force-pushed the dev/ICSI/5453-ios-setting-bitmap-source-after-assigned-wont-work branch 2 times, most recently from 478f28f to 1e2943e Compare April 1, 2022 11:51
@ghost
Copy link
Author

ghost commented Apr 4, 2022

I increased timeout to 10 seconds, however, still having errors waiting controls. And there's another test failing and not exactly related to the commit. Any suggestions? Thanks!!

@carldebilly
Copy link
Member

@iury-kc I don't think the problem is timeout related...

ImageAssert.AreEqual @ line 420
pixelTolerance: No color tolerance
expected: Before (When_BitmapImage_is_SetSource_After_Delay_Before.png {Width=1024, Height=768}) in {X=10,Y=134,Width=1004,Height=50}
actual  : After (When_BitmapImage_is_SetSource_After_Delay_After.png {Width=1024, Height=768}) in {X=10,Y=134,Width=1004,Height=50}

@ghost
Copy link
Author

ghost commented Apr 27, 2022

Tests are giving errors because issues related:

#7328
#8618

Solving that, will allow test pass.

@ghost
Copy link
Author

ghost commented Jul 4, 2022

Since #7328 was successfully merged, i`ll go back to this one asap. Thanks @MartinZikmund !!

@ghost ghost force-pushed the dev/ICSI/5453-ios-setting-bitmap-source-after-assigned-wont-work branch from 1e2943e to 2787d9c Compare July 4, 2022 11:06
@agneszitte
Copy link
Contributor

@iury-kc what's left for this PR ;) ?

@jeromelaban jeromelaban marked this pull request as draft September 14, 2022 15:08
@ghost ghost force-pushed the dev/ICSI/5453-ios-setting-bitmap-source-after-assigned-wont-work branch from 2787d9c to 08a8f47 Compare December 16, 2022 16:56
@github-actions github-actions bot added area/automation Categorizes an issue or PR as relevant to project automation platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform labels Dec 16, 2022
@ghost
Copy link
Author

ghost commented Dec 16, 2022

Sorry @agneszitte-nventive ... didin't see your message.

I was waiting some changes in StorageFile. Then, after going to Uno.Figma I didn't have time to move on with this one.
Rebased now... Let's see!

@ghost ghost marked this pull request as ready for review December 16, 2022 16:58
@ghost ghost force-pushed the dev/ICSI/5453-ios-setting-bitmap-source-after-assigned-wont-work branch 2 times, most recently from b17b61d to a6afbbb Compare December 16, 2022 18:28
@ghost ghost force-pushed the dev/ICSI/5453-ios-setting-bitmap-source-after-assigned-wont-work branch from a6afbbb to 2d572d5 Compare December 16, 2022 18:31
@agneszitte
Copy link
Contributor

@iury-kc this PR is still a work in progress left aside? (cc @jeromelaban)

@ghost
Copy link
Author

ghost commented Jan 12, 2023

Originally it was waiting for some adjusts in StorageFile.cs ..
And then, I couldn't make the tests pass...
I rebased the code to see if the tests were passing now, but they didn't.

Quite sure Im missing something pretty simple. And to be honest, i didn't had time yet to look at it closer, after joinning Figma team. Still Im very curious to understand better this case. If any of you @jeromelaban , @MartinZikmund (since Martin worked in StorageFile.cs) could give me some tips. Thanks!

@iurycarlos iurycarlos assigned iurycarlos and unassigned ghost Mar 8, 2023
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


iury seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

ImageBrush.AlignmentYProperty,
(_, __) => imageBrushCallback()
).DisposeWith(disposables);
imageBrush.ImageChanged += ImageChanged;
Copy link
Member

Choose a reason for hiding this comment

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

These changes overlap the contents of #12234 by @MartinZikmund, where we want to remove subscriptions and events in general (they're more costly than using method overrides). What do you think, @MartinZikmund?

@iurycarlos
Copy link
Contributor

Also, didn't have the chance to test it after #12076 .

@jeromelaban
Copy link
Member

The target code has changed significantly, let's review later when this will get prioritized. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/automation Categorizes an issue or PR as relevant to project automation platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform
Projects
None yet
8 participants