[go: nahoru, domu]

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

Set letterSpacing to 0 for cursive writing systems #596

Open
wants to merge 2 commits into
base: androidx-main
Choose a base branch
from

Conversation

YektaDev
Copy link

Letter spacing doesn't play well with cursive writing systems, as it can break the connection between letters and make the text fragmented. To solve this problem, I propose checking the first "letter" character in the text using isLetter(). If it belongs to a cursive writing system (Persian and Arabic to name a few) and letterSpacing is greater than 0, set letterSpacing to 0.sp. However, I agree that it feels "hacky" and better fixes can be applied at lower levels. I just wanted to share my solution to this.

An example of how the text is fragmented

  • This is a "Hello World" in Persian:
    سلام دنیا

  • This is how it looks like when space is added between the letters (Which looks broken/odd in a cursive writing system):
    سـ ـلـ ـا م ‌ د نـ ـیـ ـا

* as it can break the connection between letters and make the text fragmented.
*/
@Stable
private fun String.isFirstLetterCursive(): Boolean {

Choose a reason for hiding this comment

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

In this version, we have defined a binarySearchRange extension for the range list that performs a binary search to check if a value is in any of the ranges. This drastically reduces the number of comparisons needed compared to the original approach of comparing against each range individually.

@Stable
private fun String.isFirstLetterCursive(): Boolean {
    val cursiveRanges = listOf(
        1536..1920,
        1984..2048,
        2144..2304,
        4096..4256,
        43488..43520,
        43616..43648,
        64336..65024,
        65136..65280
    )

    for (char in this) {
        if (!char.isLetter()) continue
        val c = char.code

        if (cursiveRanges.binarySearchRange(c)) {
            return true
        }
    }

    return false
}

private fun List<IntRange>.binarySearchRange(value: Int): Boolean {
    var low = 0
    var high = size - 1

    while (low <= high) {
        val mid = (low + high) / 2
        val range = this[mid]

        if (value in range) {
            return true
        }

        if (value < range.first) {
            high = mid - 1
        } else {
            low = mid + 1
        }
    }

    return false
}

Copy link
Author

Choose a reason for hiding this comment

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

Hey @miguelalegria, thanks for the suggestion. The reason I didn't incorporate binary search and ranges is to avoid unnecessary allocations given the size of our search. I assume the current workaround would be more efficient. However, this pull request isn't probably going to be merged since the problem lies in lower layer of the system.

@lelandrichardson
Copy link
Member

Hey @YektaDev thanks for surfacing this issue and proposing a fix! I don't work directly on the Compose text stack and this should probably ultimately be decided by someone who does, but I thought I would share some of my thoughts.

There's a bit of a philosophical question here. I definitely agree that a cursive writing system and large letter spacing feel a bit at odds with each other and that in most cases, applying a positive letter spacing here is usually not desired, but I am also not sure that enforcing that here is the right place to be doing so. One thing in particular that bothers me about this solution is the fact that we aren't giving anyone a way to get around it if they need to. Additionally, this is being done for Material 3's Text, but not the underlying BasicText, nor for DrawScope.drawText, or even TextView.

I can imagine some fonts may handle cursive characters slightly differently and may require very slight tweaking of letter spacing in order to display correctly, and this would defeat it. I can imagine some typesetting app where adjusting these values would be expected to result in differences. I am sure there are others but I'm not all that imaginative :)

Additionally, I am curious if there are examples of any other frameworks or toolkits which behave this way? Android does not and I just tried and it looks like browsers do not either.

@YektaDev
Copy link
Author
YektaDev commented Aug 12, 2023

Hey @lelandrichardson, thanks for your time and attention.

I agree with almost all of the points you've made. Especially that Material 3's Text Composable is not the right place to address this concern; it definitely needs to be addressed in lower layers. I just wanted to show my workaround for this problem (alongside maybe showing the range of characters in which the writing system happens to be cursive).

However, for cursive systems ─ or at least for Persian and Arabic since I can speak for them ─ fragmentation of characters is not a matter of "not feeling right", it is simply broken and wrong (it's even mentioned by https://www.w3.org/TR/css-text-3/#example-9902d8b5).

Even though I still agree that this is the way both Android and Browsers behave and this gives us a valid reason to not touch the current implementation and leave it as it is (and of course, it's not possible given the breaking change it introduces). I came up with another idea, which I don't know if it's feasible. What if this kind of behavior/fix can be opted-in through the use of some flag or property (maybe something in PlatformTextStyle)?

@yamin8000
Copy link
yamin8000 commented Aug 13, 2023

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.

4 participants