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

BoxShadow.ToString() do not allow to parse it again (exception thrown) #14211

Closed
dbriard opened this issue Jan 15, 2024 · 4 comments · Fixed by #14228
Closed

BoxShadow.ToString() do not allow to parse it again (exception thrown) #14211

dbriard opened this issue Jan 15, 2024 · 4 comments · Fixed by #14228
Labels

Comments

@dbriard
Copy link
Contributor

dbriard commented Jan 15, 2024

Describe the bug

The ToString() method of BoxShadow to not allow to Parse it again. It throw an exception.

To Reproduce

  1. Create a BoxShadow with OffsetX=1, OffsetY=0, Blur=0, Color=#ffaa00
  2. ToString => 1 #ffaa00
  3. Try to parse it => if (p.Length < 3 || p.Length > 6) throw new FormatException();

Expected behavior

ToString() should write all BoxShadow properties all the time, or we cannot parse it again, and also we don't know what value in the string correspond to what properties...

I would like to do:
var boxShadow2 = BoxShadow.Parse(boxShadow1.ToString())

Environment

  • Avalonia-Version: 11.0.6

Additional context

In the code below, we don't know don't know what value in the string correspond to what property...

internal void ToString(StringBuilder sb)
{
    if (this == default(BoxShadow))
    {
        sb.Append("none");
        return;
    }

    if (IsInset)
    {
        sb.Append("inset ");
    }

    if (OffsetX != 0.0)
    {
        sb.AppendFormat("{0} ", OffsetX.ToString(CultureInfo.InvariantCulture));
    }

    if (OffsetY != 0.0)
    {
        sb.AppendFormat("{0} ", OffsetY.ToString(CultureInfo.InvariantCulture));
    }

    if (Blur != 0.0)
    {
        sb.AppendFormat("{0} ", Blur.ToString(CultureInfo.InvariantCulture));
    }

    if (Spread != 0.0)
    {
        sb.AppendFormat("{0} ", Spread.ToString(CultureInfo.InvariantCulture));
    }

    sb.AppendFormat("{0}", Color.ToString());
}
@dbriard dbriard added the bug label Jan 15, 2024
@Gillibald
Copy link
Contributor

OffsetX and OffsetY are required all the time

@dbriard
Copy link
Contributor Author

dbriard commented Jan 16, 2024

@Gillibald Sure, but in the ToString method, if OffsetX or OffsetY is zero, it is skiped.

The two BoxShadow below produce the same output string, and cannot be parsed as they do not have a minimum of 3 values.
OffsetX=0 OffsetY=1 Color=Red => "1 Red"
OffsetX=1 OffsetY=0 Color=Red => "1 Red"

It is even worse when using other parameters, as we can't know if the 1 is for OffsetX or Y, Blur or Spread:
OffsetX=1 OffsetY=0 Color=Red => "1 Red"
OffsetX=0 OffsetY=0 Blur=1 Color=Red => "1 Red"

My suggestion is to remove the four zero checks in the ToString() and to always write OffsetX, Y, Blur, Spread in the output string.

@Gillibald
Copy link
Contributor

Gillibald commented Jan 16, 2024

It should look like so:

internal void ToString(StringBuilder sb)
{
    if (this == default(BoxShadow))
    {
        sb.Append("none");
        return;
    }

    if (IsInset)
    {
        sb.Append("inset ");
    }

    sb.AppendFormat("{0} ", OffsetX.ToString(CultureInfo.InvariantCulture)); 

    sb.AppendFormat("{0} ", OffsetY.ToString(CultureInfo.InvariantCulture));
    
    if (Blur != 0.0)
    {
        sb.AppendFormat("{0} ", Blur.ToString(CultureInfo.InvariantCulture));
    }

    if (Spread != 0.0)
    {
        sb.AppendFormat("{0} ", Spread.ToString(CultureInfo.InvariantCulture));
    }

    sb.AppendFormat("{0}", Color.ToString());
}

The other parameters are optional

@dbriard
Copy link
Contributor Author

dbriard commented Jan 16, 2024

Personnally I never use Spread so that's fine to me, but if you keep the checks you cannot differentiate those cases:

Blur = 0 and Spread = 1
and Blur = 1 and Spread = 0

So I think it is better to always write all four values.

workgroupengineering added a commit to workgroupengineering/Avalonia that referenced this issue Jan 16, 2024
github-merge-queue bot pushed a commit that referenced this issue Feb 7, 2024
* test: Refatctoring BoxShadowTests, handle ToString case when Blur is 0 and Spread is not zero

* fix: #14211 BoxShadow.ToString() behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants