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

feature: Word-wrap functionality for MultilineTextBox #984

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

Flyga-M
Copy link
Contributor

@Flyga-M Flyga-M commented Sep 18, 2024

Adds word-wrap functionality to theMultilineTextBox. Related to #620.

  • Adds a DisplayText property that may be manipulated by inheriting classes of TextInputBase. The current Text property retains its functionality while the DisplayText makes it possible to change the text that is drawn onto the Control.
  • Implements the new DisplayText property in MultilineTextBox to apply a word-wrap.

notable behaviour changes

  • with the current implementation, all MultilineTextBoxes will have word-wrap applied to them. See "open questions" below for alternatives.
  • DrawUtil.WrapTextSegment() stops adding an additional space character to the end of the text segment. The previous behaviour is assumed to be a bug. This change affects Blish Core in 3 places (SpriteBatchExtensions.DrawStringOnCtrl(), FormattedLabel.InitializeRectangles() and LabelBase.GetTextDimensions()). The consequences at the time of writing are unclear and need to be tested. It's also unclear if any modules rely on that method.

open questions

  • Should word-wrap just be the new implementation of the MultilineTextBox (as it is implemented in this PR), or is it desirable to have a property AutoWordWrap or something like that on the MultilineTextBox so the current behaviour is still possible?
  • If a property like that is wanted, should the default be the current or the new implementation?
  • What should happen if a word is so long, that it won't fit in a line? Break in the middle of the word, or just let it overflow like it currently does?

Implementation details

  • added DisplayText property to TextInputBase

  • added virtual ProcessDisplayText() method to TextInputBase

  • modified TextInputBase to draw new DisplayText instead of Text

  • added WrapText() and WrapTextSegment() overloads to DrawUtil with out parameter that contains the indices of the added new line characters

  • modified DrawUtil.WrapTextSegment() to stop adding an additional space character to the end of the text segement

  • added DisplayNewLineIndices property to MultilineTextBox

  • added GetCursorIndexFromDisplayIndex() and GetDisplayIndexFromCursorIndex() methods to MultilineTextBox to switch between cursorIndex and displayIndex

  • modified MoveLine(), GetCursorIndexFromPosition(), CalculateHighlightRegions() and CalculateCursorRegion() in MultilineTextBox to be based of the DisplayText instead of Text

  • implemented new virtual ProcessDisplayText() override in MultilineTextBox

  • added ApplyWordWrap() method in MultilineTextBox

  • modified GetSplitIndex() in MultilineTextBox to return the line and character based on the DisplayText instead of the Text

  • modified RecalculateLayout() in MultilineTextBox to include recalculation of the DisplayText

  • added HandleDelete override in MultilineTextBox to include a recalculation of the layout after characters were deleted

notable Cons

  • This implementation basically keeps 2 copies of almost identical strings in memory for all TextInputBase subclasses.

Discussion Reference

All new features must be discussed prior to code review. This is to ensure that the implementation aligns with other design considerations. Please link to the Discord discussion:

https://discord.com/channels/531175899588984842/536970543736291346/1285731312056930339

Is this a breaking change?

Breaking changes require additional review prior to merging. If you answer yes, please explain what breaking changes have been made.

No

- added DisplayText property to TextInputBase
- added virtual ProcessDisplayText method to TextInputBase
- modified TextInputBase to draw new DisplayText instead of Text

- added WrapText and WrapTextSegment overloads to DrawUtil with out parameter that contains the indices of the added new line characters
- modified DrawUtil.WrapTextSegment to stop adding an additional space character to the end of the text segement

- added DisplayNewLineIndices property to MultilineTextBox
- added GetCursorIndexFromDisplayIndex and GetDisplayIndexFromCursorIndex methods to MultilineTextBox to switch between cursorIndex and displayIndex
- modified MoveLine, GetCursorIndexFromPosition, CalculateHighlightRegions and CalculateCursorRegion in MultilineTextBox to be based of the DisplayText instead of Text
- implemented new virtual ProcessDisplayText override in MultilineTextBox
- added ApplyWordWrap method in MultilineTextBox
- modified GetSplitIndex in MultilineTextBox to return the line and character based on the DisplayText instead of the Text
- modified RecalculateLayout in MultilineTextBox to include recalculation of the DisplayText
- added HandleDelete override in MultilineTextBox to include a recalculation of the layout after characters were deleted
@dlamkins
Copy link
Member

Should word-wrap just be the new implementation of the MultilineTextBox (as it is implemented in this PR), or is it desirable to have a property AutoWordWrap or something like that on the MultilineTextBox so the current behaviour is still possible?

I think it would desirable to make this a setting and make it the default.

What should happen if a word is so long, that it won't fit in a line? Break in the middle of the word, or just let it overflow like it currently does?

After playing around some with the build, I believe the prior. If you don't break somewhere then it just extends completely off the screen and isn't visible. Maybe that isn't likely to happen, but I imagine it could for smaller inputs or depending on how the module dev is using it.

The rest of the changes seem safe. I didn't notice any issue when running through a couple of things.

@Flyga-M
Copy link
Contributor Author

Flyga-M commented Sep 30, 2024

Thanks for the testing and feedback!

Will probably submit changes sometime in the next couple of days.

Flyga-M and others added 2 commits October 5, 2024 15:58
- added wrap-functionality in the middle of a word
- added DisableWordWrap property on MultilineTextBox
- added WrapCharacters property on MultilineTextBox
- added WrapWord method to DrawUtil
- modified WrapTextSegment and WrapText methods in DrawUtil to wrap words that are too long to fit in a single line
- added overloads to WrapTextSegment and WrapText methods in DrawUtil to allow for additional wrap characters where a word may wrap.
…ments

finishing touches on word-wrap for MultilineTextBoxes
@Flyga-M
Copy link
Contributor Author

Flyga-M commented Oct 5, 2024

Added word-wrap functionality in the middle of a word.

With the added changes

  • a word that won't fit in a single line will be wrapped
  • MultilineTextBoxes will have the word-wrap functionality by default
  • word wrap can be disabled by using the DisableWordWrap property on a MultilineTextBox
  • additional characters where a word may wrap can be specified via the WrapCharacters property on a MultilineTextBox. This affects all words, not just the ones that won't fit in a single line by themselves

Final thoughts

I thought about adding a dash - as a default wrap character for MultilineTextBoxes, but decided against it, because there may be some use cases where this is not preferable.

As I mentioned above, the changes to the utility functions do affect other places in core and may also affect some modules. The new changes to breaking a word in the middle have larger ramifications than the removed space above.
Where a word was too long before, it would just be cropped and ignored, but now a new line (er even multiple) are introduced, which changes text dimensions in Labels or wrapped text that is rendered via DrawStringOnCtrl.

This could be avoided by introducing a new method WrapText2 with every place in core (other than MultilineTextBoxes) keeping the reference to WrapText. Even though the changes might even be beneficial in those places too.
To determine the better choice of action there are still some tests needed. I'm just not confident that I currently have the resources to make those test myself.

If the changes should eventually be brought over to other places like DrawStringOnCtrl, then it will be possible to add a new parameter wrapCharacters to the method. Making the benefit of the added word wrapping utility accessible for module devs in other places.

Implementation details

  • added DisableWordWrap property on MultilineTextBox
  • added WrapCharacters property on MultilineTextBox
  • added WrapWord method to DrawUtil
  • modified WrapTextSegment and WrapText methods in DrawUtil to wrap words that are too long to fit in a single line
  • added overloads to WrapTextSegment and WrapText methods in DrawUtil to allow for additional wrap characters where a word may wrap.

@Flyga-M Flyga-M changed the title WIP: word-wrap functionality for MultilineTextBox feature: Word-wrap functionality for MultilineTextBox Oct 5, 2024
@Flyga-M Flyga-M marked this pull request as ready for review October 5, 2024 14:54
Flyga-M added a commit to Flyga-M/Blish-HUD that referenced this pull request Oct 5, 2024
- modified blish-hud#992 to be compatible with blish-hud#984
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