cocos2d / cocos2d-x

Cocos2d-x is a suite of open-source, cross-platform, game-development tools utilized by millions of developers across the globe. Its core has evolved to serve as the foundation for Cocos Creator 1.x & 2.x.
https://www.cocos.com/en/cocos2d-x
18.24k stars 7.06k forks source link

UIRichText should not be trimming spaces in RichElementText #18874

Closed rh101 closed 6 years ago

rh101 commented 6 years ago

Steps to Reproduce:

A change made in 3.17, one of which is #18447, the other being #18751, introduced right side text trimming for a RichElementText even if it's not splitting a line, which is incorrect behavior.

A UIRichText is made up of several elements, and spaces in RichElementText are inserted on purpose to make sure there is space between a text element and the next element. If you trim the right side spaces, you're purposely breaking the look of the UIRichText. It is understandable if you want to trim end spaces when line-splitting, but for single line RichElementText no spaces should be trimmed.

One of the problem is in UIRichText.cpp method handleTextRenderer():

            // skip spaces
            StringUtils::StringUTF8::CharUTF8Store& str = utf8Text.getString();
            int rightStart = leftLength;
            while (rightStart < (int)str.size() && str[rightStart].isASCII() && std::isspace(str[rightStart]._char[0], std::locale()))
                ++rightStart; 

The other is in CCLabelTextFormatter.cpp, method multilineTextWrap():

                if (tokenLen != 1 || !StringUtils::isUnicodeSpace(character))
                {
                    tokenRight = nextLetterX / contentScaleFactor;
                }

If there is a valid reason to want to trim the right side spaces in a RichElementText, then give the user an option to either turn it on or off. This can be easily done via the flag bits, such as RichElementText::TRIM_RIGHT_SPACE added to the enum:

enum {
    ITALICS_FLAG = 1 << 0,          /*!< italic text */
    BOLD_FLAG = 1 << 1,             /*!< bold text */
    UNDERLINE_FLAG = 1 << 2,        /*!< underline */
    STRIKETHROUGH_FLAG = 1 << 3,    /*!< strikethrough */
    URL_FLAG = 1 << 4,              /*!< url of anchor */
    OUTLINE_FLAG = 1 << 5,          /*!< outline effect */
    SHADOW_FLAG = 1 << 6,           /*!< shadow effect */
    GLOW_FLAG = 1 << 7,              /*!< glow effect */
    TRIM_RIGHT_SPACE  1 << 8     /*!< trim right side spaces */
};

...or another method, it doesn't matter, just don't have the functionality on by default, because it is just unexpected behavior.

To test it, try this code:

messageBarRichText = ui::RichText::create();
messageBarRichText->ignoreContentAdaptWithSize(true); // Try it with both true and false, both have the problem

auto reText = ui::RichElementText::create(1, color, 255, "This is a test of 3 spaces before image   ", "fonts/Arimo-Regular.ttf", 30, ui::RichElementText::SHADOW_FLAG);
richText->pushBackElement(reText);

auto customSprite = Sprite::createWithSpriteFrameName("CloseNormal.png");
auto reCustom = ui::RichElementCustomNode::create(2, color, 255, customSprite);
richText->pushBackElement(reCustom);

reText = ui::RichElementText::create(3, color, 255, "   and 3 spaces after image", "fonts/Arimo-Regular.ttf", 30, ui::RichElementText::SHADOW_FLAG);
richText->pushBackElement(reText);

Expected output: expected

Actual output (which is not correct!): actual

rh101 commented 6 years ago

I've got a working fix for this issue, without breaking the functionality required by the changes in #18447 and #18751 that I will create a pull request for soon.

One question though that I need input on first, do you want trimming to be default ON or OFF?

If it is default OFF, we can create a flag called: RichElementText::TRIM_TRAILING_WHITESPACE to turn trimming on.

If you want it to be default ON (which is the behavior in 3.17 at the moment), then the flag name needs to reflect the action, so perhaps: RichElementText::KEEP_TRAILING_WHITESPACE to turn trimming off.

summerinsects commented 6 years ago

~~The label with text '___and 3 spaces after image' is same as the text 'and 3 spaces after image' in its content size. It is the reason why the string seems be trimmed~~

skip spaces in handleTextRenderer() is for the case below text__text

it may cut in the continuous spaces, i.e. text[cut here]____text I trim it to make the remainder start at beginning of the new line

rh101 commented 6 years ago

@summerinsects I'm not quite sure what you mean. Assuming trailing spaces are trimmed, and the line is too long to fit into the width of the label, then "text__text", with right alignment, should show up as this in 3.17 (which is expected, and correct, all right side space stripped):

text
text

In v3.16 and prior, it would have shown up as something like this with right alignment as an example (and this is not correct output):

text________________  
________________text

The issue in my post is to do with UIRichText, which uses separate elements, as you can see in the code example in my original post. The RichText is made up of 3 elements, RichElementText -> RichElementCustomNode (sprite) -> RichElementText.

The UIRichText width is set to a greater value than the width of all 3 elements combined, so it should not slice it into multiple lines, and since it is not slicing, it should not remove trailing spaces.

The first RichElementText has "This is a test of 3 spaces before image ", and since the width of this is less than the width of the entire UIRichText, there should be no slicing (3 spaces at the end which should not be trimmed).

summerinsects commented 6 years ago

Yes, leading spaces in a new line should not be trimmed. Your PR is perfect

For leading spaces or trailing spaces in one RichElementText, with your test code, i got the correct output

auto richText = ui::RichText::create();
richText->ignoreContentAdaptWithSize(true); // Try it with both true and false, both have the problem

auto reText = ui::RichElementText::create(1, color, 255, "This is a test of 3 spaces before image   ", "fonts/Arimo-Regular.ttf", 30, ui::RichElementText::SHADOW_FLAG);
richText->pushBackElement(reText);

auto customSprite = Sprite::createWithSpriteFrameName("CloseNormal.png");
auto reCustom = ui::RichElementCustomNode::create(2, color, 255, customSprite);
richText->pushBackElement(reCustom);

reText = ui::RichElementText::create(3, color, 255, "   and 3 spaces after image", "fonts/Arimo-Regular.ttf", 30, ui::RichElementText::SHADOW_FLAG);
richText->pushBackElement(reText);

image

rh101 commented 6 years ago

@summerinsects Thank you for testing it. I've gone through all the test cases in the cpp-tests project relating to Labels and UIRichText, and visually they all seem correct, so I'm hoping it works for everyone.

rh101 commented 6 years ago

The pull request has been tested by another user for a similar issue, and it works. Refer to: https://discuss.cocos2d-x.org/t/label-setstring-strips-white-spaces-at-the-end-i-require-it-not-to-what-can-i-do/42733/2

rh101 commented 6 years ago

Closing this issue since the patch has been merged #18876