search

ForNeVeR / xaml-math

A collection of .NET libraries for rendering mathematical formulae using the LaTeX typesetting style, for the WPF and Avalonia XAML-based frameworks
MIT License
641 stars 102 forks source link

The logic in HorizontalBox.Add is broken #403

Closed Orace closed 1 year ago

Orace commented 1 year ago

Actual code reads:

https://github.com/ForNeVeR/xaml-math/blob/d4330c54e16ef1d30c484eabc1d21b22c6d1f505/src/XamlMath.Shared/Boxes/HorizontalBox.cs#L50-L59

But Children.Count is necessarily at least 1. Hence when the first child is added, the Math.Max first argument value is always 0 (initial value). This may cause an issue if the second argument is less than 0.

ForNeVeR commented 1 year ago

I'm not sure I get it.

The code is really weird, but, since we know this.Children.Count is never 0, let's clean these branches up and discuss the resulting version:

this.childBoxesTotalWidth += box.Width;
this.Width = Math.Max(this.Width, this.childBoxesTotalWidth);
this.Height = Math.Max(this.Height, box.Height - box.Shift);
this.Depth = Math.Max(this.Depth, box.Depth + box.Shift);
this.Italic = Math.Max(this.Italic, box.Italic);

Does this look incorrect? If so, then in what cases?

This may cause an issue if the second argument is less than 0.

First of all, is this even possible? I guess in case of incorrect metrics, it may happen, but such values will just coerce to 0, right?

Orace commented 1 year ago

Does this look incorrect? If so, then in what cases? First of all, is this even possible? I guess in case of incorrect metrics, it may happen, but such values will just coerce to 0, right?

The code looks good to me and I share your analysis, however I was afraid of cases where negative numbers could appear and I needed a second opinion 🙂