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

Fixes #3273. Updates FindDeepestView to support Adornment #3278

Closed
wants to merge 0 commits into from

Conversation

tig
Copy link
Collaborator

@tig tig commented Feb 28, 2024

Fixes

Is dependent on

See also:

Proposed Changes/Todos

  • Modify FindDeepestView to return Margin, Padding, or Border if x/y match.

  • Add MouseEvent handling code to Adornment, Margin, Border, and Padding as appropriate (e.g. Adornment.MouseClick += Parent.SetFocus()).

  • Add more FindDeepestView unit tests to prove this change works.

  • Move drag support out of Toplevel into Adornment - Dragging margin or border drags the view.

  • Drag support is not quite right - dragging anywhere on border.Thickness SHOULD work, but only top does

  • Add support for Adornment.SubViews

  • Computed Layout for Adornment.SubViews is not working right always.

    e.g. var btnButtonInPadding = new Button { X = Pos.Center (), Y = Pos.Center (), Text = "_Button in Padding" }; in Adornments scenario doesn't work (the Y =).

  • Figure out best place for adornments to add their own subviews. Can't do it in constructor or Initialized event.

  • Add a prototype Close button to Border - partially implemented.

  • Focus on a Adornment.SubView is not working.

  • When a subview of an Adornment gets focus, Adornment.Parent should get focus.

  • Adjusting Adornment.Thickness needs to re layout subviews

  • Positive Thicknesses in Adornment.Subviews is broken

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@tig
Copy link
Collaborator Author

tig commented Mar 2, 2024

@BDisp please check this out.

Adornment now handles Toplevel drag. Eventually, when

is completed, any View will support move/sizing. For now, I've hacked in at test for Parent is not Toplevel.

Copy link
Collaborator

@dodexahedron dodexahedron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙂

@BDisp
Copy link
Collaborator

BDisp commented Mar 4, 2024

I need the missing the out parameters. Now I can't have the subviews on adornmemts working anymore. I hope you fix that before the dragging feature.

@tig
Copy link
Collaborator Author

tig commented Mar 4, 2024

I need the missing the out parameters. Now I can't have the subviews on adornmemts working anymore. I hope you fix that before the dragging feature.

I don't recall what you mean by "the out parameters." Please remind me.

@BDisp
Copy link
Collaborator

BDisp commented Mar 4, 2024

I don't recall what you mean by "the out parameters." Please remind me.

Surely. When you refactored FindDeepestView to support adornments, you got rid of the output parameters, rx and ry. I used these values returned by the adornment to recall FindDeepestView and get the adornment's subview. Without these output parameters I am no longer able to do this.
Imagine you have a subview inside a superview that contains border. The subview has the Frame.Location as (0,0) and Padding.Thickness as (0,0,1,0). Padding has a subview like (0,0,Dim.Fill(),Dim.Fill). If the mouse is at position (0,0) of the Padding subview, FindDeepestView returns the correct padding, but when I call FindDeepestView again to return its subview it returns null because I'm not calling it with the correct location that was previously obtained with the return of the output parameters. FrameToScreen returns (1,1) or (1,2) if there is a MenuBar and it becomes more difficult to get the values to be sent to return the correct subview.

@tig
Copy link
Collaborator Author

tig commented Mar 4, 2024

I don't recall what you mean by "the out parameters." Please remind me.

Surely. When you refactored FindDeepestView to support adornments, you got rid of the output parameters, rx and ry. I used these values returned by the adornment to recall FindDeepestView and get the adornment's subview. Without these output parameters I am no longer able to do this. Imagine you have a subview inside a superview that contains border. The subview has the Frame.Location as (0,0) and Padding.Thickness as (0,0,1,0). Padding has a subview like (0,0,Dim.Fill(),Dim.Fill). If the mouse is at position (0,0) of the Padding subview, FindDeepestView returns the correct padding, but when I call FindDeepestView again to return its subview it returns null because I'm not calling it with the correct location that was previously obtained with the return of the output parameters. FrameToScreen returns (1,1) or (1,2) if there is a MenuBar and it becomes more difficult to get the values to be sent to return the correct subview.

Right.

What you're needing, and what I'll get you asap, is for FindDeepestView to recurse in to padding.SubViews here:

@BDisp
Copy link
Collaborator

BDisp commented Mar 4, 2024

Right.

What you're needing, and what I'll get you asap, is for FindDeepestView to recurse in to padding.SubViews here:

Yes I know. I already I'm using it but but with no success for now. Thanks.

@tig
Copy link
Collaborator Author

tig commented Mar 4, 2024

Right.
What you're needing, and what I'll get you asap, is for FindDeepestView to recurse in to padding.SubViews here:

Yes I know. I already I'm using it but but with no success for now. Thanks.

What did you try?

Have you merged this PR into yours?

Or, are you just trying an experiment on a fork of this PR?

@BDisp
Copy link
Collaborator

BDisp commented Mar 4, 2024

What did you try?

Have you merged this PR into yours?

No, I just trying to merge the v2_develop branch into my PR #3254 and using some code you are using on this PR but I cannot make it work as before, because I can't send the right coordinates to the Padding's subview on the MouseEvent.

Or, are you just trying an experiment on a fork of this PR?

No, because none of yours WIP PR or the already merged one handles subviews into adornments. It's working fine to deal with adornments but not with subviews in adornments.

@tig
Copy link
Collaborator Author

tig commented Mar 4, 2024

What did you try?
Have you merged this PR into yours?

No, I just trying to merge the v2_develop branch into my PR #3254 and using some code you are using on this PR but I cannot make it work as before, because I can't send the right coordinates to the Padding's subview on the MouseEvent.

Or, are you just trying an experiment on a fork of this PR?

No, because none of yours WIP PR or the already merged one handles subviews into adornments. It's working fine to deal with adornments but not with subviews in adornments.

Just to bubble up what's going on so we're on the same page:

@tig
Copy link
Collaborator Author

tig commented Mar 4, 2024

qKWtz57 1

@BDisp, you can now re-target #3254 to this branch!

You will likely need to revert a bunch of little things you did to ViewDrawing.cs, ViewLayout.cs, Adorrnment.cs and others as I've addressed the stuff you did in a more proper way.

I bet if you starrt with the Adornments scenario and start adding other views to Padding, you'll both be delighted and you'll find things that are still broken and need to be addressed.

@tig
Copy link
Collaborator Author

tig commented Mar 5, 2024

Fuck. I screwed something up trying to rebase. Arrrrrggg!!!

@tig
Copy link
Collaborator Author

tig commented Mar 5, 2024

@BDisp / @dodexahedron

If either of you pulled this branch down yesterday afternoon PLEASE DON'T DELETE IT.

I somehow completely clobbered this PR trying rebase and everything I did yesterday is GONE.

I have an older version at home, but it's missing everythign I did yesterday!

@BDisp
Copy link
Collaborator

BDisp commented Mar 5, 2024

Sorry @tig I didn't pull it. See if you have some stash you can recover from.

@tig
Copy link
Collaborator Author

tig commented Mar 5, 2024

Fuck. My branch on my home computer was last updated 3/1. That's 4 fraking days of REALLY GREAT (if I say so myself) work, GONE.

@tig
Copy link
Collaborator Author

tig commented Mar 5, 2024

@tig
Copy link
Collaborator Author

tig commented Mar 5, 2024

Back! Yay.

#3295

@dodexahedron
Copy link
Collaborator

Yay.

Yeah, your remote will always have whatever you last pushed to it until you force push over it. Handy in case you do something unfortunate or even if you simply want to try doing it another way, like maybe squashing or re-ordering a few commits.

Another option, however, that is much safer, is that, instead of force pushing over the original branch, change the remote branch name and push to an entirely new branch. That won't be a force push and you can then hold on to your old branch until you feel safe to just delete it altogether.

I generally don't pull anything from non-origin forks except explicitly to test or debug something. Although doing so is less dirty now than it was before fixing the merge strategy. When I do, though, I clone my working copy to a dedicated folder. While that's not really necessary, it is easier (read: lazier 😅) than pulling the remote fork branch to my repo and ensuring everything is properly committed/stashed/etc before that.

I also will sometimes copy my local repo if I'm about to do something I'm either unsure of or that risks losing work, so that rolling back the operation is as simple as deleting the working copy and copying the backup copy back to the original location. Then I don't have to think so much about how to undo something or what commit is proper to reset to or what kind of reset is correct, or whatever.

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

Successfully merging this pull request may close these issues.

3 participants