Line-break ideographic text by character #6828
Conversation
1ec5
added
Core
|
@1ec5, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jfirebaugh, @kelvinabrokwa and @kkaefer to be potential reviewers. |
| namespace i18n { | ||
|
|
||
| /// Returns whether a line break can be inserted after any character in the given string. | ||
| /// If false, line breaking should occur on word boundaries instead. |
There was a problem hiding this comment.
We're not using triple slashes in core
| @@ -143,7 +153,8 @@ void GlyphSet::lineWrap(Shaping &shaping, const float lineHeight, const float ma | |||
| || shape.glyph == 0xb7 /* middle dot */ | |||
| || shape.glyph == 0x200b /* zero-width space */ | |||
| || shape.glyph == 0x2010 /* hyphen */ | |||
| || shape.glyph == 0x2013 /* en dash */) { | |||
| || shape.glyph == 0x2013 /* en dash */ | |||
There was a problem hiding this comment.
We could move all of those as well to a sister function of allowsIdeographicBreaking in the i18n namespace
|
The failing test is:
|
|
Worth noticing I am temporarily ignoring |
|
The test-suite failure was prexisting. It's tracked in #6796 and the test case is now ignored for native. If you rebase this PR, I expect test-suite will pass. Was that the only remaining obstacle to merging this? |
I also need to address @kkaefer’s feedback, but that should be it. |
|
Are there any tests that specifically test breaking by character? |
|
There are rendering tests in the test suite for the overall ideographic breaking feature, but no comprehensive test that goes character by character. The rendering tests were disabled on the native side in mapbox/mapbox-gl-test-suite@3b005f1, but I verified that they passed before pinning to that commit. |
|
Can we enable these tests when merging? |
|
Yes, it's on the to-do list above. |
|
Putting this on the Android SDK v4.2.0 milestone because it blocks #6996. |
|
@1ec5 any reason why we're not reenabling the tests as part of this PR rather than doing to after merge? |
This reverts commit 3b005f1 in preparation for mapbox/mapbox-gl-native#6828.
OK, I’ve pointed this PR to the 1ec5-換行-1223 branch in gl-test-suite, in which I’ve reenabled the tests. Once I get the green light, I’ll fast-forward-merge that branch, then update the commit hash in package.json, then merge this PR. |
This reverts commit 3b005f1 in preparation for mapbox/mapbox-gl-native#6828.
Allow a line break to be inserted after any supported Chinese, Japanese, or Yi character in a point-placed label. Balance the lines unless non-ideographic text such as Latin letters are present. Fixes #1223.
Automatically insert a line break after any supported Chinese, Japanese, or Yi character in a point-placed label as needed to stay within the
text-max-width. Balance the lines unless non-ideographic text such as Latin letters are present.Compare the label on the map to the popover title, which shows the queried feature’s name. A line break is absent from the feature but applied automatically by the renderer.
Ported from mapbox/mapbox-gl-js#3420. Fixes #1223. Depends on mapbox/mapbox-gl-test-suite#153.
Before merging:
/cc @lucaswoj @xrwang