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

Views - View.Height += 1 does not have the expected behavior #1291

Closed
jmperricone opened this issue May 8, 2021 · 8 comments
Closed

Views - View.Height += 1 does not have the expected behavior #1291

jmperricone opened this issue May 8, 2021 · 8 comments

Comments

@jmperricone
Copy link
Contributor

jmperricone commented May 8, 2021

Before I use view.Height = view.Subviews.Count;, I had it in another way where I did view.Height += 1;
I remove a lot of other views with Dim and Pos to other views, thinking maybe was circular reference or something.
But no, the problem is just that line .... view.Height += 1;

If you try this, it gets exponentially slower.

This is beacuse Dim.Combine. After 3 times,
view.Height == {Dim.Combine(Dim.Combine(Dim.Combine(Dim.Absolute(0)+Dim.Absolute(1))+Dim.Absolute(1))+Dim.Absolute(1))}

Is this the expected behavior for this case ???

Maybe if it's the same instance, combine should behave differently, because I think that view.Height += x is something very basic to have such a behavior.

public override void Setup ()
{
  var view = new View () { X = 0, Y = 0, Width = 20, Height = 0 };
  var field = new TextField () { X = 0, Y = Pos.Bottom (view), Width = 20 };
  
  field.KeyDown += (k) => {
	  if (k.KeyEvent.Key == Key.Enter) {
  
		  var label = new Label (field.Text) { X = 0, Y = view.Subviews.Count, Width = 20 };
		  
                  field.Text = "";
  
		  view.Add (label);
  
                  // works
		  view.Height = view.Subviews.Count;
                  
                  // don't try this at home
		  // view.Height += 1;
	  }
  };
  
  Win.Add (view);
  Win.Add (field);
}
@jmperricone
Copy link
Contributor Author

plus1

@jmperricone jmperricone changed the title View.Height += 1 Views - View.Height += 1 does not have the expected behavior May 8, 2021
@BDisp
Copy link
Collaborator

BDisp commented May 8, 2021

That is the way that Pos and Dim works, but why don't do that?

view.Height = view.Bounds.Height + 1;

@BDisp
Copy link
Collaborator

BDisp commented May 8, 2021

Maybe if it's the same instance, combine should behave differently, because I think that view.Height += x is something very basic to have such a behavior.

I agree. Do you have some solution for this?

@BDisp
Copy link
Collaborator

BDisp commented May 8, 2021

I already found the bug and will submit a PR, unless you already have your own fix, which if it is the case please then say anything.

@BDisp
Copy link
Collaborator

BDisp commented May 8, 2021

I think it's here.

			void CollectPos (Pos pos, View from, ref HashSet<View> nNodes, ref HashSet<(View, View)> nEdges)
			{
				if (pos is Pos.PosView pv) {
					if (pv.Target != this) {
						nEdges.Add ((pv.Target, from));
					}
					foreach (var v in from.InternalSubviews) {
						CollectAll (v, ref nNodes, ref nEdges);
					}
					return;
				}
				if (pos is Pos.PosCombine pc) {
					foreach (var v in from.InternalSubviews) {
						CollectPos (pc.left, from, ref nNodes, ref nEdges);
						CollectPos (pc.right, from, ref nNodes, ref nEdges);
					}
				}
			}

and I changed to:

			void CollectPos (Pos pos, View from, ref HashSet<View> nNodes, ref HashSet<(View, View)> nEdges)
			{
				if (pos is Pos.PosView pv) {
					if (pv.Target != this) {
						nEdges.Add ((pv.Target, from));
					}
					foreach (var v in from.InternalSubviews) {
						CollectAll (v, ref nNodes, ref nEdges);
					}
					return;
				}
				if (pos is Pos.PosCombine pc) {
					foreach (var v in from.InternalSubviews) {
						CollectPos (pc.left, v, ref nNodes, ref nEdges);
						CollectPos (pc.right, v, ref nNodes, ref nEdges);
					}
				}
			}

But now I get error while running the tests here:

		[Fact]
		public void PosCombine_Do_Not_Throws ()
		{
			Application.Init (new FakeDriver (), new FakeMainLoop (() => FakeConsole.ReadKey (true)));

			var t = Application.Top;

			var w = new Window ("w") {
				X = Pos.Left (t) + 2,
				Y = Pos.Top (t) + 2
			};
			var f = new FrameView ("f");
			var v1 = new View ("v1") {
				X = Pos.Left (w) + 2,
				Y = Pos.Top (w) + 2
			};
			var v2 = new View ("v2") {
				X = Pos.Left (v1) + 2,
				Y = Pos.Top (v1) + 2
			};

			f.Add (v1, v2);
			w.Add (f);
			t.Add (w);

			f.X = Pos.X (t) + Pos.X (v2) - Pos.X (v1);
			f.Y = Pos.Y (t) + Pos.Y (v2) - Pos.Y (v1);

			t.Ready += () => {
				Assert.Equal (0, t.Frame.X);
				Assert.Equal (0, t.Frame.Y);
				Assert.Equal (2, w.Frame.X);
				Assert.Equal (2, w.Frame.Y);
				Assert.Equal (2, f.Frame.X);
				Assert.Equal (2, f.Frame.Y);
				Assert.Equal (4, v1.Frame.X);
				Assert.Equal (4, v1.Frame.Y);
				Assert.Equal (6, v2.Frame.X);
				Assert.Equal (6, v2.Frame.Y);
			};

			Application.Iteration += () => Application.RequestStop ();

			Application.Run ();
			Application.Shutdown ();
		}

Can we try to come to some conclusion together?

@BDisp
Copy link
Collaborator

BDisp commented May 8, 2021

I also have errors in the following scenarios:

Dialogs
MessageBoxes
Progress

@BDisp
Copy link
Collaborator

BDisp commented May 8, 2021

Well, the problem is nothing related to it. It is only related to Pos/Dim operators. I'll fix it.

BDisp added a commit to BDisp/Terminal.Gui that referenced this issue May 8, 2021
@jmperricone
Copy link
Contributor Author

@BDisp Sorry I couldn't help you.

That is the way that Pos and Dim works, but why don't do that?

view.Height = view.Bounds.Height + 1;

Yes, there are many ways to do this, but I think that operators should work as expected.

@tig tig closed this as completed in a492a52 May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants