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

RichText improvements #18447

Merged
merged 7 commits into from Nov 8, 2017
Merged

RichText improvements #18447

merged 7 commits into from Nov 8, 2017

Conversation

summerinsects
Copy link
Contributor

@summerinsects summerinsects commented Nov 6, 2017

This is a PR to improve RichText.

There is a feedback that can not get the real height of a richtext from many forums.
I modify _customSize.height in function RichText::formatRenderers to make content size equals to real size.


改进RichText

许多论坛反馈RichText无法获取真实高度。我在函数RichText::formatRenderers里面修改了_customSize.height,使得content size等于真实尺寸。

int getNextWordPos(const StringUtils::StringUTF8& text, int idx)
{
const StringUtils::StringUTF8::CharUTF8Store& str = text.getString();
auto it = std::find_if(str.begin() + idx + 1, str.end(), isUTF8CharWrappable);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if idx == str.length(), then it may have problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will be out of bounds. I will add an if condition

leftStr = text.getAsCharSequence(0, newidx);
label->setString(leftStr);
textRendererWidth = label->getContentSize().width;
if (textRendererWidth < originalLeftSpaceWidth)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if textRendererWidth == originalLeftSpaceWidth, then should return newidx instead of idx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I will add an if condition

// 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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why need to skip spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In original code, it skips one space.
But I think skipping all spaces may be better.

eg(_ instead of space): "abcd________________123"
suppose it is splitted after letter 'd'
Should it desplay like

abcd
123

or

abcd
________________123
? please give me some advice

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not idea. How it been displayed on Android/iOS UI element? I think can have the same behavior as them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I test it with iOS's webview. It trims the spaces. see below

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

K, thanks.

}
if (row.empty())
{
maxHeight = std::max(fontSize, _defaultHeights[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why height compare with font size?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for continuous new lines.
eg:"abc\n\n123" should display like this:
abc

123

Label* leftRenderer = nullptr;
if (fileExist)
addNewLine();
_defaultHeights.back() = fontSize;
Copy link
Contributor

@minggo minggo Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why height vector record font size?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also for continuous new lines.
eg:"abc\n\n123" should display like this:
abc

123

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be should use another name for the member variable, the name is _defaultHeights , but the element value is font size, it is hard to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about _emptyLineHeights

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand why height is equal to font size. When i reviewed the codes, it seems font size is used as height.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • there is a RichText with fontSize 20 contains RichElementText(string="abc\n\n123", fontSize=10).
    Then it will display like this:
abc

123

Now, the gap between two lines should be equal to RichElementText's fontSize 10

  • In another case, there is a RichText with fontSize 20 contains RichElementText("abc"), RichElementNewLine, RichElementNewLine, RichElementText("123").
    Then it will display like this:
abc

123

Now, the gap between two lines should be equal to RichText's fontSize 20

@summerinsects
Copy link
Contributor Author

I test it with iOS's webview. It trims the spaces.
wx20171107-181012

img_20171107_181412

leftStr.append(ch._char);
label->setString(leftStr);
textRendererWidth = label->getContentSize().width;
if (originalLeftSpaceWidth < textRendererWidth) // protruded, undo add
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if originalLeftSpaceWidth == textRendererWidth, then it should return leftLength

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

// gap for empty line, if _lineHeights[i] == 0, use current RichText's fontSize
if (row.empty())
{
maxHeight = (_lineHeights[i] != 0.0f ? _lineHeights[i] : fontSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use font size for height? And when row will be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider the following two cases:

  • a RichText with fontSize 20 contains RichElementText(string="abc\n\n123", fontSize=10).
    Then it will display like this:
abc

123

Now, the gap between two lines should be equal to RichElementText's fontSize 10

  • a RichText with fontSize 20 contains RichElementText("abc"), RichElementNewLine, RichElementNewLine, RichElementText("123").
    Then it will display like this:
abc

123

Now, the gap between two lines should be equal to RichText's fontSize 20

In both of two cases, row.empty() == true. because of RichText::addNewLine(), the code _elementRenders.emplace_back();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use vertical space instead of font size? Use font size as line height is strange.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vertical space is not same as font size.please see below

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds reasonable

while (_elementRenders.back().empty())
{
_elementRenders.pop_back();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When element render will be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the case of end with some new lines, for example, the last element of a RichText is RichElementNewLine, or RichElementText with some \n's.
RichText::addNewLine() will be invoked, but no renders in the vector.

Copy link
Contributor

@minggo minggo Nov 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove them? May be it is the expected effect, use some new line at the end to make some space at the end of content.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first, I think new lines at end of RichText is meaningless.
Maybe we can add a method to decide whether trim or not, how about it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may make things complex. If they don't want the effect, then why add these new lines explicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I see. I will delete these codes

@summerinsects
Copy link
Contributor Author

In my opinion
wx20171108-101726

@minggo minggo merged commit b7bb03a into cocos2d:v3 Nov 8, 2017
@minggo minggo added this to the 3.17 milestone Nov 8, 2017
@summerinsects
Copy link
Contributor Author

@minggo
I get inspiration from class Labels source code, which is

const Size& Label::getContentSize() const
{
    if (_systemFontDirty || _contentDirty)
    {
        const_cast<Label*>(this)->updateContent();
    }
    return _contentSize;
}

Rect Label::getBoundingBox() const
{
    const_cast<Label*>(this)->getContentSize();

    return Node::getBoundingBox();
}

It means that when invoke getContentSize with a dirty content, it should update first.

Are these codes necessary similarly?

const Size& RichText::getContentSize() const
{
    if (_formatTextDirty)
    {
        const_cast<RichText*>(this)->formatText();
    }
    return Widget::getContentSize();
}

Rect RichText::getBoundingBox() const
{
    if (_formatTextDirty)
    {
        const_cast<RichText*>(this)->formatText();
    }
    return Widget::getBoundingBox();
}

@minggo
Copy link
Contributor

minggo commented Nov 8, 2017

From the logic it seems it is needed, but i am not sure. Do you meet any problem?

@summerinsects
Copy link
Contributor Author

summerinsects commented Nov 8, 2017

I did not notice this problem.
In my code, I always invoked formatText explicitly before getContentSize
I am not sure whether overriding these functions will cause side effects

@minggo
Copy link
Contributor

minggo commented Nov 8, 2017

I am not sure too, since some call back is invoked from UIWidget. So let's keep as it is and fix them when meet issue.

@summerinsects
Copy link
Contributor Author

OK

@summerinsects summerinsects deleted the richtext branch November 10, 2017 08:36
stevetranby added a commit to stevetranby/cocos2d-x that referenced this pull request Nov 19, 2017
* commit 'bceb242ebd9c0bf60fdf1cfcf671efc47f07e1ee': (28 commits)
  no message (cocos2d#18460)
  Added missing #include statement (cocos2d#18452)
  Fix cocos2d::log va_list re-use bug (cocos2d#18426)
  Improve StringUtils::format implementation. (cocos2d#18425)
  RichText improvements (cocos2d#18447)
  Update CCTableView.cpp (cocos2d#18451)
  Update UIWebViewImpl-ios.mm (cocos2d#18448)
  Reverts add search path logic in PR cocos2d#17435 . (cocos2d#18419)
  Fix 60 fps for android (cocos2d#18445)
  set texture anti-alias by deault in RenderTexture (cocos2d#18442)
  fix typos (cocos2d#18444)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#18440)
  Spine update (cocos2d#18438)
  remove vs2013 libs (cocos2d#18439)
  Revert cocos2d#18327 (cocos2d#18436)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#18435)
  update glfw to 3.2.1 (cocos2d#18434)
  Editbox init member variable (cocos2d#18432)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#18431)
  Update spine (cocos2d#18427)
  ...

# Conflicts:
#	build/cocos2d_libs.xcodeproj/project.pbxproj
#	cocos/Android.mk
#	cocos/base/CCConsole.cpp
#	cocos/editor-support/spine/Android.mk
#	cocos/editor-support/spine/Animation.c
#	cocos/editor-support/spine/Animation.h
#	cocos/editor-support/spine/AnimationState.c
#	cocos/editor-support/spine/AnimationState.h
#	cocos/editor-support/spine/AnimationStateData.h
#	cocos/editor-support/spine/Atlas.c
#	cocos/editor-support/spine/Atlas.h
#	cocos/editor-support/spine/AtlasAttachmentLoader.c
#	cocos/editor-support/spine/AtlasAttachmentLoader.h
#	cocos/editor-support/spine/Attachment.h
#	cocos/editor-support/spine/AttachmentLoader.h
#	cocos/editor-support/spine/Bone.c
#	cocos/editor-support/spine/Bone.h
#	cocos/editor-support/spine/BoneData.h
#	cocos/editor-support/spine/BoundingBoxAttachment.c
#	cocos/editor-support/spine/BoundingBoxAttachment.h
#	cocos/editor-support/spine/CMakeLists.txt
#	cocos/editor-support/spine/Event.h
#	cocos/editor-support/spine/EventData.h
#	cocos/editor-support/spine/IkConstraint.c
#	cocos/editor-support/spine/IkConstraint.h
#	cocos/editor-support/spine/IkConstraintData.h
#	cocos/editor-support/spine/MeshAttachment.c
#	cocos/editor-support/spine/MeshAttachment.h
#	cocos/editor-support/spine/PathAttachment.c
#	cocos/editor-support/spine/PathAttachment.h
#	cocos/editor-support/spine/PathConstraint.c
#	cocos/editor-support/spine/PathConstraint.h
#	cocos/editor-support/spine/PathConstraintData.h
#	cocos/editor-support/spine/RegionAttachment.c
#	cocos/editor-support/spine/RegionAttachment.h
#	cocos/editor-support/spine/Skeleton.c
#	cocos/editor-support/spine/Skeleton.h
#	cocos/editor-support/spine/SkeletonAnimation.cpp
#	cocos/editor-support/spine/SkeletonAnimation.h
#	cocos/editor-support/spine/SkeletonBatch.cpp
#	cocos/editor-support/spine/SkeletonBatch.h
#	cocos/editor-support/spine/SkeletonBinary.c
#	cocos/editor-support/spine/SkeletonBinary.h
#	cocos/editor-support/spine/SkeletonBounds.c
#	cocos/editor-support/spine/SkeletonBounds.h
#	cocos/editor-support/spine/SkeletonData.h
#	cocos/editor-support/spine/SkeletonJson.c
#	cocos/editor-support/spine/SkeletonJson.h
#	cocos/editor-support/spine/SkeletonRenderer.cpp
#	cocos/editor-support/spine/SkeletonRenderer.h
#	cocos/editor-support/spine/Skin.h
#	cocos/editor-support/spine/Slot.c
#	cocos/editor-support/spine/Slot.h
#	cocos/editor-support/spine/SlotData.c
#	cocos/editor-support/spine/SlotData.h
#	cocos/editor-support/spine/TransformConstraint.c
#	cocos/editor-support/spine/TransformConstraint.h
#	cocos/editor-support/spine/TransformConstraintData.h
#	cocos/editor-support/spine/VertexAttachment.c
#	cocos/editor-support/spine/VertexAttachment.h
#	cocos/editor-support/spine/extension.c
#	cocos/editor-support/spine/extension.h
#	cocos/editor-support/spine/kvec.h
#	cocos/editor-support/spine/proj.win32/libSpine.vcxproj
#	cocos/editor-support/spine/proj.win32/libSpine.vcxproj.filters
#	cocos/editor-support/spine/spine.h
#	cocos/ui/UIRichText.h
#	plugin
#	tools/cocos2d-console
stevetranby added a commit to stevetranby/cocos2d-x that referenced this pull request Nov 21, 2017
* commit 'a88ad877314a7b27cd16b33368862f1a3bdc95bd':
  Bug in mp3reader.cpp (cocos2d#18476)
  Uses Images.xcassets instead of several icon png files. (cocos2d#18485)
  fix comments for JniHelper.h (cocos2d#18481)
  Fixing crash in Allocator if there are no allocated pages (cocos2d#12908)
  Unregister spine event lua event handler should set listener to null. (cocos2d#12845)
  --force-yes => --allow, -y (cocos2d#18464)
  Add Comments for JniHelper.h (cocos2d#18477)
  no message (cocos2d#18460)
  Added missing #include statement (cocos2d#18452)
  Fix cocos2d::log va_list re-use bug (cocos2d#18426)
  Improve StringUtils::format implementation. (cocos2d#18425)
  RichText improvements (cocos2d#18447)
  Update CCTableView.cpp (cocos2d#18451)
  Update UIWebViewImpl-ios.mm (cocos2d#18448)

# Conflicts:
#	cocos/base/CCConsole.cpp
@summerinsects summerinsects restored the richtext branch December 19, 2017 14:23
@summerinsects summerinsects deleted the richtext branch December 22, 2017 01:52
@rh101
Copy link
Contributor

rh101 commented Jun 4, 2018

Guys, introducing the trimming spaces on the right side of a text element makes no sense. It should not trim any space, since a RichText is made out of several elements, and spaces on the right side of text elements are used to literally put space between those elements.

For example, say you have this UIRichText made up of 3 elements, text, image, and another text:

text[space]Image[space]text

which should be rendered as:

text image text

yet it appears as:

textimagetext

If for some reason you need it to trim right side spaces, then at least give us the option to turn that functionality on or off by some kind of flag bit (such as RichElementText::TRIM_RIGHT_SPACE etc).

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.

None yet

3 participants