Skip to content

[WIP] Multiline Entry#4743

Closed
ckaiser wants to merge 1 commit into
aseprite:betafrom
ckaiser:feature/entry-box
Closed

[WIP] Multiline Entry#4743
ckaiser wants to merge 1 commit into
aseprite:betafrom
ckaiser:feature/entry-box

Conversation

@ckaiser

@ckaiser ckaiser commented Oct 29, 2024

Copy link
Copy Markdown
Member

First mostly-working draft of a multiline Entry, still missing a few things but I want to get it out there to validate that I'm at least on the right track.

Highlighted things to be worked on in the code TODOs:

  • Not compensating for TTF character bounds when filling the selection rect
  • Horizontal scrolling in general behaves weirdly, and the caret is not drawn in the right position
  • Untested with RTL languages yet
  • Dead keys get stuck and sometimes eat all input or cause weird behavior
  • Need to integrate the changes from 77dd92c, text currently clips a little bit outside bounds
  • Selecting and translating mouse position to caret has some issues
  • Word deletion removes newlines, might not be wanted behavior

@dacap

dacap commented Nov 1, 2024

Copy link
Copy Markdown
Member

I've just tested a little bit, it's quite a progress so far!

In general terms the approach is correct, but I'm thinking about some points that might change this "a little bit":

  1. Generally scrollable widgets are inside a View/Viewport (the View has the scrollbars + the Viewport, and the Viewport the widget that is bigger which just a part of it is displayed). I see that this multiline Entry doesn't have scrollbars
  2. Probably, a multiline Entry should take a similar route as the Timeline widget: the multiline Entry could contain the horizontal/vertical ui::ScrollBar integrated and be displayed in a similar fashion as the Timeline (and only when they are required)
  3. Given that the multiline starts to get a lot of extra behavior, why not create a MultiEntry or something similar deriving the Entry widget (adding to Entry only the extension points through protected virtual member functions if needed). I don't remember if we talked about this possibility
  4. The horizontal required space should be calculated one time (or each time we change font or text), or in the onSizeHint() function, not on each onPaint()/drawing process (probably that can help you to calculate the required size)
  5. Probably this MultiEntry could have the m_lines/text splitted (m_boxes for each line) instead of doing the split on each onPaint on SkinTheme::drawEntryText, it could update the lines when we change the text (or update only the current line when we edit that specific line), and the Entry logic could ask about the current line to modify (std::string& getCurrentLine() (?)) or something like that.
  6. I'd highly avoid doing refactors while adding new features (e.g. renaming variables), this add a lot of noise to the commit/review process that is unnecessary. In case you want to do some refactor, please create a PR exclusively for that before this one (with just the renames).
  7. Some another little bug introduced with this patch is that the Text tool doesn't paint the Entry edges when we click the canvas to write text (just in case we cannot always bypass the whole painting process when the widget text is empty).

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.

2 participants