Finally, really, fixed clipping

[tig]

@BDisp and @migueldeicaza - Please do a code review of all this...

I started over by first reading every line (and comment) in View and Window. I re-wrote all the documentation to ensure I actually understood the intent. I found that either originally, or over time, the code had gotten confused on Frame vs. Bounds and view-relative, screen-relative, and superview-relative coordinates. I fixed all of that, renaming internals to be consistent and updating all XML documentation.

One of the most confusing parts was the internal API RectToScreen which is used extensively across the project. This API was mis-named (and the comment was not clear) and it was obvious people didn't know whether it took view-relative or super-view-relative coordinates. Some usages were RectToScreen (Bounds) and others were RectToScreen (Frame). I decided to rename this API to ViewToScreen and change the parameter name from rect to region. I also updated the comment:

		/// <summary>
		/// Converts a region in view-relative coordinates to screen-relative coordinates.
		/// </summary>
		internal Rect ViewToScreen (Rect region)

I fixed all usages across the project (sorry; touches lots of stuff and might make merging hard) to correctly call this using superview-relative coordinates (Frame). This fixed several visual bugs (and exposed others...see FileDialog).

Similarly, I noted that implementations of Redraw were confused on what coordinates were passed in the region parameter. I made it clear by changing the parameter name to bounds and fixing the docs:

		/// <summary>
		/// Redraws this view and its subviews; only redraws the views that have been flagged for a re-display.
		/// </summary>
		/// <param name="bounds">The view-relative region to redraw.</param>
		/// <remarks>
		/// <para>
		///    Views should set the color that they want to use on entry, as otherwise this will inherit
		///    the last color that was set globaly on the driver.
		/// </para>
		/// <para>
		///    Overrides of <see cref="Redraw"/> must ensure they do not set <c>Driver.Clip</c> to a clip region
		///    larger than the <c>region</c> parameter.
		/// </para>
		/// </remarks>
		public virtual void Redraw (Rect bounds)

Of course, almost every file in the project references Draw so that's why 21 files have changed with this PR.

I was confused by what the internal RelativeLayout API did. After studying the code I noted it SETS THE FRAME. So I renamed it to SetRelativeLayout and added better comments so future contributors aren't as confused as I was.

The key to fixing clipping was to utilize Rect.Intersects() to ensure the Driver.Clip that gets set is never larger than the previous clip. I did this in the SetClilp API to keep the code centralized:

/// <summary>
/// Sets the <see cref="ConsoleDriver"/>'s clip region to the current View's <see cref="Bounds"/>.
/// </summary>
/// <returns>The existing driver's clip region, which can be then re-eapplied by setting <c><see cref="Driver"/>.Clip</c> (<see cref="ConsoleDriver.Clip"/>).</returns>
/// <remarks>
/// <see cref="Bounds"/> is View-relative.
/// </remarks>
public Rect ClipToBounds ()
{
	return SetClip (Bounds);
}

/// <summary>
/// Sets the clip region to the specified view-relative region.
/// </summary>
/// <returns>The previous screen-relative clip region.</returns>
/// <param name="region">View-relative clip region.</param>
public Rect SetClip (Rect region)
{
	var previous = Driver.Clip;
	Driver.Clip = Rect.Intersect (previous, ViewToScreen (region));
	return previous;
}

Typical Redraw code now does this when calling Redraw on subviews (or contentViews):

// Clip the sub-view
var savedClip = ClipToBounds ();
view.Redraw (view.Bounds);
Driver.Clip = savedClip;

I expanded several Scenarios in UICatalog to help me test. And I added a clipping specific test scenario named Clipping.

[BDisp]

This is magnificent. Congratulations on your beautiful work. However, there is something that causes Application.Top not to be designed well and that should always be the first to design and lastly the active top-level (see bellow). I already noticed that you solved the clipping problem in the ScrollView way, which I think was the best option. A strange behavior that I see in the ScrollView of the Scrolling.cs scenario is that it does not respect the continuous pressed click to scroll faster and in the Example project this works well.

[migueldeicaza]

Whoa! Great patch! Will review tonight!

[tig]

I, personally, think it's silly that Window can be dragged. But this behavior is not due to clipping. It is due to the fact that Window is not calling NeedsDisplay on the windows it moves over as it's dragged.

[tig]

A strange behavior that I see in the ScrollView of the Scrolling.cs scenario is that it does not respect the continuous pressed click to scroll faster and in the Example project this works well.

This has nothing to do with clipping either. I was able to change the Scrolling Scenario to reproduce the correct behavior by adding a AddTimeout based progress bar to it. See the PR I just submitted.

[BDisp]

I, personally, think it's silly that Window can be dragged. But this behavior is not due to clipping. It is due to the fact that Window is not calling NeedsDisplay on the windows it moves over as it's dragged.

It's not totally silly and of course it has nothing to do with clipping. In Windows when sometimes when I receive a modal message box I need to drag it to see some content underneath before closing it. We are not talking about fixed windows like on the web, you can't move them. When I worked in dragging I always tested on the console with at least three TopLevel. The active modal, a smaller windows and Application.Top. The incorrect color overlapping sometimes happened and I had to deal with the design sequence from the inside to the outside.

[BDisp]

This has nothing to do with clipping either. I was able to change the Scrolling Scenario to reproduce the correct behavior by adding a AddTimeout based progress bar to it. See the PR I just submitted.

@tig this is really strange behavior because in Threadings.cs async / await is working very well by calling the public override void Post method (SendOrPostCallback d, object state) when it is already available for the Gui to manage the Task. Now the Task in MouseEvent ToDriverMouse (WindowsConsole.MouseEventRecord mouseEvent) is not triggering the above method. It only works with AddTimeout because it will perform everything that is pending (timers and idlehandlers). I don't know why the Post method is not called as in Threading.

[migueldeicaza]

Let us get this merged - Charlie, I am going to let you do the honors, given how large this patch is.

Longer version - I truly appreciate the work that went into this, the clearing up of the naming and conventions is clearly a problem that was going to keep biting us down the line as both time takes a toll on memory and new contributors bring controls to the project. I love this patch.

Regarding @BDisp comments on repainting - initially when Gui.cs was brought up, I did not really do much work to "queue" the damaged region on the parent view, and this might be just a side-effect of moving a window not sending the right setNeedDisplay to the container.

[tig]

Glad you like this. I've got a mondo PR coming that includes this one and the others; this will make merging easier and the history more clean.

Note, I already merged #590 into that mondo and it'll be a pain to back out. So i'd appreciate it if @migueldeicaza would look at #590. It's another fairly big set of changes.

[BDisp]

I, personally, think it's silly that Window can be dragged. But this behavior is not due to clipping. It is due to the fact that Window is not calling NeedsDisplay on the windows it moves over as it's dragged.

@tig the issue is in Window.cs in the MouseEvent method. You changed to Application.Top.Redraw (Bounds) but it must to be Application.Top.Redraw (Frame), otherwise it will only redraw from (0, 0, w, h) of the Toplevel.

				if (dragPosition.HasValue) {
					if (SuperView == null) {
						Application.Top.SetNeedsDisplay (Frame);
						Application.Top.Redraw (Frame);
					} else {
						SuperView.SetNeedsDisplay (Frame);
					}

[tig]

I, personally, think it's silly that Window can be dragged. But this behavior is not due to clipping. It is due to the fact that Window is not calling NeedsDisplay on the windows it moves over as it's dragged.

@tig the issue is in Window.cs in the MouseEvent method. You changed to Application.Top.Redraw (Bounds) but it must to be Application.Top.Redraw (Frame), otherwise it will only redraw from (0, 0, w, h) of the Toplevel.

				if (dragPosition.HasValue) {
					if (SuperView == null) {
						Application.Top.SetNeedsDisplay (Frame);
						Application.Top.Redraw (Frame);
					} else {
						SuperView.SetNeedsDisplay (Frame);
					}

Good catch.

However, Redraw is defined to take view-relative coords, meaning the coords that are received will always be Rect(0,0, h, w).

As noted in above, people have confused Frame (SuperView-relative) and Bounds (View-Relative) throughout this project. This is why I renamed the parameter to DrawFrame to be DrawFrame(Rect **bounds**).

ALWAYS CALL Redraw with Bounds. Never Frame.

Based on you identifying this call chain, I found that the REAL cause of this dragging bug is in Toplevel.Redraw():

///<inheritdoc cref="Redraw"/>
public override void Redraw (Rect bounds)
{
	Application.CurrentView = this;

	if (IsCurrentTop || this == Application.Top) {
		if (NeedDisplay != null && !NeedDisplay.IsEmpty) {
			Driver.SetAttribute (Colors.TopLevel.Normal);
			Clear (frame);
			Driver.SetAttribute (Colors.Base.Normal);
		}
		foreach (var view in Subviews) {
			if (view.Frame.IntersectsWith (bounds)) {
				view.SetNeedsLayout ();
				view.SetNeedsDisplay (view.Bounds);
			}
		}

		ClearNeedsDisplay ();
	}

	base.Redraw (base.Bounds);
}

Note View.Clear is defined as:

/// <summary>
///   Clears the specified region with the current color. 
/// </summary>
/// <remarks>
/// </remarks>
/// <param name="regionScreen">The screen-relative region to clear.</param>
public void Clear (Rect regionScreen)
{
	var h = regionScreen.Height;
	var w = regionScreen.Width;
	for (int line = regionScreen.Y; line < regionScreen.Y + h; line++) {
		Driver.Move (regionScreen.X, line);
		for (int col = 0; col < w; col++)
			Driver.AddRune (' ');
	}
}

Clear takes screen-relative coords. Since in this call chain the View in question is THE Top level, it's Bounds are 0, 0, w, h. So Clear (bounds) should be Clear(Frame).

I'll fix this before I submit again.

[BDisp]

@tig please see my comment here #601 (comment)

[tig]

This was incorporated in #600. Closing.