Fix bug of arrange for WrapPanel #1217

OpportunityLiu · 2017-06-13T02:06:07Z

Use int as backing field of U and V, see #1085

Use `int` as backing field of `U` and `V`, see #1085

dnfclas · 2017-06-13T02:06:11Z

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

IbraheemOsama · 2017-06-13T02:29:32Z

Microsoft.Toolkit.Uwp.UI.Controls/WrapPanel/WrapPanel.Data.cs


-            internal double V { get; set; }
+            private int u, v;
+            internal double U { get { return u / FACTOR; } set { u = (int)(value * FACTOR); } }


I think this getter/setter could be replaced with Math.Floor ? I didn't try my suggestion but it seems you convert to integer then divide to get rid of decimal point to get the floor of the number

IbraheemOsama · 2017-06-13T02:33:09Z

Microsoft.Toolkit.Uwp.UI.Controls/WrapPanel/WrapPanel.Data.cs

+            internal const double FACTOR = 10000;

-            internal double V { get; set; }
+            private int u, v;


doubles are used for a reasons, you can't just drop the double and use int :)

Changed to double and Math.Floor, thanks for your advice.

IbraheemOsama · 2017-06-13T03:09:42Z

Microsoft.Toolkit.Uwp.UI.Controls/WrapPanel/WrapPanel.Data.cs


-            internal double V { get; set; }
+            private double u, v;
+            internal double U { get { return u / FACTOR; } set { u = Math.Floor(value * FACTOR); } }


Now you've done the floor you're no longer need the FACTOR :)

You means Math.Floor(value * FACTOR) / FACTOR ?

I mean
u=Math.Floor(value); directly

Why? Doing this will cut all fractions, which is used in some controls. We should keep several number of digits.

dnfclas · 2017-06-13T03:16:55Z

@OpportunityLiu, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

IbraheemOsama · 2017-06-13T03:29:48Z

Microsoft.Toolkit.Uwp.UI.Controls/WrapPanel/WrapPanel.Data.cs

-                U = 0.0;
-                V = 0.0;
-            }
+            internal double V { get { return v ; } set { v = Math.Floor(value * FACTOR) / FACTOR; } }


You don't need the FACTOR in this case mate :) right ? just do Math.Floor(value) with no multiplication or division.

what do you think ?

OpportunityLiu · 2017-06-13T03:39:53Z

FACTOR removed

IbraheemOsama · 2017-06-13T11:54:54Z

Microsoft.Toolkit.Uwp.UI.Controls/WrapPanel/WrapPanel.Data.cs

-            internal double U { get; set; }
-
-            internal double V { get; set; }
+            private double u, v;


Private variables must start with Underscore, _u, _v

IbraheemOsama · 2017-06-13T11:58:17Z

Microsoft.Toolkit.Uwp.UI.Controls/WrapPanel/WrapPanel.Data.cs

-
-            internal double V { get; set; }
+            private double u, v;
+            internal double U { get { return u; } set { u = Math.Floor(value); } }


There should be a space between private double u, v; and the next line.
Internal double U .... need to be on multiple lines.
The toolkit already has StyleCop file that contains everything regarding the recommended practices which should be highlighted in the file by default.

IbraheemOsama · 2017-06-13T11:58:32Z

Microsoft.Toolkit.Uwp.UI.Controls/WrapPanel/WrapPanel.Data.cs

-                U = 0.0;
-                V = 0.0;
-            }
+            internal double V { get { return v; } set { v = Math.Floor(value); } }


Same as above

IbraheemOsama · 2017-06-13T12:01:57Z

Microsoft.Toolkit.Uwp.UI.Controls/WrapPanel/WrapPanel.Data.cs

-            }
+            internal double V { get { return v; } set { v = Math.Floor(value); } }

            public UvMeasure(Orientation orientation, double width, double height)


The default constructor was used to reinitialize U and V values, the default constructor was used in the wrapPanel class multiple times. I know that you're using a struct which has no a default constructor so either you go back to class or create a function to reset the values to zeroes which should be used in the wrapPanel.cs class.

IbraheemOsama · 2017-06-13T12:02:46Z

Microsoft.Toolkit.Uwp.UI.Controls/WrapPanel/WrapPanel.Data.cs


            public UvMeasure(Orientation orientation, double width, double height)
            {
+                this.u = 0.0;


"this.u" is useless you can remove it and use "u" directly

IbraheemOsama · 2017-06-13T12:03:01Z

Microsoft.Toolkit.Uwp.UI.Controls/WrapPanel/WrapPanel.Data.cs

            public UvMeasure(Orientation orientation, double width, double height)
            {
+                this.u = 0.0;
+                this.v = 0.0;


same as above

OpportunityLiu · 2017-06-13T13:19:30Z

Microsoft.Toolkit.Uwp.UI.Controls/WrapPanel/WrapPanel.cs

            totalMeasure.V += lineMeasure.V;

+            totalMeasure.U = Math.Ceiling(totalMeasure.U);
+


Just use Ceiling here once, all problem solved.

How did I come out with that complex idea...

Fix bug of arrange

4680481

Use `int` as backing field of `U` and `V`, see #1085

dnfclas added the cla-not-required label Jun 13, 2017

Opportunity added 3 commits June 13, 2017 10:12

Create WrapPanel.Data.cs

0bfaa1c

Create WrapPanel.Data.cs

3a60e0b

init field

00b57a7

OpportunityLiu changed the title ~~Fix bug of arrange~~ Fix bug of arrange for WrapPanel Jun 13, 2017

IbraheemOsama reviewed Jun 13, 2017

View reviewed changes

Opportunity added 2 commits June 13, 2017 10:48

Uses Math.Floor

09078ad

Create WrapPanel.Data.cs

2cb0773

IbraheemOsama suggested changes Jun 13, 2017

View reviewed changes

dnfclas added the cla-signed label Jun 13, 2017

Update WrapPanel.Data.cs

ed9527b

IbraheemOsama suggested changes Jun 13, 2017

View reviewed changes

Update WrapPanel.Data.cs

4e44322

IbraheemOsama suggested changes Jun 13, 2017

View reviewed changes

IbraheemOsama reviewed Jun 13, 2017

View reviewed changes

IbraheemOsama suggested changes Jun 13, 2017

View reviewed changes

OpportunityLiu added 2 commits June 13, 2017 20:57

fix code style

f5b19f8

so stupid... just use ceiling rather than floor.

53f231c

OpportunityLiu commented Jun 13, 2017

View reviewed changes

IbraheemOsama approved these changes Jun 14, 2017

View reviewed changes

IbraheemOsama added bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️ labels Jun 14, 2017

nmetulev approved these changes Jun 19, 2017

View reviewed changes

nmetulev merged commit d0b1426 into CommunityToolkit:dev Jun 19, 2017

OpportunityLiu deleted the patch-1 branch June 27, 2017 04:09

		totalMeasure.V += lineMeasure.V;

		totalMeasure.U = Math.Ceiling(totalMeasure.U);

Fix bug of arrange for WrapPanel #1217

Fix bug of arrange for WrapPanel #1217

Uh oh!

Conversation

OpportunityLiu commented Jun 13, 2017

Uh oh!

dnfclas commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IbraheemOsama Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfclas commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OpportunityLiu commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

IbraheemOsama Jun 13, 2017 •

edited

Loading

OpportunityLiu commented Jun 13, 2017 •

edited

Loading