Fixes #2558. MenuBar positions wrong in some situations. #2567
Conversation
|
@BDisp I've started reviewing this, and as I've read the code I've just gotten myself confused. I need to step back from all of the details and make sure I fully understand how we want menus to work. Let's riff on it here in the comments, please... Please correct anything I state below if I'm incorrect! What is "MenuBar"?
Where should
|
Actually we only can create
I already fixed that in this PR. Is the
I think you are confusing the
Of course it can, but not actually yet. Is a design and layout matter.
It was broken in the v2, but I already fixed. See the unit tests Terminal.Gui/UnitTests/Views/MenuTests.cs Lines 914 to 920 in f280ded
I don't agree because of the reasons I already explained above. Only the
But also supports a
But also can be used on a view, if needed.
It can be null if you right click on any location of a toplevel to open the
Where is that code? I can't find it. I think you are mean about
The reason for that is a user can prefer use this property as true on a
That right :-)
The reason for this property is to avoid each view having different key to activate the
Like each item of a
I already fixed this in this PR. A
That is only true if the
I think I also agree.
It's fine If they are both on the same superview, otherwise I'm not sure if the layout will work.
But we need the
I think the
If you are meaning that the
The
It's not a good idea to create them and hiding. They must be created dynamically as needed.
It's not very easy but I can try it. |
|
Menu is always aligned on resizing now.
|
|
BTW, all the refactoring stuff above doesn't seem very high priority relative to other v2 stuff. If you want to dive into something big, I'd suggest these: |
The refactoring for the
Thanks for remember me this. By the way, do you want I close the #2561? I think I prefer the approach of this PR. |
|
I'll be travelling internationally next few days, so I may or may not get a chance to dive deep into this PR again for a bit (depends on how travel goes and how wifi is ;-)). |
There was a problem hiding this comment.
@BDisp When I wrote the comments above, I didn't understand this PR. I think I now understand it better, but I'm still confused.
I'd like you to revisit DriverFrame and DriverFrameOffset. I think they are confusing and complex.
DriverFrame- does not need to be a
Rect. It is aSize. - should not be on
View. Why not just useDriver.ColsandDriver.Rowsdirectly? If you think a helper is needed, then put it onApplicationasApplication.Size.
- does not need to be a
DriverFrameOffset- I am not sure why this is needed. But assuming it is:
- This is a
Rect, but you instead return twoPoints. "Dimension" is, literally, aSize, not a point. LocationisApplication.Top.Frame.Location.DimensionisApplication.Top.FrameminusApplication.Size, which makes no sense to me.- The name "DriverFrameOffset" is confusing.
Can you help me understand all of this please?
They are really irrelevant. The correct solution of this are in the |
Terminal.Gui/View/View.cs
Outdated
| @@ -508,5 +508,30 @@ protected override void Dispose (bool disposing) | |||
| } | |||
| base.Dispose (disposing); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I still don't understand what these are for, nor do I think they are named correctly. My comments before still apply. I do not think these belong on View.
You've said several times "Remember that there are views that needed to draw outside the Application.Top".
I'm either not understanding what you mean, or I disagree.
I know v1 didn't work this way, but I think v2 should:
- No View should draw outside of it's Frame. Menus, popups, etc... that are created by a View should be bound by that view's Frame.
- The system (
Application) can draw anywhere and will do so in some cases (e.g. drawing drop-shadows).
If the view that is Application.Top is too small for content to fit within it's frame then the developer can make it bigger.
I may be missing some very obvious scenario that you are seeing clearly though. Please help me see it!
There was a problem hiding this comment.
I still don't understand what these are for, nor do I think they are named correctly. My comments before still apply. I do not think these belong on
View.
Ok, I'll move them to the Menu as private methods.
You've said several times "Remember that there are views that needed to draw outside the Application.Top".
I'm either not understanding what you mean, or I disagree.
I know v1 didn't work this way, but I think v2 should:
* No View should draw outside of it's Frame. Menus, popups, etc... that are created by a View should be bound by that view's Frame. * The system (`Application`) can draw anywhere and will do so in some cases (e.g. drawing drop-shadows).If the view that is
Application.Topis too small for content to fit within it's frame then the developer can make it bigger.
Well, sorry I disagree with you in this specific matter. The user may not know the size of a drop-down, a context menu or other popup. If you use WPF or WindowsForms it allows to draw outside the container of the control.
I may be missing some very obvious scenario that you are seeing clearly though. Please help me see it!
@tig this unit test in the ContextMenuTests explain perfectly what I' saying. It's a TextField on a smaller Dialog that is also the Application.Top and, of course, the Application.Current. As you can see the ContextMenu is also showing outside the Dialog bounds, over the non-full console.
[Fact, AutoInitShutdown]
public void Draw_A_ContextMenu_Over_A_Top_Dialog ()
{
((FakeDriver)Application.Driver).SetBufferSize (20, 15);
Assert.Equal (new Rect (0, 0, 20, 15), Application.Driver.Clip);
TestHelpers.AssertDriverContentsWithFrameAre (@"", output);
var dialog = new Dialog () { X = 2, Y = 2, Width = 15, Height = 4 };
dialog.Add (new TextField ("Test") { X = Pos.Center (), Width = 10 });
var rs = Application.Begin (dialog);
Assert.Equal (new Rect (2, 2, 15, 4), dialog.Frame);
Assert.Equal (dialog, Application.Top);
TestHelpers.AssertDriverContentsWithFrameAre (@"
┌─────────────┐
│ Test │
│ │
└─────────────┘", output);
ReflectionTools.InvokePrivate (
typeof (Application),
"ProcessMouseEvent",
new MouseEvent () {
X = 9,
Y = 3,
Flags = MouseFlags.Button3Clicked
});
var firstIteration = false;
Application.RunMainLoopIteration (ref rs, true, ref firstIteration);
TestHelpers.AssertDriverContentsWithFrameAre (@"
┌─────────────┐
│ Test │
┌───────────────────
│ Select All Ctrl+
│ Delete All Ctrl+
│ Copy Ctrl+
│ Cut Ctrl+
│ Paste Ctrl+
│ Undo Ctrl+
│ Redo Ctrl+
└───────────────────", output);
Application.End (rs);
}There was a problem hiding this comment.
But, a ContextMenu is just a Menu which is a View. The Dialog is not drawing out side of it's Frame. The Menu is. And that's ok because it is not a subview of the dialog.
There was a problem hiding this comment.
In this case it's a sub view of the Dialog because is added into the Dialog. See the ContextMenu code. In the case of the MenuBar it really is a Dialog subview and it only is draw inside the Dialog frame. Only the Menu can be draw outside there aren't enough space to accommodate it.
There was a problem hiding this comment.
Remember, in v2, 'MenuBar' will be a subview of 'view.Padding' (or maybe 'Border', I haven't decided yet).
'ContextMenu' should not be a View subclass at all. It is just a helper for causing 'Menu' objects to be shown without a 'MenuBar'.
Proposal to resolve this:
- Rule: a subview can never draw outside of its superview's Bounds
- If 'view.Modal == true'
- 'view.SuperView' must be 'null'.
- 'view.Parent' must not be null
- The view will be constrained within 'Parent.Frame'
- 'Menu' (the private class in Menu.cs) will always be created with 'Modal = true' and 'Parent = Application.Top'.
This solves both my desire to have a model where Views can't draw outside their superview.Bounds and the need for menus to sometimes escape.
Does this proposal sound reasonable to you?
There was a problem hiding this comment.
Does this proposal sound reasonable to you?
I think that fit the logic of handling float views. I only have a doubt when you referred Parent = Application.Top may be is better Parent = Application.Current. Imagine you have two toplevels opened and if the float view is managed by some view in the Current, which is modal, then must be this one the container of the float view, right?
There was a problem hiding this comment.
Does this proposal sound reasonable to you?
I think that fit the logic of handling float views. I only have a doubt when you referred
Parent = Application.Topmay be is betterParent = Application.Current. Imagine you have two toplevels opened and if the float view is managed by some view in theCurrent, which is modal, then must be this one the container of the float view, right?
Sorta. In v2 I think Application.Current goes away with the concept of fully supporting Overlapped. Any view can host Overlapped views (formerly called Mdi). There is only ONE Application.Top. It cannot be changed between Run/End.
However, ANY view can have overlapped subviews. ONE of those overalapped subviews can have the topmost "z-order", which is equivalent to "Current". It's also the same as "Most Focused" in Overlapped scenarios.
There was a problem hiding this comment.
Thanks @tig. Understood. Tell me what you want I change in this PR.
Fixes #2558 - Allows managing driver frame offset for smaller tops. @tig I think you'll prefer this PR instead of #2561.
Pull Request checklist:
CTRL-K-Dto automatically reformat your files before committing.dotnet testbefore commit///style comments)