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

Pos.Combine is incorrect for scenarios involving PosAbsolute #2358

Closed
tig opened this issue Feb 20, 2023 · 6 comments
Closed

Pos.Combine is incorrect for scenarios involving PosAbsolute #2358

tig opened this issue Feb 20, 2023 · 6 comments
Labels
breaking-change For PRs that introduces a breaking change (behavior or API) bug v2 For discussions, issues, etc... relavant for v2

Comments

@tig
Copy link
Collaborator

tig commented Feb 20, 2023

Change the ComputedScenario to add an additional button like this:

                       Win.Add (rightButton);
....
			centerButton = new Button ("0 + Center") {
				X = 0 + Pos.Center (),
				Y = Pos.AnchorEnd (2)
			};
			Win.Add (centerButton);

Expected:

The new button is centered in the view.

Actual:

image

The bug is in View.SetRelativeLayout. It needs to check for PosCombine and iterate through PosCombine.left and PosCombine.right to adjust the relative location properly.

I'm not actually sure it is possible to deal with all combinations. This may not be worth fixing.

Related:

  • What would you expect Pos.Center() + Pos.Center() to do?
  • What would you expect Pos.Percent() + Pos.Center() to do?
  • How about Pos.Percent(20) + Pos.Percent(20)? (this one's obvious: just like Pos.Percent(40)).
    etc...
@tig tig added the bug label Feb 20, 2023
@BDisp
Copy link
Collaborator

BDisp commented Feb 20, 2023

The bug is in View.SetRelativeLayout. It needs to check for PosCombine and iterate through PosCombine.left and PosCombine.right to adjust the relative location properly.

Right, the View.SetRelativeLayout have to send the PosCombine to the PosCombine class which will calculate the appropriate layout value.

I'm not actually sure it is possible to deal with all combinations. This may not be worth fixing.

Depends if it has any logic or not.

Related:

* What would you expect `Pos.Center() + Pos.Center()` to do?

My opinion is that it doesn't sense because is the same as using Pos.AnchorEnd (100/2=50+100/2=50=100) and also Pos.Center() - Pos.Center() is the same as using PosAbsolute equal to zero (100/2=50-100/2=50=0).

* What would you expect `Pos.Percent() + Pos.Center()` to do?

Without percent value it doesn't make sense (is the same as only using Pos.Center()), but with percent value it does make sense because it will calculate the percentage area and the will add more center value (eg. 100*20%=20+100/2=50=70).

* How about `Pos.Percent(20) + Pos.Percent(20)`? (this one's obvious: just like `Pos.Percent(40)`).

Right (10020%=20+10020%=20=40 so it's the same as 100*40%=40)

  etc...

Ask for more :-)

@tig
Copy link
Collaborator Author

tig commented Feb 20, 2023

Thanks. BTW, I have a refactor of all this in progress that fixes this an simplifies the code significantly. I believe I can make it work in a backwards compatible way.

@BDisp
Copy link
Collaborator

BDisp commented Feb 20, 2023

Thanks. BTW, I have a refactor of all this in progress that fixes this an simplifies the code significantly. I believe I can make it work in a backwards compatible way.

If is a backwards compatible merge it in the develop (v1) first and then merge it in the v2 while V2 is not released.

@tig
Copy link
Collaborator Author

tig commented Feb 21, 2023

Thanks. BTW, I have a refactor of all this in progress that fixes this an simplifies the code significantly. I believe I can make it work in a backwards compatible way.

@tig tig changed the title Pos.Combine is incorrect if left is PosAbsolute Pos.Combine is incorrect for scenarios involving PosAbsolute Feb 24, 2023
@tig tig changed the title Pos.Combine is incorrect for scenarios involving PosAbsolute BREAKING CHANGE - Pos.Combine is incorrect for scenarios involving PosAbsolute Feb 24, 2023
@tig tig added the breaking-change For PRs that introduces a breaking change (behavior or API) label Feb 24, 2023
@tig tig moved this to 📋 Backlog in Terminal.Gui V2 Beta Feb 28, 2023
@tig tig added the v2 For discussions, issues, etc... relavant for v2 label Feb 28, 2023
@tig tig moved this from 📋 Backlog to 🏗 In progress in Terminal.Gui V2 Beta Mar 2, 2023
@tig tig moved this from 🏗 In progress to 👀 In review in Terminal.Gui V2 Beta Mar 2, 2023
@tig tig changed the title BREAKING CHANGE - Pos.Combine is incorrect for scenarios involving PosAbsolute Pos.Combine is incorrect for scenarios involving PosAbsolute Mar 3, 2023
@tig
Copy link
Collaborator Author

tig commented Mar 3, 2023

Changed title since this is not such a big deal now that it's targeting v2.

tig added a commit that referenced this issue Mar 7, 2023
Fixes #2358 - BREAKING CHANGE: Pos.Combine is incorrect for scenarios involving PosAbsolute.
@BDisp
Copy link
Collaborator

BDisp commented Mar 7, 2023

Can this be closed as completed?

@tig tig closed this as completed Mar 7, 2023
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Terminal.Gui V2 Beta Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change For PRs that introduces a breaking change (behavior or API) bug v2 For discussions, issues, etc... relavant for v2
Projects
No open projects
Status: ✅ Done
2 participants