Add helper functions to FormattedMessage #5633
Conversation
There was a problem hiding this comment.
Looks good besides a potential issue I found with how you try to find tags in certain helper methods.
If that turns out to be an actual issue you might want to traverse the node array from start to end and for every opening tag that matches you traverse again from that index, keeping a counter that gets incremented for every additional occurrence of a matching opening tag and decremented for every closing tag. If the counter is 0 when encountering a matching closing tag you've found the closing tag that belongs to the opening tag you initially matched.
A flat tree like this isn't very good for operations like that 🤔
I somewhat mentioned this in my initial writeup about richtext: https://hackmd.io/@juliang/r1lzQwPRss
| /// </summary> | ||
| /// <param name="markupNode">The node to be inserted; may not be a text node.</param> | ||
| /// <param name="tagText">The tag to search for.</param> | ||
| public void InsertOutsideTag(MarkupNode markupNode, string tagText) |
There was a problem hiding this comment.
What happens if you, for example, try to wrap color when then markup looks like this:
[color]text1[color/][color]text2[color/]
Wouldn't the second iteration cause InsertAtIndex to throw?
| /// </summary> | ||
| /// <param name="returnMessage">The node to be inserted.</param> | ||
| /// <param name="tagText">The tag to search for.</param> | ||
| public bool TryGetMessageInsideTag(out FormattedMessage? returnMessage, string tagText) |
There was a problem hiding this comment.
Same question as InsertOutsideTag
| /// </summary> | ||
| /// <param name="markupNode">The node to be inserted; may not be a text node.</param> | ||
| /// <param name="tagText">The tag to search for.</param> | ||
| public void InsertInsideTag(MarkupNode markupNode, string tagText) |
There was a problem hiding this comment.
Same question as InsertOutsideTag
|
I've implemented changes based on the review comments. The way the InsertInside/OutsideTag functions work is that they first ensure there's equal amounts of opening and closing target tags (there always should in any correct message, so the check is arguably redudant and could be removed!), and then it simply adds an instance of the new tag before/after every instance of the target tag. TryGetMessageInsideTag was indeed causing issues with multiple instances of a tag. Now it appropriately gets the contents of the first tag. |
This PR adds extra functions to FormattedMessage that allows for greater manipulation of the markup node list. All new functions should be safe, properly closing any new opened tags and dealing with text nodes appropriately.
InsertAtIndex- Inserts a markup node at a specific index in the list. To avoid issues with unclosed tags, any node that isn't a text node is required to also define the index of the closing node.InsertAroundMessage- Wraps the message with a non-text node. Good for full-color text.InsertAroundText- Wraps the first-to-last text node with a non-text node.InsertAroundString- Wraps every occurence of a string with a non-text node. The string must be fully contained in a text node (i.e. no strings separated by other nodes). Useful for codewords.InsertBeforeTag/InsertAfterTag- Inserts a self-closing node in front of/behind every node with a specificName.InsertOutsideTag/InsertInsideTag- Wraps every node/the contents of every node with a specificName.InsertBeforeMessage-InsertAtIndexat index 0.InsertAfterMessage-PushTagwith self-closing set to true.TryGetMessageInsideTag- Gets all nodes inside of the first tag with a specificName. Useful for stuff like speech bubbles.