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.

Sign up for GitHub

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 #2558. MenuBar positions wrong in some situations. #2567

Merged
merged 21 commits into from
May 5, 2023
Merged

Fixes #2558. MenuBar positions wrong in some situations. #2567

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
9ff50d3
Fixes #2558. MenuBar positions wrong in some situations.
BDisp Apr 20, 2023
82beaa1
Replacing Application.Top with Application.Current.
BDisp Apr 24, 2023
80410ba
Fix typo.
BDisp Apr 24, 2023
f759b27
Fix shortcut tag overlapping help on smaller width and add more unit …
BDisp Apr 24, 2023
fac62cc
Resizing the console will close all opened menus.
BDisp Apr 24, 2023
b362bb3
Resize first the console before show ContextMenu.
BDisp Apr 24, 2023
d984bcb
Merge branch 'v2_develop' into v2_menu-on-smaller-top-fix_2558
tig Apr 28, 2023
c6c9fc3
Merge branch 'v2_develop' into v2_menu-on-smaller-top-fix_2558
BDisp Apr 29, 2023
d975a57
Remove DriverFrame and DriverFrameOffset as not relevant.
BDisp Apr 29, 2023
bc0bf2f
Replace _frame with Frame as requested.
BDisp Apr 29, 2023
2d9b874
Fix xml document comment.
BDisp Apr 29, 2023
77f0b43
Compare equality between Dialog and Application.Top.
BDisp Apr 29, 2023
2816f67
Move GetDriverLocationOffset and GetDriverLocationOffsetFromCurrent t…
BDisp Apr 29, 2023
ee4a3ee
Merge branch 'v2_develop' into v2_menu-on-smaller-top-fix_2558
BDisp Apr 29, 2023
100d49e
Merge branch 'v2_develop' into v2_menu-on-smaller-top-fix_2558
BDisp May 4, 2023
4c74092
Resolving merge conflicts.
BDisp May 4, 2023
14338b7
Fix merge errors.
BDisp May 4, 2023
dd3a763
Ensure menu is closed on click.
BDisp May 4, 2023
3c267c3
Force Height always be 1 to avoid mouse events respond even outside b…
BDisp May 5, 2023
0f54fdd
Recovering UseSubMenusSingleFrame hope doesn't break again.
BDisp May 5, 2023
b97f318
Fix bugs and made requested changes.
BDisp May 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Terminal.Gui/Application.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1203,9 +1203,9 @@ static void OnUnGrabbedMouse (View view)

static void ProcessMouseEvent (MouseEvent me)
{
bool OutsideFrame (Point p, Rect r)
bool OutsideBounds (Point p, Rect r)
{
return p.X < 0 || p.X > r.Width - 1 || p.Y < 0 || p.Y > r.Height - 1;
return p.X < 0 || p.X > r.Right || p.Y < 0 || p.Y > r.Bottom;
}

if (IsMouseDisabled) {
Expand Down Expand Up @@ -1238,7 +1238,7 @@ bool OutsideFrame (Point p, Rect r)
OfY = me.Y - newxy.Y,
View = view
};
if (OutsideFrame (new Point (nme.X, nme.Y), _mouseGrabView.Frame)) {
if (OutsideBounds (new Point (nme.X, nme.Y), _mouseGrabView.Bounds)) {
_lastMouseOwnerView?.OnMouseLeave (me);
}
//System.Diagnostics.Debug.WriteLine ($"{nme.Flags};{nme.X};{nme.Y};{mouseGrabView}");
Expand Down
28 changes: 26 additions & 2 deletions Terminal.Gui/View/View.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,17 @@ public void EndInit ()
/// </summary>
public static ConsoleDriver Driver => Application.Driver;

/// <summary>
BDisp marked this conversation as resolved.
Show resolved Hide resolved
/// Gets the current driver <see cref="Rect"/> in use by the view.
/// </summary>
public static Rect DriverFrame => new (0, 0, Driver.Cols, Driver.Rows);

/// <summary>
/// Gets the current driver location and dimension in use by the view
/// offset by the <see cref="Application.Top"/> location and dimension.
BDisp marked this conversation as resolved.
Show resolved Hide resolved
/// </summary>
public static (Point Location, Point Dimension) DriverFrameOffset => new (new Point (Application.Top.Frame.X, Application.Top.Frame.Y), new (Application.Top.Frame.X + Application.Top.Frame.Width - Driver.Cols, Application.Top.Frame.Y + Application.Top.Frame.Height - Driver.Rows));

/// <summary>
/// Gets or sets arbitrary data for the view.
/// </summary>
Expand Down Expand Up @@ -434,7 +445,7 @@ public override bool Enabled {
}
}
}

/// <summary>
/// Event fired when the <see cref="Visible"/> value is being changed.
/// </summary>
Expand Down Expand Up @@ -481,7 +492,7 @@ bool CanBeVisible (View view)

return true;
}

/// <summary>
/// Pretty prints the View
/// </summary>
Expand All @@ -508,5 +519,18 @@ protected override void Dispose (bool disposing)
}
base.Dispose (disposing);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.Top is 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);
		}

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @tig. Understood. Tell me what you want I change in this PR.

/// <summary>
/// Gets the superview location offset relative to the <see cref="DriverFrame"/>.
/// </summary>
/// <param name="superView"></param>
/// <returns>The location offset and the respective superview.</returns>
public Point GetDriverLocationOffset (View superView)
{
var superViewFrame = superView == null || (superView == Application.Top && SuperView != superView) ? DriverFrame : superView.Frame;
var sv = superView == null ? Application.Top : superView;
return new Point (superViewFrame.X - sv.Frame.X - (superViewFrame.X != sv.Frame.X ? 1 : 0),
superViewFrame.Y - sv.Frame.Y - (superViewFrame.Y != sv.Frame.Y ? 1 : 0));
}
}
}
4 changes: 2 additions & 2 deletions Terminal.Gui/View/ViewLayout.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,7 @@ protected void ClearLayoutNeeded ()
public Point ScreenToView (int x, int y)
{
if (SuperView == null) {
return new Point (x - Frame.X, y - _frame.Y);
return new Point (x - _frame.X, y - _frame.Y);
BDisp marked this conversation as resolved.
Show resolved Hide resolved
} else {
var parent = SuperView.ScreenToView (x, y);
return new Point (parent.X - _frame.X, parent.Y - _frame.Y);
Expand All @@ -512,7 +512,7 @@ public Point ScreenToBounds (int x, int y)
{
if (SuperView == null) {
var boundsOffset = GetBoundsOffset ();
return new Point (x - Frame.X + boundsOffset.X, y - Frame.Y + boundsOffset.Y);
return new Point (x - _frame.X + boundsOffset.X, y - _frame.Y + boundsOffset.Y);
} else {
var parent = SuperView.ScreenToView (x, y);
return new Point (parent.X - _frame.X, parent.Y - _frame.Y);
Expand Down
6 changes: 3 additions & 3 deletions Terminal.Gui/Views/ContextMenu.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,10 @@ public void Show ()
container = Application.Top;
container.Closing += Container_Closing;
container.TerminalResized += Container_Resized;
var frame = container.Frame;
var frame = View.DriverFrame;
var position = Position;
if (Host != null) {
Host.ViewToScreen (container.Frame.X, container.Frame.Y, out int x, out int y);
Host.ViewToScreen (frame.X, frame.Y, out int x, out int y);
var pos = new Point (x, y);
pos.Y += Host.Frame.Height - 1;
if (position != pos) {
Expand All @@ -119,7 +119,7 @@ public void Show ()
if (Host == null) {
position.Y = frame.Bottom - rect.Height - 1;
} else {
Host.ViewToScreen (container.Frame.X, container.Frame.Y, out int x, out int y);
Host.ViewToScreen (frame.X, frame.Y, out int x, out int y);
var pos = new Point (x, y);
position.Y = pos.Y - rect.Height - 1;
}
Expand Down
54 changes: 38 additions & 16 deletions Terminal.Gui/Views/Menu.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,15 @@ public Menu (MenuBar host, int x, int y, MenuBarItem barItems, Menu parent = nul

private void Application_RootMouseEvent (MouseEvent me)
{
var view = View.FindDeepestView (this, me.X, me.Y, out int rx, out int ry);
if (me.View is MenuBar) {
return;
}
var locationOffset = GetDriverLocationOffset (null);
if (SuperView != null) {
locationOffset.X += SuperView.Border.Thickness.Left;
locationOffset.Y += SuperView.Border.Thickness.Top;
}
var view = View.FindDeepestView (this, me.X + locationOffset.X, me.Y + locationOffset.Y, out int rx, out int ry);
if (view == this) {
var nme = new MouseEvent () {
X = rx,
Expand Down Expand Up @@ -540,17 +548,21 @@ public override void Redraw (Rect bounds)
if (barItems.Children == null) {
return;
}
var savedClip = Application.Driver.Clip;
Application.Driver.Clip = Application.Top.Frame;
var savedClip = Driver.Clip;
Driver.Clip = DriverFrame;

Driver.SetAttribute (GetNormalColor ());

OnDrawFrames ();
OnRenderLineCanvas ();

for (int i = Bounds.Y; i < barItems.Children.Length; i++) {
if (i < 0)
if (i < 0) {
continue;
}
if (ViewToScreen (Bounds).Y + i >= Driver.Rows) {
break;
}
var item = barItems.Children [i];
Driver.SetAttribute (item == null ? GetNormalColor ()
: i == current ? ColorScheme.Focus : GetNormalColor ());
Expand Down Expand Up @@ -858,12 +870,17 @@ public override bool MouseEvent (MouseEvent me)
}
host.handled = false;
bool disabled;
var meYOffset = BorderStyle != LineStyle.None ? 1 : 0;
Point locationOffset = default;
if (SuperView != null) {
locationOffset.X += SuperView.Border.Thickness.Left;
locationOffset.Y += SuperView.Border.Thickness.Top;
}
var meYOffset = Border.Thickness.Top + locationOffset.Y;
var meY = me.Y - meYOffset;
if (me.Flags == MouseFlags.Button1Clicked) {
disabled = false;
if (me.Y < meYOffset)
if (meY < 0)
return true;
var meY = me.Y - meYOffset;
if (meY >= barItems.Children.Length)
return true;
var item = barItems.Children [meY];
Expand All @@ -878,14 +895,14 @@ public override bool MouseEvent (MouseEvent me)
me.Flags.HasFlag (MouseFlags.Button1Pressed | MouseFlags.ReportMousePosition)) {

disabled = false;
if (me.Y < meYOffset || me.Y - meYOffset >= barItems.Children.Length) {
if (meY < 0 || meY >= barItems.Children.Length) {
return true;
}
var item = barItems.Children [me.Y - meYOffset];
var item = barItems.Children [meY];
if (item == null) return true;
if (item == null || !item.IsEnabled ()) disabled = true;
if (item != null && !disabled)
current = me.Y - meYOffset;
current = meY;
if (host.UseSubMenusSingleFrame || !CheckSubMenu ()) {
SetNeedsDisplay ();
SetParentSetNeedsDisplay ();
Expand Down Expand Up @@ -1390,12 +1407,12 @@ internal void OpenMenu (int index, int sIndex = -1, MenuBarItem subMenu = null)
// text belonging to the menu
for (int i = 0; i < index; i++)
pos += Menus [i].TitleLength + (Menus [i].Help.ConsoleWidth > 0 ? Menus [i].Help.ConsoleWidth + 2 : 0) + leftPadding + rightPadding;

var superView = SuperView == null ? Application.Top : SuperView;
Point locationOffset;
if (superView.BorderStyle != LineStyle.None) {
locationOffset = new Point (superView.Frame.X + 1, superView.Frame.Y + 1);
} else {
locationOffset = new Point (superView.Frame.X, superView.Frame.Y);
var locationOffset = GetDriverLocationOffset (superView);
if (superView != Application.Top) {
locationOffset.X += superView.Border.Thickness.Left;
locationOffset.Y += superView.Border.Thickness.Top;
}
openMenu = new Menu (this, Frame.X + pos + locationOffset.X, Frame.Y + 1 + locationOffset.Y, Menus [index], null, MenusBorderStyle);
openCurrentMenu = openMenu;
Expand Down Expand Up @@ -1923,7 +1940,12 @@ public override bool MouseEvent (MouseEvent me)
(me.Flags == MouseFlags.ReportMousePosition && selected > -1) ||
(me.Flags.HasFlag (MouseFlags.Button1Pressed | MouseFlags.ReportMousePosition) && selected > -1)) {
int pos = xOrigin;
int cx = me.X;
Point locationOffset = default;
if (SuperView != null) {
locationOffset.X += SuperView.Border.Thickness.Left;
locationOffset.Y += SuperView.Border.Thickness.Top;
}
int cx = me.X - locationOffset.X;
for (int i = 0; i < Menus.Length; i++) {
if (cx >= pos && cx < pos + leftPadding + Menus [i].TitleLength + Menus [i].Help.ConsoleWidth + rightPadding) {
if (me.Flags == MouseFlags.Button1Clicked) {
Expand Down
Loading