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

[net7.0] Fixed Android's StreamImageSourceService.LoadDrawableAsync() #16640

Merged
merged 9 commits into from Sep 20, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 9, 2023

Backport of #14109 to net7.0

/cc @PureWeen @jstedfast

@Eilon Eilon added the area/image 🖼️ Image loading, sources, caching label Aug 9, 2023
@samhouts samhouts added this to the .NET 7 + Servicing milestone Aug 9, 2023
@rmarinho
Copy link
Member

/rebase

@rmarinho
Copy link
Member

/azp run MAUI-DeviceTests-public

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rmarinho
Copy link
Member

rmarinho commented Aug 10, 2023

@PureWeen @jstedfast

/Users/builder/azdo/_work/1/s/src/Controls/tests/DeviceTests/Elements/Image/ImageTests.Android.cs(62,11): error CS1929: 'View' does not contain a definition for 'AssertContainsColor' 

@PureWeen
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@PureWeen PureWeen enabled auto-merge (squash) August 14, 2023 18:01
Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

ImageSetFromStreamRenders is failing

@PureWeen PureWeen assigned PureWeen and unassigned jstedfast Aug 19, 2023
@jstedfast jstedfast self-assigned this Aug 30, 2023
@mattleibow
Copy link
Member

/azp run MAUI-DeviceTests-public

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mattleibow mattleibow dismissed PureWeen’s stale review August 30, 2023 20:25

Things should be fixed now!

@jstedfast
Copy link
Member

@mattleibow nope :(

Looks like the error is in some screenshotting code and the width/height are -1 or something.

Probably something needs to be fixed in the unit test or in the testing infrastructure, but I'm not sure what.

@samhouts samhouts added stale Indicates a stale issue/pr and will be closed soon s/pr-needs-author-input PR needs an update from the author labels Sep 18, 2023
@ghost
Copy link

ghost commented Sep 18, 2023

Hi @github-actions[bot]. We have added the "s/pr-needs-author-input" label to this issue, which indicates that we have an open question/action for you before we can take further action. This PRwill be closed automatically in 14 days if we do not hear back from you by then - please feel free to re-open it if you come back to this PR after that time.

SetupBuilder();
var layout = new VerticalStackLayout()
{
HeightRequest = 100,
Copy link
Member

Choose a reason for hiding this comment

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

@mattleibow @jstedfast I modified this test to just set the height/width because I don't think the remeasuring of the layout is relevant to the test here.

I also don't think this test completely accurately embodies the original issue. I rolled back the changes from this PR and this test still passes without throwing a UIThread exception

Copy link
Member

Choose a reason for hiding this comment

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

I haven't been able to reproduce the original issue outside the sample repository.

I started a branch here to attempt reproducing
https://github.com/dotnet/maui/tree/test_for_Issue14052 but haven't had any luck yet.

I did test this PR directly against the repro and was able to reproduce the crash and then verified this PR fixes the crash.

This fix has been on NET8 since preview5 so I feel alright to merge this Backport

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

D'oh, this Width/HeightRequest change makes total sense. I was assuming the issue was that the screenshot was happening before layout had finished or something and so the width/height of the element wasn't yet known (hence the -1 values).

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I think the test only ever randomly failed - I don't think it failed every run.

@PureWeen PureWeen marked this pull request as draft September 18, 2023 21:22
auto-merge was automatically disabled September 18, 2023 21:22

Pull request was converted to draft

@PureWeen PureWeen marked this pull request as ready for review September 18, 2023 23:32
@jstedfast jstedfast self-requested a review September 19, 2023 13:48
@rmarinho
Copy link
Member

/azp run MAUI-DeviceTests-public

@rmarinho rmarinho enabled auto-merge (squash) September 19, 2023 13:53
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rmarinho rmarinho merged commit ee47877 into net7.0 Sep 20, 2023
26 of 29 checks passed
@rmarinho rmarinho deleted the backport/pr-14109-to-net7.0 branch September 20, 2023 13:13
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2023
@ghost ghost removed stale Indicates a stale issue/pr and will be closed soon s/pr-needs-author-input PR needs an update from the author labels Dec 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/image 🖼️ Image loading, sources, caching platform/android 🤖
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants