Skip to content

Fixed Test case failure in PR 29037 - [2025/04/21] Candidate #29049

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

Conversation

HarishKumarSF4517
Copy link
Contributor

Description

This pull request includes changes to resolve the test failures on Candidate PR(#29037) and included the copilot code suggestion on the PhoneFlyoutPageRenderer class to improve code clarity and correctness.

Test cases:

Issue8632 - Changed the conditional compilation directive in Issue8632.cs from || (logical OR) to && (logical AND) to refine the conditions under which the test is excluded.
Issue28536 - Added base line image generated from CI.

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Apr 17, 2025
Copy link
Contributor

Hey there @@HarishKumarSF4517! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@dotnet-policy-service dotnet-policy-service bot added the partner/syncfusion Issues / PR's with Syncfusion collaboration label Apr 17, 2025
@anandhan-rajagopal anandhan-rajagopal added area-testing Unit tests, device tests test-flight labels Apr 17, 2025
@HarishKumarSF4517 HarishKumarSF4517 marked this pull request as ready for review April 17, 2025 07:45
@HarishKumarSF4517 HarishKumarSF4517 requested a review from a team as a code owner April 17, 2025 07:45
@HarishKumarSF4517 HarishKumarSF4517 requested review from jfversluis and jsuarezruiz and removed request for a team April 17, 2025 07:45
Copy link
Contributor

@bhavanesh2001 bhavanesh2001 Apr 17, 2025

Choose a reason for hiding this comment

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

The Windows snapshot doesn't look right. If SetBlur(10) is used on the canvas, I expect the specified radius (10) to be visibly blurred. I believe this works correctly on Android. So either this is a bug on Windows, or I might be mistaken.
CanvasShouldHonorBlur

Copy link
Contributor

@anandhan-rajagopal anandhan-rajagopal Apr 18, 2025

Choose a reason for hiding this comment

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

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.

@anandhan-rajagopal
Copy link
Contributor

#29049 (comment)

@PureWeen @bhavanesh2001 As per the current implementation on Windows, the GaussianBlurEffect only supports applying blur to the entire visual content, as documented here: https://learn.microsoft.com/en-us/windows/win32/direct2d/gaussian-blur. Therefore, the current output image on Windows is expected and aligns with the platform's behavior, as there is no available API to apply the blur effect selectively based on corner values.

Shall we proceed with the current image? Please share your thoughts or concerns.

@bhavanesh2001
Copy link
Contributor

@anandhan-rajagopal Keeping the snapshot doesn’t seem right to me. The expected behavior is that the blur should be applied within the specified radius. Windows not supporting that directly is more of a limitation we should handle, not something we should lock in as the correct output.

public interface IBlurrableCanvas
{
void SetBlur(float blurRadius);
}

@HarishKumarSF4517
Copy link
Contributor Author

HarishKumarSF4517 commented Apr 21, 2025

@anandhan-rajagopal Keeping the snapshot doesn’t seem right to me. The expected behavior is that the blur should be applied within the specified radius. Windows not supporting that directly is more of a limitation we should handle, not something we should lock in as the correct output.

public interface IBlurrableCanvas
{
void SetBlur(float blurRadius);
}

Hi @bhavanesh2001 , Thank you for your feedback. For now, I have decided to consider the Windows issues separately and have excluded Windows from the test case accordingly, along with linking the relevant issue report

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@PureWeen PureWeen merged commit 16cd5f2 into dotnet:inflight/candidate Apr 21, 2025
5 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-testing Unit tests, device tests community ✨ Community Contribution partner/syncfusion Issues / PR's with Syncfusion collaboration test-flight
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants