Partially Fixes #2432 - Dim.Auto automatically sizes views based on Text - Replaces View.AutoSize

[tig]

(this is a new PR as I goofed up the other one)

Fixes

• Partially Fixes Finish Dim.Auto - DimAutoStyle.Content is half-baked #2432:
  • Fully implements DimAutoStyle.Text
  • Replaces View.AutoSize (and removes it)
  • Does NOT fully implement DimAutoStyle.Subviews (will be a future PR).

Adds Dim.Auto to Computed Layout to automatically size views based on Text

• Initial WIP and Scenario to demonstrate concept
• WIP: Support auto-size based on subviews
• Figure out how to make CheckDimAuto deal with PosCombine
• Re-investigate the concept of Dim.Auto (max: n) and/or Dim.Auto (max: dim)
  • Without this, AnchorEnd doesn't work, which is really sad.
• Merge the fix to
  • Improve correctness / clarity of existing View.AutoSize functionality/unit tests  #3192
• Support auto-size based on Text
• Implemement Dim.Auto (min: dim)
• DimAutoStyle.Text works
  • View.AutoSize now just uses Dim.Auto.
  • Ensure vertical text works
  • ObsoleteAttribute addeed
  • No internal View base class code uses AutoSize
• Remove View.AutoSize and update all code to use Dim.AutoSize instead
• Enough Unit tests
• API docs

For Future PRs

• Fully implement/test DimAutoStyle.Subviews
• Implemement Dim.AutoSize (max: dim)
• Use Dialog and MessageBox as test cases - This will require moving the dialog buttons to Padding as adornments.
• Dive deep into all other PosDim objects and get clarity on behavior relative to Dim.Auto - Improve unit tests to validate
  • Dim.Absolute
  • Dim.Auto
  • Dim.Fill
  • Dim.Function
  • Dim.Width/Height
  • Pos.Absolute/At
  • Pos.AnchorEnd
  • Pos.Function
  • Pos.Anchor
  • Pos.Percent
  • Pos.Center
  • Pos.X/Y etc...

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]

@BDisp I can't figure out what I merged incorrectly, but the cursor is now showing by default on all views.

Can you tell where I screwed up?

[BDisp]

@BDisp I can't figure out what I merged incorrectly, but the cursor is now showing by default on all views.

Can you tell where I screwed up?

It seems that you already fix it.

[tig]

@BDisp I can't figure out what I merged incorrectly, but the cursor is now showing by default on all views.
Can you tell where I screwed up?

It seems that you already fix it.

Yep. I think.

What do you think of this new API doc for PositionCursor?

    /// <summary>
    ///     Positions the cursor in the right position based on the currently focused view in the chain.
    /// </summary>
    /// <remarks>
    ///     <para>
    ///         Views that are focusable and want the cursor visible should override <see cref="PositionCursor"/>,
    ///         use <see cref="Move"/> to place the cursor in a location that makes sense, use
    ///         <see cref="ConsoleDriver.SetCursorVisibility"/>
    ///         to make the cursor visible, and return the position where the cursor was placed.
    ///     </para>
    ///     <para>
    ///         Unix terminals do not have  a way of hiding the cursor, so it can be distracting to have the cursor left at
    ///         the last focused view. Views should make sure that they place the cursor in a visually sensible place.
    ///     </para>
    /// </remarks>
    /// <returns>Viewport-relative cursor position. Return <see langword="null"/> to ensure the cursor is not visible.</returns>
    public virtual Point? PositionCursor ()
    {
        if (IsInitialized && CanFocus && HasFocus && ContentSize.HasValue)
        {
            // Base class will position the cursor at the end of the text.
            Point location = Viewport.Location;
            location.X = TextFormatter.HotKeyPos == -1 ? 0 : TextFormatter.CursorPosition;
            location.Y = 0;
            Move (location.X, location.Y);
        }

        // Returning null will hide the cursor.
        return null;
    }

[BDisp]

    ///     <para>
    ///         Unix terminals do not have  a way of hiding the cursor, so it can be distracting to have the cursor left at
    ///         the last focused view. Views should make sure that they place the cursor in a visually sensible place.
    ///     </para>

I don't quite understand this. In Unix it is also possible to hide the cursor through ANSI escape sequence. In WSL with version v2 the cursor only blinks if we move the mouse. A view that wants to show the cursor must also call the SetCursorVisibility method to define the cursor type.

[BDisp]

@tig this change is enough in CursesDriver.

    public override void UpdateCursor ()
    {
        EnsureCursorVisibility ();

        if (!RunningUnitTests && Col >= 0 && Col < Cols && Row >= 0 && Row < Rows)
        {
            Curses.move (Row, Col);
            Curses.raw ();
            Curses.noecho ();
            Curses.refresh ();
        }
    }

[BDisp]

The only difference is that the WindowsDriver immediately assumes the type of cursor that is configured in the WT and the CursesDriver initially uses the type of underline cursor in the TextField and after selecting the TextView the application assumes the cursor configured in the WT.

[tig]

    ///     <para>
    ///         Unix terminals do not have  a way of hiding the cursor, so it can be distracting to have the cursor left at
    ///         the last focused view. Views should make sure that they place the cursor in a visually sensible place.
    ///     </para>

I don't quite understand this. In Unix it is also possible to hide the cursor through ANSI escape sequence. In WSL with version v2 the cursor only blinks if we move the mouse. A view that wants to show the cursor must also call the SetCursorVisibility method to define the cursor type.

That comment has been there since Miguel wrote the code. It must have been true at some point.

[dodexahedron]

For that PositionCursor() method:

Do note that use of Point? means construction of a Nullable<Point> wrapper, which means a copy will be made of the Point, plus the cost of the rest of that struct, even though you don't see it, plus the cost of null being construction of a Nullable<Point> anyway, just with HasValue == false.

To avoid the cost of a wrapper, implementing the TryX pattern is a good option (whether named with Try or not), and then make the argument either ref or out, but not nullable.

[dodexahedron]

And, as with all virtuals, be sure to consider what any internal usages of it may mean if it is overridden by a descendent type, as the most-derived is used no matter if it's called from this type or descendants, with the exception of calls to base.SomeVirtual of course.

Code in the base declaring type can't make assumptions based on its implementation of a virtual.

Well... I mean... It can... But any such assumptions need to be loudly documented on the virtual as well as whatever internally uses it that is making those assumptions.

A good general practice in a library exposing overridable stuff on types is to define them as either virtual or, even better, as abstract, on a base class which has little to no real dependency on the specifics of the implementation, and then override them in inheriting types' implementations (possibly even sealing at that point, if prudent) and/or to provide a relevant interface, with or without a default implementation.

Interfaces are often better for that, due to greater flexibility with co-/contra- variance.

[dodexahedron]

This part is also a lot of copying that can all be eliminated:

Point location = Viewport.Location; // Value copy of Viewport.Location (avoidable) with potential heap access for Viewport (avoidable)
location.X = TextFormatter.HotKeyPos == -1 ? 0 : TextFormatter.CursorPosition; // Construction of new Point with X mutated (avoidable), plus the possible heap access for TextFormatter (unavoidable)
location.Y = 0; // Construction of new Point with Y mutated (avoidable)

Better to use a single mutation like:

Point location = Viewport.Location with { X = TextFormatter.HotKeyPos == -1 ? 0 : TextFormatter.CursorPosition, Y = 0 };

or, what the compiler will probably tell you to do instead, since all properties are being set, and which avoids bothering with Viewport, which seems unneeded altogether here:

Point location = new (TextFormatter.HotKeyPos == -1 ? 0 : TextFormatter.CursorPosition, 0);

[dodexahedron]

Also, with Y unconditionally set to 0, is a Point struct really even needed for that method?

It'll get created, then copied again for the call to Move, and then whatever happens in Move as well after that.

And that copy is avoidable, too, even if still using a point, by making it an out or ref parameter and having the caller supply the reference.

ref assignment takes that another step further, but isn't really necessary for that struct and the restrictions ref assignment brings with it, so just a plain old out or ref parameter is great.

But it seems to me that that method really only needs to bother returning (or assigning to an out or ref parameter) an int offset to be used by the caller.

Remember everything is pass and return by value - always meaning a copy of the value type or the value of the reference for a reference type - implicitly and unconditionally. ref/out still means a copy of a reference getting copied, but that's the minimum possible without inlining.

[dodexahedron]

@tig this change is enough in CursesDriver.

    public override void UpdateCursor ()
    {
        EnsureCursorVisibility ();

        if (!RunningUnitTests && Col >= 0 && Col < Cols && Row >= 0 && Row < Rows)
        {
            Curses.move (Row, Col);
            Curses.raw ();
            Curses.noecho ();
            Curses.refresh ();
        }
    }

I'd strongly suggest not ever having anything unit test related in compiled output in any situation other than unit tests.

For this, it would mean defining a compiler constant in the unit test project (that's useful for other things anyway), and then using a #if to skip the check if defined, rather than a runtime check on a boolean that shouldn't even be in consuming code and which could potentially interact with a consumer's unit test projects, too.

That also means there should almost definitely be an interface defined for this method instead of or in addition to how it is currently declared.

[BDisp]

Now it's working as expected. The views that wants to show the cursor must explicitly to call the SetCursorVisibility method. One solution is creating a property WantShowCursorPosition in the View class and letting it to manage it.
I think you should remove from the XML document about the Unix not supporting hiding the cursor.

[tig]

For that PositionCursor() method:

Do note that use of Point? means construction of a Nullable<Point> wrapper, which means a copy will be made of the Point, plus the cost of the rest of that struct, even though you don't see it, plus the cost of null being construction of a Nullable<Point> anyway, just with HasValue == false.

To avoid the cost of a wrapper, implementing the TryX pattern is a good option (whether named with Try or not), and then make the argument either ref or out, but not nullable.

FWIW, this is all temporary - This all needs a serious refactor per #3444

[tig]

@tig this change is enough in CursesDriver.

    public override void UpdateCursor ()
    {
        EnsureCursorVisibility ();

        if (!RunningUnitTests && Col >= 0 && Col < Cols && Row >= 0 && Row < Rows)
        {
            Curses.move (Row, Col);
            Curses.raw ();
            Curses.noecho ();
            Curses.refresh ();
        }
    }

I'd strongly suggest not ever having anything unit test related in compiled output in any situation other than unit tests.

For this, it would mean defining a compiler constant in the unit test project (that's useful for other things anyway), and then using a #if to skip the check if defined, rather than a runtime check on a boolean that shouldn't even be in consuming code and which could potentially interact with a consumer's unit test projects, too.

That also means there should almost definitely be an interface defined for this method instead of or in addition to how it is currently declared.

I agree. However, until these are fully addressed, we don't have a ton of choice here:

• UnitTests: Need unit tests that test the real drivers #1316
• Refactor ConsoleDriver to minimize duplicated code in all drivers #2319

[tig]

Now it's working as expected. The views that wants to show the cursor must explicitly to call the SetCursorVisibility method. One solution is creating a property WantShowCursorPosition in the View class and letting it to manage it. I think you should remove from the XML document about the Unix not supporting hiding the cursor.

I am currently working on finalizing this, in a temporary way, in this PR.

I am taking a short-term approach that partially addresses this:

• Re-factor Cursor handling per ./docfx/docs/cursor.md #3444

Specifically:

• Default cursor is invisible. For a view to enable the cursor it must do two things:
  • Set View.CursorStyle (public CursorVisibility CursorStyle {get;set} = CursorVisibility.Invisible) to Default (or something else).
  • Return something other than null from PositionCursor

I've found some more serious issues in how Focused and MostFocused work along the way. These will need to be addrssed in #3444 too.

@dodexahedron You once suggested a way to annotate unit tests that currently prove bad behavior so they can still run/pass but be identified as needing to be fixed later. Can you remind me of the technique? Thanks.

[tig]

@BDisp, regarding the cursor stuff in this PR:

• It all seems to work now
• Flickring is gone in ALL DRIVERS (including netdriver!).