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

CJK soft wrap #9162

Merged
merged 23 commits into from Oct 16, 2015

Conversation

Projects
None yet
8 participants
@as-cii
Member

as-cii commented Oct 15, 2015

Instead of considering every character to have the same width when we perform soft-wrapping, we now make a distinction between default characters, double width characters, half width characters and Korean characters .

The strategy consists in measuring these kinds of characters once when sampling font styles and then using the calculated width to find where a certain line wraps.

Please, note that this won't work with non-monospaced fonts, but it's consistent with how we currently handle soft-wrapping for latin characters.

/cc: @nathansobo @maxbrunsfeld

@benogle

This comment has been minimized.

Show comment
Hide comment
@benogle

benogle Oct 15, 2015

Contributor

OMG, if this fixes the 80% case, I ❤️ ❤️ you. 🎉

Contributor

benogle commented Oct 15, 2015

OMG, if this fixes the 80% case, I ❤️ ❤️ you. 🎉

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Oct 15, 2015

Member

cjk

@benogle: 😂 👆 👆 👆 ❤️

I had to treat Korean characters differently, as their width isn't the same as japanese and chinese. I couldn't find a good name to express this in code (as opposed to double width and half width), so I simply named them koreanCharacters (I could have probably mentioned "Hangul" but as a non-native speaker it didn't feel super memorable).

Honestly I performed only super basic tests to check if this worked fine: probably a native speaker could give a more detailed feedback. I'll make sure to mention some of the people who actively participated in the CJK discussion if this looks good code-wise.

Thanks!

Member

as-cii commented Oct 15, 2015

cjk

@benogle: 😂 👆 👆 👆 ❤️

I had to treat Korean characters differently, as their width isn't the same as japanese and chinese. I couldn't find a good name to express this in code (as opposed to double width and half width), so I simply named them koreanCharacters (I could have probably mentioned "Hangul" but as a non-native speaker it didn't feel super memorable).

Honestly I performed only super basic tests to check if this worked fine: probably a native speaker could give a more detailed feedback. I'll make sure to mention some of the people who actively participated in the CJK discussion if this looks good code-wise.

Thanks!

@saschanaz

This comment has been minimized.

Show comment
Hide comment
@saschanaz

saschanaz Oct 16, 2015

Korean characters use spaces but usually they don't wrap by words. For example:

아이돌마스터 신데렐라걸즈 스타라이트   |
스테이지

아이돌마스터 신데렐라걸즈 스타라이트 스| <- This is usually preferred
테이지

Web browser behavior example: https://twitter.com/newstapa/status/654793987543306241

saschanaz commented Oct 16, 2015

Korean characters use spaces but usually they don't wrap by words. For example:

아이돌마스터 신데렐라걸즈 스타라이트   |
스테이지

아이돌마스터 신데렐라걸즈 스타라이트 스| <- This is usually preferred
테이지

Web browser behavior example: https://twitter.com/newstapa/status/654793987543306241

@Zireael07

This comment has been minimized.

Show comment
Hide comment
@Zireael07

Zireael07 Oct 16, 2015

<3 this is amazing for CJK users

Zireael07 commented Oct 16, 2015

<3 this is amazing for CJK users

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Oct 16, 2015

Member

@saschanaz: I wonder if, as a first step, it could be acceptable to wrap by words. How bad do you think that would be? The 🆒 thing about this approach is that it fits quite nicely our current code structure: it's far from being perfect, but it seems like it's way better than how we handle this scenario currently. What do you think? By the way, thanks for all the effort you have put into this. ❤️

Member

as-cii commented Oct 16, 2015

@saschanaz: I wonder if, as a first step, it could be acceptable to wrap by words. How bad do you think that would be? The 🆒 thing about this approach is that it fits quite nicely our current code structure: it's far from being perfect, but it seems like it's way better than how we handle this scenario currently. What do you think? By the way, thanks for all the effort you have put into this. ❤️

@saschanaz

This comment has been minimized.

Show comment
Hide comment
@saschanaz

saschanaz Oct 16, 2015

@as-cii I think there can be some more progress but I agree that this is far better than current behavior. Great work 👍

saschanaz commented Oct 16, 2015

@as-cii I think there can be some more progress but I agree that this is far better than current behavior. Great work 👍

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Oct 16, 2015

Contributor

🚢

Contributor

maxbrunsfeld commented Oct 16, 2015

🚢

nathansobo added a commit that referenced this pull request Oct 16, 2015

@nathansobo nathansobo merged commit 30f45d3 into master Oct 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nathansobo nathansobo deleted the as-cjk-soft-wrap branch Oct 16, 2015

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 16, 2015

Contributor

Thanks @as-cii for your continued excellence.

Contributor

nathansobo commented Oct 16, 2015

Thanks @as-cii for your continued excellence.

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Oct 16, 2015

Member

❤️

Member

as-cii commented Oct 16, 2015

❤️

@benogle

This comment has been minimized.

Show comment
Hide comment
@benogle

benogle Oct 16, 2015

Contributor

I feel like this should close #1783 Thoughts?

Contributor

benogle commented Oct 16, 2015

I feel like this should close #1783 Thoughts?

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Oct 16, 2015

Contributor

Yeah, I agree. We can create more specific issues for whatever remaining problems we find.

Contributor

maxbrunsfeld commented Oct 16, 2015

Yeah, I agree. We can create more specific issues for whatever remaining problems we find.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Oct 16, 2015

Member

so awesome 🎆

Member

kevinsawicki commented Oct 16, 2015

so awesome 🎆

@ethanluoyc

This comment has been minimized.

Show comment
Hide comment
@ethanluoyc

ethanluoyc Oct 16, 2015

This is awesome

ethanluoyc commented Oct 16, 2015

This is awesome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment