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

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

Merged
merged 217 commits into from
May 7, 2024

Conversation

tig
Copy link
Collaborator

@tig tig commented Apr 17, 2024

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

Fixes

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
  • 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
Copy link
Collaborator Author

tig commented May 6, 2024

@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?

urKSNw7 1

@BDisp
Copy link
Collaborator

BDisp commented May 6, 2024

@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
Copy link
Collaborator Author

tig commented May 6, 2024

@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
Copy link
Collaborator

BDisp commented May 6, 2024

    ///     <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
Copy link
Collaborator

BDisp commented May 6, 2024

@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
Copy link
Collaborator

BDisp commented May 6, 2024

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.

WindowsTerminal_RKmy2CuW5G

@tig
Copy link
Collaborator Author

tig commented May 6, 2024

    ///     <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
Copy link
Collaborator

dodexahedron commented May 7, 2024

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
Copy link
Collaborator

dodexahedron commented May 7, 2024

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
Copy link
Collaborator

dodexahedron commented May 7, 2024

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
Copy link
Collaborator

dodexahedron commented May 7, 2024

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
Copy link
Collaborator

dodexahedron commented May 7, 2024

@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
Copy link
Collaborator

BDisp commented May 7, 2024

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.

WindowsTerminal_5i7Hsdis8l

@tig
Copy link
Collaborator Author

tig commented May 7, 2024

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
Copy link
Collaborator Author

tig commented May 7, 2024

@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:

@tig
Copy link
Collaborator Author

tig commented May 7, 2024

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.

WindowsTerminal_5i7Hsdis8l WindowsTerminal_5i7Hsdis8l

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:

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
Copy link
Collaborator Author

tig commented May 7, 2024

@BDisp, regarding the cursor stuff in this PR:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants