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

IsNullOrEmpty for Paint not returning true if Color(s) are not set #15668

Merged
merged 8 commits into from
Mar 3, 2025
Merged

IsNullOrEmpty for Paint not returning true if Color(s) are not set #15668

merged 8 commits into from
Mar 3, 2025
+175 −14

Conversation

mos379
Copy link
Contributor

@mos379 mos379 commented Jun 15, 2023 •
edited by samhouts
Loading

Description of Change

Adding additional condition for IsNullOrEmpty check

Issues Fixed

Fixes #15667

@ghost ghost added the community ✨ Community Contribution label Jun 15, 2023
@ghost
Copy link

ghost commented Jun 15, 2023

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

@Eilon Eilon added the area-drawing Shapes, Borders, Shadows, Graphics, BoxView, custom drawing label Jun 16, 2023
@rmarinho

This comment was marked as off-topic.

Sign in to view
@rmarinho rmarinho requested a review from jsuarezruiz June 21, 2023 10:10
@azure-pipelines

This comment was marked as outdated.

Sign in to view
@PureWeen

This comment was marked as outdated.

Sign in to view
@azure-pipelines

This comment was marked as off-topic.

Sign in to view
jsuarezruiz
jsuarezruiz previously approved these changes Jul 5, 2023
View reviewed changes
mattleibow
mattleibow requested changes Jul 5, 2023
View reviewed changes
Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

This PR is on the right track, but after my first comment about adding a test, I think we may also actually want to rather make null become white instead. So a "blank" gradient is the same as 2 white stops?

What happens if one of the stops is null - such that start and end are not null, but there is one in the middle that is null.

@jsuarezruiz thoughts?

src/Core/src/Graphics/PaintExtensions.cs Outdated
@@ -28,7 +28,7 @@ public static bool IsNullOrEmpty([NotNullWhen(true)] this Paint? paint)
return solidPaint == null || solidPaint.Color == null;

if (paint is GradientPaint gradientPaint)
return gradientPaint == null || gradientPaint.GradientStops.Length == 0;
return gradientPaint == null || gradientPaint.GradientStops.Length == 0 || gradientPaint.StartColor == null || gradientPaint.EndColor == null;
Copy link
Member

Choose a reason for hiding this comment

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

What happens if just one color is set? Maybe a null color should go through to Graphics, but then be set to white? The default in graphics seems to be white, so we could replace nulls in the stops with white?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mnn, could be an option. i changed this validation for now to check if the gradient StartColor and EndColor is null. In that case, without start and end colors, we can consider the gradient empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattleibow how much longer do we have to wait for this tiny fix?

@jsuarezruiz jsuarezruiz requested a review from mattleibow July 6, 2023 11:26
@jsuarezruiz jsuarezruiz requested a review from mattleibow July 19, 2023 12:02
@jsuarezruiz jsuarezruiz mentioned this pull request Aug 10, 2023
Closed
@samhouts samhouts modified the milestones: Under Consideration, .NET 8 GA Aug 29, 2023
@mattleibow

This comment was marked as outdated.

Sign in to view
@azure-pipelines

This comment was marked as off-topic.

Sign in to view
@samhouts
Copy link
Member

@mos379 I was unable to reproduce the issue that the PR was meant to solve. Are you sure we still need this? Thanks!

@samhouts samhouts added the s/pr-needs-author-input PR needs an update from the author label Sep 15, 2023
@ghost
Copy link

ghost commented Sep 15, 2023

Hi @mos379. 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.

@ghost

This comment was marked as outdated.

Sign in to view
@ghost ghost added the stale Indicates a stale issue/pr and will be closed soon label Sep 25, 2023
@mos379
Copy link
Contributor Author

mos379 commented Sep 27, 2023

@samhouts this is still the case crashing my application
easy to reproduce

    <Style
        x:Key="TestBackground"
        ApplyToDerivedTypes="True"
        TargetType="{x:Type ContentPage}">
        <Setter Property="Background">
            <Setter.Value>
                <LinearGradientBrush EndPoint="0,1">
                    <GradientStop Offset="0.1" />
                    <GradientStop Offset="1" Color="Red" />
                </LinearGradientBrush>
            </Setter.Value>
        </Setter>
    </Style>

crashes
while adding a color to the first gradient works

tested with 8.0.100-rc.1.23455.8

@mos379 mos379 changed the title IsNullOrEmpty for Paint not returning true if Colors are not set IsNullOrEmpty for Paint not returning true if Color(s) are not set Sep 27, 2023
@ghost ghost removed the stale Indicates a stale issue/pr and will be closed soon label Sep 27, 2023
@mos379 mos379 requested a review from a team as a code owner September 27, 2023 08:08
@PureWeen PureWeen modified the milestones: .NET 8 GA, .NET 8 SR1 Sep 28, 2023
@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Oct 12, 2023
@samhouts

This comment was marked as off-topic.

Sign in to view
@azure-pipelines

This comment was marked as off-topic.

Sign in to view
@Redth Redth removed this from the .NET 8 SR1 milestone Nov 30, 2023
@azure-pipelines Azure Pipelines

This comment was marked as off-topic.

Sign in to view
@jfversluis

This comment was marked as off-topic.

Sign in to view
@jsuarezruiz
Copy link
Contributor

jsuarezruiz commented Jun 12, 2024 •
edited by jfversluis
Loading

/azp run

@azure-pipelines Azure Pipelines

This comment was marked as outdated.

Sign in to view
@mos379
Copy link
Contributor Author

mos379 commented Jun 12, 2024

Oh this is still in the process 😂

@jfversluis jfversluis added this to the .NET 9 SR5 milestone Feb 19, 2025
@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mattleibow mattleibow merged commit 4dcf1e3 into dotnet:main Mar 3, 2025
123 checks passed
tj-devel709 pushed a commit that referenced this pull request Mar 3, 2025
@mos379 @jsuarezruiz @tj-devel709
IsNullOrEmpty for Paint not returning true if Color(s) are not set (#…
eda4183 
…15668)

* IsNullOrEmpty for Paint not returning true if Colors are not set

* Updated unit test

* Updated tests

* More tests

* Added more tests

* Updated conditions

* Added more tests

* Changed condition to work when only one collor is missing

---------

Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
@mos379
Copy link
Contributor Author

mos379 commented Mar 4, 2025

WOW after almost 2 years :)

bhavanesh2001 pushed a commit to bhavanesh2001/maui that referenced this pull request Mar 7, 2025
@mos379 @jsuarezruiz @bhavanesh2001
IsNullOrEmpty for Paint not returning true if Color(s) are not set (d…
bd8bdf4 
…otnet#15668)

* IsNullOrEmpty for Paint not returning true if Colors are not set

* Updated unit test

* Updated tests

* More tests

* Added more tests

* Updated conditions

* Added more tests

* Changed condition to work when only one collor is missing

---------

Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattleibow mattleibow

@jsuarezruiz jsuarezruiz

Assignees
No one assigned
Labels
area-drawing Shapes, Borders, Shadows, Graphics, BoxView, custom drawing community ✨ Community Contribution partner/syncfusion/review stale Indicates a stale issue/pr and will be closed soon
Projects
MAUI SDK Ongoing
Status: Done
Milestone
.NET 9 SR5
Development

Successfully merging this pull request may close these issues.

[regression/8.0.0-preview.5.8529] invalid GradientBrush color causes crash
9 participants
@mos379 @rmarinho @PureWeen @mattleibow @samhouts @jfversluis @jsuarezruiz @Eilon @Redth