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

TextModel.ToRuneCellList is internal and is better move it to the public RuneCell class. #3774

Closed
BDisp opened this issue Oct 2, 2024 · 17 comments
Labels
Milestone

Comments

@BDisp
Copy link
Collaborator

BDisp commented Oct 2, 2024

          > Unfortunately TextModel is internal. But I can copy the content of ToRuneCellList()

It can be moved to the public RuneCell class where it can be more useful for others users views that want to use it.

Originally posted by @BDisp in #3773 (comment)

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 3, 2024

Moving this from #3773 (comment)

Digging a little more about this perhaps these static methods in TextModel could also be moved in RuneCell or changed/removed

  • StringToLinesOfRuneCells() (which could internally use ToRuneCellList()) moved in RuneCell
  • ToString (IEnumerable<RuneCell> cells) moved in RuneCell
  • StringToRuneCells() moved in RuneCell
  • ToRuneCells (IEnumerable<Rune> runes, ColorScheme? colorScheme = null) moved in RuneCell
  • SplitNewLines() (it's private but should be public) moved in RuneCell
  • ToRuneCells (List<RuneCell> cells) (ambigous name with another ToRuneCells ()) can be removed - use instead SplitNewLines()

@tig
Copy link
Collaborator

tig commented Oct 3, 2024

I don't think RuneCell is a great name. If we make it public we should spend time debating terminology. It was ok when it was internal.

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 3, 2024

I don't think RuneCell is a great name. If we make it public we should spend time debating terminology. It was ok when it was internal.

I agree. What about ColoredRune?

BDisp added a commit to BDisp/Terminal.Gui that referenced this issue Oct 3, 2024
@BDisp
Copy link
Collaborator Author

BDisp commented Oct 3, 2024

I don't think RuneCell is a great name. If we make it public we should spend time debating terminology. It was ok when it was internal.

I agree. What about ColoredRune?

@tznind or anyone interested can you please suggest a good name for RuneCell? I suggested ColoredRune.

@tznind
Copy link
Collaborator

tznind commented Oct 3, 2024

ColoredRune is fine.

Only others i thought of were:

ColorSchemedRune
RuneWithColorScheme
etc

And I think those are strictly worse

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 3, 2024

And I think those are strictly worse

I agree 😄 But thanks any way for your great help. If @tig agree with ColoredRune I'll do the necessary changes.

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 3, 2024

Add support for colored runes in the Editor scenario in bb5af01.

WindowsTerminal_tjjeHSHeDk

@tig
Copy link
Collaborator

tig commented Oct 3, 2024

Someday we'll have Bold, etc as attributes in addition to color.

Remind me again why this "cell" is different than 'Cell'?

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 3, 2024

Someday we'll have Bold, etc as attributes in addition to color.

Great.

Remind me again why this "cell" is different than 'Cell'?

I don't remember, sincerely. Maybe because I wanted to use ColorScheme instead of Attribute.

@tig
Copy link
Collaborator

tig commented Oct 4, 2024

Someday we'll have Bold, etc as attributes in addition to color.

Great.

Remind me again why this "cell" is different than 'Cell'?

I don't remember, sincerely. Maybe because I wanted to use ColorScheme instead of Attribute.

I encourage you to think through two things and be able to have clarity:

  1. How will this work once we support attributes other than color. We'll want TextView to support editing text with eg underline.

  2. 'Cell' will need to be enhanced the same way. How can we avoid duplicate code and implementation such that the work you do here can be leveraged.

Thanks.

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 4, 2024

I see what you mean. I'll use the Cell and move the methods to the Cell record. Then I'll remove the RuneCell class. Maybe it'll work. Thanks.

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 4, 2024

I see what you mean. I'll use the Cell and move the methods to the Cell record. Then I'll remove the RuneCell class. Maybe it'll work. Thanks.

@tig I really try and did all what I said and I end up with the bellow image.

WindowsTerminal_aF8H40SDeY

The reason to the unsuccess is because the RuneCell is very tied to the ColorScheme and manipulate the right attribute to apply to the view. Another issue is that the Cell is a struct which doesn't let assign a value when is iterating in a foreach block. The Cell object is used for the output on the screen and the RuneCell is used to iterate with the view draw which will set the necessary attribute for the current state of the view and if a RuneCell has a null ColorScheme the view ColorScheme will be used. I'll try to fix it but with no warranty.

@tznind
Copy link
Collaborator

tznind commented Oct 5, 2024

the Cell is a struct with doesn't let assign a value

Is there a reason we made it struct? Can't we just make it a class. It seems like there are many cases where you might want to be able to edit a Cell e.g. events etc.

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 5, 2024

Is there a reason we made it struct? Can't we just make it a class. It seems like there are many cases where you might want to be able to edit a Cell e.g. events etc.

I don't know why it's a struct but I think that because he already inherited IEquatable<T> interface. I already confirm that is possible to change his properties but through a variable and then assign back the whole object. Now, I'm facing an issue with xunit Assert.Equal because the GetHashCode gets different values with two Cell objects with the same properties values. Maybe I need to use the new() keyword.

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 5, 2024

I already found the issue. Although record struct Cell has properties it's needed to add them as parameter, at least on the mutable ones.

public record struct Cell (Attribute? Attribute = null, bool IsDirty = false, Rune Rune = default)
{

    /// <summary>The attributes to use when drawing the Glyph.</summary>
    public Attribute? Attribute { get; set; } = Attribute;

    /// <summary>
    ///     Gets or sets a value indicating whether this <see cref="T:Terminal.Gui.Cell"/> has been modified since the
    ///     last time it was drawn.
    /// </summary>
    public bool IsDirty { get; set; } = IsDirty;

    private Rune _rune = Rune;

    /// <summary>The character to display. If <see cref="Rune"/> is <see langword="null"/>, then <see cref="Rune"/> is ignored.</summary>
    public Rune Rune
    {
        get => _rune;
        set
        {
            CombiningMarks?.Clear ();
            _rune = value;
        }
    }

    /// <summary>
    ///     The combining marks for <see cref="Rune"/> that when combined makes this Cell a combining sequence. If
    ///     <see cref="CombiningMarks"/> empty, then <see cref="CombiningMarks"/> is ignored.
    /// </summary>
    /// <remarks>
    ///     Only valid in the rare case where <see cref="Rune"/> is a combining sequence that could not be normalized to a
    ///     single Rune.
    /// </remarks>
    internal List<Rune> CombiningMarks { get; } = [];

    /// <inheritdoc/>
    public override string ToString () { return $"[{Rune}, {Attribute}]"; }
}

Now the Assert.Equal pass even after changed the properties.

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 5, 2024

Final version. When Rune is changed the CombiningMarks.Clear (); throw a null exception because his value is always null no matter it has internal List<Rune> CombiningMarks { get; } = new ();. Since it's an internal property it can't be put in the parameters, but it could we can't initialize a List<T> in a parameter. So, with backing field it work, but we need to set the getter and setter as it's. Without these changes the Assert.Equal was always failing when the object is changed. Now, with the parameters when the the object is changed, the GetHashCode recalculate the right hash for the assert pass.

public record struct Cell (Attribute? Attribute = null, bool IsDirty = false, Rune Rune = default)
{
    /// <summary>The attributes to use when drawing the Glyph.</summary>
    public Attribute? Attribute { get; set; } = Attribute;

    /// <summary>
    ///     Gets or sets a value indicating whether this <see cref="T:Terminal.Gui.Cell"/> has been modified since the
    ///     last time it was drawn.
    /// </summary>
    public bool IsDirty { get; set; } = IsDirty;

    private Rune _rune = Rune;

    /// <summary>The character to display. If <see cref="Rune"/> is <see langword="null"/>, then <see cref="Rune"/> is ignored.</summary>
    public Rune Rune
    {
        get => _rune;
        set
        {
            CombiningMarks.Clear ();
            _rune = value;
        }
    }

    private List<Rune> _combiningMarks;

    /// <summary>
    ///     The combining marks for <see cref="Rune"/> that when combined makes this Cell a combining sequence. If
    ///     <see cref="CombiningMarks"/> empty, then <see cref="CombiningMarks"/> is ignored.
    /// </summary>
    /// <remarks>
    ///     Only valid in the rare case where <see cref="Rune"/> is a combining sequence that could not be normalized to a
    ///     single Rune.
    /// </remarks>
    internal List<Rune> CombiningMarks
    {
        get => _combiningMarks ?? [];
        private set => _combiningMarks = value ?? [];
    }

    /// <inheritdoc/>
    public override string ToString () { return $"[{Rune}, {Attribute}]"; }
}

Some observations about record struct's valuable and downside usability:

Advantages:

  • No need to use IEquatable<T> interface. Already implements his Equals and GetHashCode methods.
  • Ideal for no-mutable properties no matter how many exist, because there is no need to add the properties parameters.

Disadvantages:

  • Not recommended for many mutable properties as that will implies to add all them into the parameters.
  • Can't use a foreach block to make changes. Only can be used the for loop block to make the changes.
  • Not very flexible to change properties. Need to get the object at some index, save to a temp variable, set the new values for the properties and finally assign the temp variable to the object at that index. But this isn't a big problem at all, we learn to do that.

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 6, 2024

Added feature to copy, cut and paste cells attributes.

WindowsTerminal_52DHGv6g3h

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

No branches or pull requests

3 participants