Skip to content

MVP for Multi-Line Textbox #6564

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

minetoblend
Copy link
Contributor

@minetoblend minetoblend commented Apr 10, 2025

A MVP implementation for a multi-line text box. This is still a bit messy and incomplete, but I wanted to see if this approach is generally a step in the right direction before going further.

I opted for splitting the text into words & putting them as individual DrawableSprites into a FillFlowContainer for the layout, and compute a small index from that to help with converting between draw-position and text-positions whenever the layout gets recalculated.
I also attempted to extract a TextSelection class from the selection handling in TextBox, so that the selection behavior could be shared between the classes without relying on inheritance.

There's still a couple bugs that I'm aware of

  • Multiple line breaks in a row get treated like a single line break.
  • Line breaks at the end of the string don't show up
  • Selection from mouse-down throws sometimes after the line count has changed, likely caused by a bug in the LineInfo calculation leading to null items in the Lines array
2025-04-10.03-34-38.mp4

Closes #1618

@Joehuu
Copy link
Member

Joehuu commented Apr 10, 2025

This will close #1618 (can't edit OP).

@Joehuu Joehuu added the area:UI label Apr 10, 2025
@frenzibyte
Copy link
Member

frenzibyte commented Apr 10, 2025

This looks pretty interesting on a quick glimpse. Without diving in details, has there been any exploration or intention to merge this together with TextBox, if so, what's the end result of having that? The amount of logic copied across between TextBox and this new TextArea component feels too rough to accept as coexisting together.

I'm also curious to know what sparked this much effort out of nowhere, has this been discussed somewhere else or just pushed out like this?

@minetoblend
Copy link
Contributor Author

minetoblend commented Apr 10, 2025

has there been any exploration or intention to merge this together with TextBox, if so, what's the end result of having that? The amount of logic copied across between TextBox and this new TextArea component feels too rough to accept as coexisting together.

The TextSelection class is about as far as I went with that, though I didn't want to alter the TextBox class to make use of it too before seeing if this is the way to go about things. I think relying on inheritance is an easy way to shoot yourself in the foot with 2 components as complex as this, so the goal was to favor composition over inheritance here. Also makes it much easier to test these things in isolation.
The same approach could be used for other parts of the TextBox too.

I'm also curious to know what sparked this much effort out of nowhere

For the most part just boredom, though I know that at some point I'll need a multiline text box for my typescript port of osu-framework, so I figured I might as well just make it for osu-framework first since it seems like a useful component to have.
I'm also interested in eventually seeing a ModdingV2 integration in lazer (see ppy/osu-web#11113), and having a multiline text box is kind of a requirement for that.

has this been discussed somewhere else or just pushed out like this?

There's a brief exchange on discord from earlier today but nothing besides that.

@minetoblend
Copy link
Contributor Author

minetoblend commented Apr 10, 2025

Couple more thoughts on the whole thing

It'd probably make sense to add entries for the following 2 actions to PlatformAction

  • Vertical text navigation (up/down keys)
  • Go to text start/end (ctrl+home/end on windows)

I think naming of Select/MoveForwardLine should be reconsidered, something like SelectLineStart/End maybe. Those names are pretty ambiguous if you can also navigate up/down between lines. Without context I'd assume they're for vertical navigation.

@bdach
Copy link
Collaborator

bdach commented Apr 28, 2025

This MVP is interesting but on a very cursory glance (yay for >1k diff stats! not) I have got so many open questions about it that I don't know where to begin.

  • Some of the ugly warts that I just got rid of are returning (FillFlowContainer.ForceNewRow), and new ones are appearing (what is OnLayoutComputed() even for? signature looks terrible, not to mention that a virtual ComputeLayoutPositions() calling a virtual OnLayoutComputed() callback feels like an overdose of OOP)
  • Inherits some of the weird breakage of flows, again (long words that exceed the width of the text box do not break a line and overflow instead, one of the worst problems remaining with text flow)
  • No audio feedback, no customisable caret animations (may feel like a much of muchness but standard text box supports it), no IME, no clipboard support even? How are these going to happen, by pasting from textbox?
  • I've gotten more bearish on code duplication as time goes on but how is stuff like
    private Word[] splitWords(string text)
    {
    var words = new List<Word>();
    var builder = new StringBuilder();
    bool lastWordWasNewline = false;
    for (int i = 0; i < text.Length; i++)
    {
    if (i == 0
    || char.IsSeparator(text[i - 1])
    || char.IsControl(text[i - 1])
    || char.GetUnicodeCategory(text[i - 1]) == UnicodeCategory.DashPunctuation
    || text[i - 1] == '/'
    || text[i - 1] == '\\'
    || (isCjkCharacter(text[i - 1]) && !char.IsPunctuation(text[i])))
    {
    words.Add(new Word(builder.ToString(), lastWordWasNewline));
    builder.Clear();
    lastWordWasNewline = i > 0 && text[i - 1] == '\n';
    }
    bool isNewLine = text[i] == '\n';
    builder.Append(isNewLine ? ' ' : text[i]);
    }
    if (builder.Length > 0)
    words.Add(new Word(builder.ToString(), false));
    return words.ToArray();
    bool isCjkCharacter(char c) => c >= '\x2E80' && c <= '\x9FFF';
    }
    not in a helper even (was lifted from TextFlowContainer)?

I dunno. It feels like there'll need to be so much follow-up work done on this to get it out of "MVP" into "usable" that I'm not even sure I have confidence this is the correct first step. If I were to say what I'd consider maybe better gun to my head style, it's probably either

  • Attempt to sell TextBox for parts so that bigger chunks of that monstrosity can be reused in this thing
  • Strip the part in this that does multiline, jam it in TextBox, and just jerry-rig things such that the current text box is a special case of the "generalised textbox" (i.e. all existing consumers work in "single-line mode", and anyone new that wants multiline just flips a flag or something).

Which is to say that to assess whether this is workable I'd probably have to have a go myself, and I do not foresee wanting to do that anytime soon 🤷

@minetoblend
Copy link
Contributor Author

minetoblend commented Apr 28, 2025

  • Some of the ugly warts that I just got rid of are returning (FillFlowContainer.ForceNewRow), and new ones are appearing (what is OnLayoutComputed() even for? signature looks terrible, not to mention that a virtual ComputeLayoutPositions() calling a virtual OnLayoutComputed() callback feels like an overdose of OOP)

The ComputeLayoutPositions function was mainly a way to get ahold of the row data computed in FillFlowContainer but I agree it's not a great solution. In hindsight the layout should have probably just been done with a custom implementation here (probably something that directly talks to the glyph store to handle text wrapping). Something along the lines of the TextBuilder class but able to handle multiline & also providing a method of mapping between draw position & character indices.

  • Inherits some of the weird breakage of flows, again (long words that exceed the width of the text box do not break a line and overflow instead, one of the worst problems remaining with text flow)

See above

  • No audio feedback, no customisable caret animations (may feel like a much of muchness but standard text box supports it), no IME, no clipboard support even? How are these going to happen, by pasting from textbox?

Those are things I wanted to do after having a solid foundation that everyone agrees with, which is the main purpose of this pr.

  • I've gotten more bearish on code duplication as time goes on but how is stuff like
    private Word[] splitWords(string text)
    {
    var words = new List<Word>();
    var builder = new StringBuilder();
    bool lastWordWasNewline = false;
    for (int i = 0; i < text.Length; i++)
    {
    if (i == 0
    || char.IsSeparator(text[i - 1])
    || char.IsControl(text[i - 1])
    || char.GetUnicodeCategory(text[i - 1]) == UnicodeCategory.DashPunctuation
    || text[i - 1] == '/'
    || text[i - 1] == '\\'
    || (isCjkCharacter(text[i - 1]) && !char.IsPunctuation(text[i])))
    {
    words.Add(new Word(builder.ToString(), lastWordWasNewline));
    builder.Clear();
    lastWordWasNewline = i > 0 && text[i - 1] == '\n';
    }
    bool isNewLine = text[i] == '\n';
    builder.Append(isNewLine ? ' ' : text[i]);
    }
    if (builder.Length > 0)
    words.Add(new Word(builder.ToString(), false));
    return words.ToArray();
    bool isCjkCharacter(char c) => c >= '\x2E80' && c <= '\x9FFF';
    }

    not in a helper even (was lifted from TextFlowContainer)?

I originally did that but the code here has slightly different handling regarding newlines which is why I moved it here. If I do layout manually (like mentioned in first paragraph) this method should become redundant anyways.

  • Attempt to sell TextBox for parts so that bigger chunks of that monstrosity can be reused in this thing

I already touched a bit upon this in an earlier comment, but my TextSelection class is an attempt at a first step in that direction. Personally I'd be in favor of this approach since a lot of this stuff should work the same regardless of what kind of textbox it's put into.

  • Strip the part in this that does multiline, jam it in TextBox, and just jerry-rig things such that the current text box is a special case of the "generalised textbox" (i.e. all existing consumers work in "single-line mode", and anyone new that wants multiline just flips a flag or something).

Can't say I'm on board with that one, the TextBox class is already so complex that I think this would make the whole thing nearly unmaintainable.

I'm a bit unsure about next steps here, since I prefer the the approach of splitting the textbox behavior into multiple chunks this is something I could look into.
What do you say about me doing a bunch of prs attempting to split out things bit-by-bit (probably best to start with the selection since I already got started on that)? This would also be a great opportunity for adding tests for the individual parts of the textbox. The multiline textbox can get revisited afterwards.

@bdach
Copy link
Collaborator

bdach commented Apr 29, 2025

What do you say about me doing a bunch of prs attempting to split out things bit-by-bit (probably best to start with the selection since I already got started on that)? This would also be a great opportunity for adding tests for the individual parts of the textbox. The multiline textbox can get revisited afterwards.

Maybe fine? I don't know. Will need to assess on a case-by-case basis. I can definitely personally say that I think there's a higher chance of that succeeding than this PR going in unchanged as I see things right now.

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

Successfully merging this pull request may close these issues.

Add MultilineTextBox component
4 participants