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

Constrain child windows to monitor bounds #8341

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Jul 20, 2020

If a parent window is at a corner of a desktop, and it is smaller than a child window, it is possible to end up in a situation where the child window is positioned outside of visible and reachable area.
Coupled with the fact that locations and dimensions of most windows get persisted this may lead to a situation where the window may become permanently inaccessible until the settings are reset.

Fixes #8091
Resolves #6427

Proposed changes

  • Constrain the window to be within the desktop bounds

Screenshots

Before

Steps to replicate:

  1. Open the app at the top left corner of the desktop
  2. Resize it to be smaller than a typical "Setting" dialog windows size
  3. Open "Settings" dialog
  4. Observe the dialog opened at negative coordinates, something like:
    image

Test methodology

  • Manual

NB: I have a single monitor and unable to test multi-monitor setup, where a secondary monitor is placed to the left or to the bottom of the main monitor. It is possible that the code need to be tweaked further to account for these cases.


✒️ I contribute this code under The Developer Certificate of Origin.

GitUI/GitExtensionsForm.cs Outdated Show resolved Hide resolved
@RussKie RussKie marked this pull request as draft October 18, 2020 23:23
@RussKie
Copy link
Member Author

RussKie commented Oct 19, 2020 via email

@RussKie
Copy link
Member Author

RussKie commented Oct 20, 2020

Incidentally I received my 2nd monitor today :D

@RussKie
Copy link
Member Author

RussKie commented Jan 14, 2021

I've spent an evening playing in a multi monitor setup, and here's what I propose.

  1. Iterate over user's monitors, and test three points of a given form: (L, T), ((L - R)/2, T), (R, T) to see whether any of the user's monitors contain any these points. This way we can find out which monitors a given form is present.
  2. If a point is displayed on a monitor test whether the following rectangle is fully displayed on this monitor: { p.X, p.Y, monitor.Width * 10%, monitor.Height * 10% } (inverted for the (R, T) point, and test each way for the middle point).
    This way we know that if the form isn't fully visible, it is visible sufficiently enough for the user to notice it, and be able to move, if necessary.
    NB: 10% is an arbitrary choice, can be more.
  3. If none of the three points and their offsets pass the check, then we move the form to main display at (0, 0).
LinqPad test script

For the test setup I have two 4K monitors:

  • primary 32" @ 100% below {X=0,Y=0,Width=3840,Height=2120}
  • secondary 27" @ 150% above {X=0,Y=-2160,Width=2467,Height=1440}

NB: the script doesn't currently take dpi into account

void Main()
{
	var screens = Screen.AllScreens.Select(screen => screen.WorkingArea).ToArray();

	int count = 0;
	foreach (Rectangle screen in screens)
	{
		Console.WriteLine($"Screen {++count}: {screen.ToString()}");
	}
	Console.WriteLine();

	Rectangle[] rects = new System.Drawing.Rectangle[]
	{
		// top left primary monitor
		new Rectangle(0, 0, 500, 500),
		// incorrect location
		new Rectangle(-100, -100, 500, 500),
		// creeps into the 2ndary screen
		new Rectangle(0, -730, 3840, 750),
		// shown on the 2ndary screen 
		new Rectangle(-100, -1730, 750, 750),
	};

	foreach (Rectangle rect in rects)
	{
		Console.WriteLine($"Testing rectangle: {rect.ToString()}\r\n---------------");
		var form = new Form();
		form.StartPosition = FormStartPosition.Manual;
		form.DesktopLocation = rect.Location;
		form.Size = rect.Size;
		form.Text = rect.ToString();
		form.Show();

		foreach (Rectangle screen in screens)
		{
			bool res = IsDisplayedOn10Percent(screen, rect, form);
			if (res)
			{
				Console.WriteLine($"Screen: {screen.ToString()}");
				form.BackColor = Color.Green;

				// no point testing other screens				
				Console.WriteLine();
				break;
			}
			else
			{
			}
		}

		if (form.BackColor != Color.Green)
		{
			form.BackColor = Color.Orange;
			form.DesktopLocation = Point.Empty;
		}

		Console.WriteLine();
	}
}

private bool IsDisplayedOn10Percent(Rectangle screen, Rectangle window, Form debugForm)
{
	if (screen.IsEmpty || window.IsEmpty) return false;

	int requiredHeight = (int)(screen.Height * 0.1);
	int requireWidth = (int)(screen.Width * 0.1);

	var label = new Label
	{
		AutoSize = false,
		Size = new Size(requireWidth, requiredHeight),
		BackColor = Color.Blue
	};

	Point p;
	if (screen.Contains(window.Location))
	{
		p = new Point(window.Left + requireWidth, window.Top + requiredHeight);
		bool leftTop = screen.Contains(p);
		if (leftTop)
		{
			Console.WriteLine($"{screen.ToString()} contains {p} (L, T)");
			label.Location = new Point(0, 0);
			debugForm.Controls.Add(label);
			return true;
		}
	}

	if (screen.Contains(new Point(window.Left + (window.Width / 2), window.Top)))
	{
		p = new Point(window.Left + (window.Width / 2) - requireWidth, window.Top + requiredHeight);
		bool middleTop = screen.Contains(p);
		if (middleTop)
		{
			Console.WriteLine($"{screen.ToString()} contains {p} (W/2-, T)");
			label.Location = new Point(p.X, 0);
			debugForm.Controls.Add(label);
			return true;
		}

		p = new Point(window.Left + (window.Width / 2) + requireWidth, window.Top + requiredHeight);
		middleTop = screen.Contains(p);
		if (middleTop)
		{
			Console.WriteLine($"{screen.ToString()} contains {p} (W/2+, T)");
			label.Location = new Point(window.Width / 2, 0);
			debugForm.Controls.Add(label);
			return true;
		}
	}

	if (screen.Contains(new Point(window.Right, window.Top)))
	{
		p = new Point(window.Right - requireWidth, window.Top + requiredHeight);
		bool rightTop = screen.Contains(p);
		if (rightTop)
		{
			Console.WriteLine($"{screen.ToString()} contains {p} (R, T)");
			label.Location = new Point(window.Width - requireWidth, 0);
			debugForm.Controls.Add(label);
			return true;
		}
	}

	return false;
}

@mstv
Copy link
Member

mstv commented Jan 14, 2021

👍 Sounds like a very good plan. I agree the title bar is most important, and left/right of the middle but not the exact middle covers the case where a window filled two screens and one of them was disconnected.
I think the 10% should be enough. Though they must be clipped to the - for ((L - R)/2, T) remaining - width and height of the window.

@RussKie
Copy link
Member Author

RussKie commented Jan 15, 2021

The primary screen 4K at 100% (bottom), the secondary screen 4K at 150% (top)

  • ✔️ new Rectangle(0, 0, 500, 500) - valid, the window is fully shown on a single monitor
    image
  • new Rectangle(-100, -100, 500, 500) - invalid as vertically is visible less than 10% on the screen that has the upper border of the windows caption bar
    image
  • new Rectangle(0, -730, 3840, 750) - invalid as vertically is visible less than 10% on the screen that has the upper border of the windows caption bar
    image
  • ✔️ new Rectangle(-100, -1730, 750, 750) - valid, visible 10% worth of the screen size in each direction starting from the middle point
    image

@RussKie
Copy link
Member Author

RussKie commented Jan 15, 2021

  • new Rectangle(-350, -1730, 750, 750) - valid, enough space to the right of the middle
    image

  • new Rectangle(-500, -1730, 750, 750) - valid, enough visible space from the right side
    image

@RussKie RussKie force-pushed the Constrain_child_windows_to_monitor_bounds branch from 910ff88 to af5ece4 Compare January 15, 2021 11:59
@RussKie RussKie force-pushed the Constrain_child_windows_to_monitor_bounds branch from af5ece4 to 8cae160 Compare January 23, 2021 08:26
@RussKie RussKie marked this pull request as ready for review January 23, 2021 11:37
@codecov
Copy link

codecov bot commented Jan 23, 2021

Codecov Report

Merging #8341 (bd40ebe) into master (80f96d5) will increase coverage by 0.02%.
The diff coverage is 92.98%.

@@            Coverage Diff             @@
##           master    #8341      +/-   ##
==========================================
+ Coverage   55.96%   55.98%   +0.02%     
==========================================
  Files         905      905              
  Lines       65273    65303      +30     
  Branches    11877    11884       +7     
==========================================
+ Hits        36530    36563      +33     
+ Misses      25805    25797       -8     
- Partials     2938     2943       +5     
Flag Coverage Δ
production 43.13% <94.44%> (+<0.01%) ⬆️
tests 94.47% <90.47%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@RussKie RussKie force-pushed the Constrain_child_windows_to_monitor_bounds branch from fdd4a74 to 47dfe55 Compare January 23, 2021 12:54
If a parent window is at a corner of a desktop, and it is smaller than
a child window, it is possible to end up in a situation where the child
window is positioned outside of visible and reachable area.
Coupled with the fact that locations and dimensions of most windows
get persisted this may lead to a situation where the window may become
permanently inaccessible until the settings are reset.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants